Re: [Qemu-devel] [PATCH v3 18/23] blockdev: Fix blockdev-add not to create IDE drive (0, 0)

[Kevin Wolf]

Am 29.09.2014 um 15:05 hat Markus Armbruster geschrieben:
> Kevin Wolf <address@hidden> writes:
> 
> > Am 16.09.2014 um 20:12 hat Markus Armbruster geschrieben:
> >> @@ -903,21 +899,19 @@ DriveInfo *drive_new(QemuOpts *all_opts, 
> >> BlockInterfaceType block_default_type)
> >>      }
> >>  
> >>      /* Set legacy DriveInfo fields */
> >> -    dinfo = blk_legacy_dinfo(blk);
> >> +    dinfo = g_malloc0(sizeof(*dinfo));
> >>      dinfo->enable_auto_del = true;
> >>      dinfo->opts = all_opts;
> >> -
> >>      dinfo->cyls = cyls;
> >>      dinfo->heads = heads;
> >>      dinfo->secs = secs;
> >>      dinfo->trans = translation;
> >> -
> >>      dinfo->type = type;
> >>      dinfo->bus = bus_id;
> >>      dinfo->unit = unit_id;
> >>      dinfo->devaddr = devaddr;
> >> -
> >>      dinfo->serial = g_strdup(serial);
> >> +    blk_set_legacy_dinfo(blk, dinfo);
> >
> > You don't like the grouping?
> 
> No, because the comment appears as if it applied only to the first
> group.
> 
> This is how this spot looks at the end of the series:
> 
>     /* Set legacy DriveInfo fields */
>     dinfo = g_malloc0(sizeof(*dinfo));
>     dinfo->opts = all_opts;
>     dinfo->cyls = cyls;
>     dinfo->heads = heads;
>     dinfo->secs = secs;
>     dinfo->trans = translation;
>     dinfo->type = type;
>     dinfo->bus = bus_id;
>     dinfo->unit = unit_id;
>     dinfo->devaddr = devaddr;
>     dinfo->serial = g_strdup(serial);
>     blk_set_legacy_dinfo(blk, dinfo);
> 
> Could do this instead:
> 
>     dinfo = g_malloc0(sizeof(*dinfo));
>     dinfo->opts = all_opts;
> 
>     dinfo->cyls = cyls;
>     dinfo->heads = heads;
>     dinfo->secs = secs;
>     dinfo->trans = translation;
> 
>     dinfo->type = type;
>     dinfo->bus = bus_id;
>     dinfo->unit = unit_id;
>     dinfo->devaddr = devaddr;
>     dinfo->serial = g_strdup(serial);
> 
>     blk_set_legacy_dinfo(blk, dinfo);
> 
> The comment is next to useless anyway.  Got a preference?

The reason why it's there is that I like to use comments to give
"headlines" to the blocks of code. In drive_new(), I can only read the
comments without looking at the code and understand what functionality
is implemented at a high level.

So for me the "headline" is valid until the next one appears (except
that this is the last one) and it's good as it is today. Your taste
differs, as it seems.

If I have to choose between your two alternatives, I'll reluctantly vote
for removing the empty lines, because making it part of the "Actual
block device init" block by removing the comment makes even less sense.

Kevin