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

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> 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.

Okay, you care for the headline comment and the grouping more than I
dislike them, so I'll keep them both.