Re: [Qemu-devel] [PATCH v2 41/45] block: New bdrv_set_buffer_alignment()

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> Am 02.09.2011 17:30, schrieb Markus Armbruster:
>> Kevin Wolf <address@hidden> writes:
>> 
>>> Am 03.08.2011 15:08, schrieb Markus Armbruster:
>>>> Device models should be able to set it without an unclean include of
>>>> block_int.h.
>>>>
>>>> Signed-off-by: Markus Armbruster <address@hidden>
>>>> ---
>>>>  block.c         |    6 ++++--
>>>>  block.h         |    1 +
>>>>  hw/ide/core.c   |    2 +-
>>>>  hw/scsi-disk.c  |    2 +-
>>>>  hw/virtio-blk.c |    3 +--
>>>>  5 files changed, 8 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/block.c b/block.c
>>>> index fed0c16..67d9429 100644
>>>> --- a/block.c
>>>> +++ b/block.c
>>>> @@ -453,7 +453,6 @@ static int bdrv_open_common(BlockDriverState *bs, 
>>>> const char *filename,
>>>>      bs->encrypted = 0;
>>>>      bs->valid_key = 0;
>>>>      bs->open_flags = flags;
>>>> -    /* buffer_alignment defaulted to 512, drivers can change this value */
>>>>      bs->buffer_alignment = 512;
>>>
>>> This comment is still right.
>> 
>> It's also next to useless.  I hate comments paraphrasing the code :)
>> 
>> I'll keep it if you insist.
>
> The information that the comment gives isn't that we set it to 512, but
> that this is only a default and "drivers" (should be "devices", I think)
> are expected to change it.

Devices can and do change any number of default values set up here, and
documenting that just for buffer_alignment is useless at best,
misleading at worst.

Nevertheless, I'll keep it if you insist.