Re: [Qemu-block] [PATCH 3/7] block/qapi: Move 'aio' option to file driver

[Eric Blake]

On 09/22/2016 05:25 AM, Kevin Wolf wrote:

>>>  
>>> +static bool get_aio_option(QemuOpts *opts, int flags, Error **errp)
>>> +{
>>> +    const char *aio = qemu_opt_get(opts, "aio");
>>> +    if (!aio) {
>>> +        return !!(flags & BDRV_O_NATIVE_AIO);
>>> +    } else if (!strcmp(aio, "native")) {
>>> +        return true;
>>> +    } else if (!strcmp(aio, "threads")) {
>>> +        return false;
>>> +    }
>>> +
>>> +    error_setg(errp, "invalid aio option");
>>> +    return false;
>>> +}
>>
>> Is there somewhere common to share this, to avoid duplication?
> 
> I don't know where I would put it. This is a driver-specific option, so
> it doesn't belong in the generic block layer. It's just that two drivers
> happen to provide the same option currently. If we add another backend
> to raw-posix, raw-win32 wouldn't get the new option, so maybe leaving
> them separate is the best anyway.
> 

Fair enough...

> I guess I could do something like this to make the "duplicated" code
> look somewhat smaller, or at least condensed into a single statement:
> 
>     BlockdevAioOptions aio =
>         qapi_enum_parse(BlockdevAioOptions_lookup,
>                         qemu_opt_get(opts, "aio"),
>                         BLOCKDEV_AIO_OPTIONS__MAX,
>                         (flags & BDRV_O_NATIVE_AIO) ?
>                             BLOCKDEV_AIO_OPTIONS_NATIVE :
>                             BLOCKDEV_AIO_OPTIONS_THREADS);
>     s->use_linux_aio = (aio == BLOCKDEV_AIO_OPTIONS_NATIVE);
> 
> Would you consider this an improvement?

Yes, that looks nicer, because it's not hand-rolling the parse. If, in
the future, we do diverge and add a mode in posix that is not available
in win32, that just means we would have two separate QAPI enums, but
keep the qapi_enum_parse() in place for no additional if/else branches.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature