On 05/13/2014 06:06 AM, Peter Lieven wrote:
>>> - changed the parse function in blockdev_init to
>>> be generic usable for other enum parameters
>> If you wouldn't mind, I think the generic function is useful enough that
>> people might want to backport it independently from this optimization.
>> It would be better to split this into a two-patch series, one for the
>> new parse_enum_option, the other for bdrv_write_zeroes utilizing it.
>>
>>
>>> + },{
>>> + .name = "detect-zeroes",
>>> + .type = QEMU_OPT_STRING,
>>> + .help = "try to optimize zero writes",
>> Might be worth listing (off, on, unmap) in the text.
>>
>> Everything else looked okay, but I'll wait for R-b until I see a
>> response about the idea of splitting the patch (even if that response is
>> justification for keeping it as one)
>>
>
> I did not split because currently there is no other possible
> user in the function. The on_error settings and discard settings
> would be possible users, but for on_error there is a hardcoded
> difference between read and write which is not reflected in the
> qapi and for discard settings we have ignore and unmap, but
> we have also off and on which are not in qapi as well.Even if there is currently only one caller of the new function, it's STILL better to split patches into manageable pieces - psychologically, it's easier to review a new function in isolation. Besides, just because there's currently only one user (the rest of your patch) does not rule out that someone else may start using the function, and then a backport would target that later commit plus your function. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
