qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [PATCH for-4.1] qemu-nbd: Look up flag names in array


From: Eric Blake
Subject: Re: [Qemu-block] [PATCH for-4.1] qemu-nbd: Look up flag names in array
Date: Fri, 5 Apr 2019 14:38:49 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1

On 4/5/19 2:16 PM, Max Reitz wrote:
> The existing code to convert flag bits into strings looks a bit strange
> now, and if we ever add more flags, it will look even stranger.  Prevent
> that from happening by making it look up the flag names in an array.

At one point, I even considered using a QAPI type and expressing things
in a way where we could add an --output=json and/or use our existing
QObject-to-human-readable formatters instead of open-coding things in
qemu-nbd.  But this patch is worthwhile in the meantime.

> 
> Signed-off-by: Max Reitz <address@hidden>
> ---
> Looking at the diff stat I can't claim this patch really improves
> anything by much, but the current code just pains me so.
> ---
>  include/block/nbd.h | 38 +++++++++++++++++++++++++------------
>  qemu-nbd.c          | 46 +++++++++++++++++----------------------------
>  2 files changed, 43 insertions(+), 41 deletions(-)

Reviewed-by: Eric Blake <address@hidden>

Will apply through my NBD tree for 4.1 (no semantic change, so -rc3 is
indeed too late for taking this into 4.0)

Hmm. nbdkit uses a generated header that scrapes the definitions of
various bit values and produces automated value-to-name lookup
functions, rather than having to open-code the lookup functions. Perhaps
we could consider doing similar in qemu for even less code to maintain.


> +                [NBD_FLAG_SEND_CACHE_BIT]           = "cache",
> +            };
> +
>              printf("  size:  %" PRIu64 "\n", list[i].size);
>              printf("  flags: 0x%x (", list[i].flags);
> -            if (list[i].flags & NBD_FLAG_READ_ONLY) {
> -                printf(" readonly");
> -            }

I'm also wondering if I should have added another nbd_*_lookup()
function in nbd/common.c to do this lookup (even if we still like the
array approach over open-coding, and whether or not we like the idea of
generating things from the .h, having all of the lookup functions in one
place and style makes more sense than special-casing NBD_FLAG_ lookups
to just live in qemu-nbd.c).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]