include/block/nbd.h | 38 +++++++++++++++++++++++++------------ qemu-nbd.c | 46 +++++++++++++++++---------------------------- 2 files changed, 43 insertions(+), 41 deletions(-)
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.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
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(-)
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 6d05983a55..bb9f5bc021 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -127,18 +127,32 @@ typedef struct NBDExtent {
/* Transmission (export) flags: sent from server to client during handshake,
but describe what will happen during transmission */
-#define NBD_FLAG_HAS_FLAGS (1 << 0) /* Flags are there */
-#define NBD_FLAG_READ_ONLY (1 << 1) /* Device is read-only */
-#define NBD_FLAG_SEND_FLUSH (1 << 2) /* Send FLUSH */
-#define NBD_FLAG_SEND_FUA (1 << 3) /* Send FUA (Force Unit Access) */
-#define NBD_FLAG_ROTATIONAL (1 << 4) /* Use elevator algorithm -
- rotational media */
-#define NBD_FLAG_SEND_TRIM (1 << 5) /* Send TRIM (discard) */
-#define NBD_FLAG_SEND_WRITE_ZEROES (1 << 6) /* Send WRITE_ZEROES */
-#define NBD_FLAG_SEND_DF (1 << 7) /* Send DF (Do not Fragment) */
-#define NBD_FLAG_CAN_MULTI_CONN (1 << 8) /* Multi-client cache consistent */
-#define NBD_FLAG_SEND_RESIZE (1 << 9) /* Send resize */
-#define NBD_FLAG_SEND_CACHE (1 << 10) /* Send CACHE (prefetch) */
+enum {
+ NBD_FLAG_HAS_FLAGS_BIT = 0, /* Flags are there */
+ NBD_FLAG_READ_ONLY_BIT = 1, /* Device is read-only */
+ NBD_FLAG_SEND_FLUSH_BIT = 2, /* Send FLUSH */
+ NBD_FLAG_SEND_FUA_BIT = 3, /* Send FUA (Force Unit Access) */
+ NBD_FLAG_ROTATIONAL_BIT = 4, /* Use elevator algorithm -
+ rotational media */
+ NBD_FLAG_SEND_TRIM_BIT = 5, /* Send TRIM (discard) */
+ NBD_FLAG_SEND_WRITE_ZEROES_BIT = 6, /* Send WRITE_ZEROES */
+ NBD_FLAG_SEND_DF_BIT = 7, /* Send DF (Do not Fragment) */
+ NBD_FLAG_CAN_MULTI_CONN_BIT = 8, /* Multi-client cache consistent */
+ NBD_FLAG_SEND_RESIZE_BIT = 9, /* Send resize */
+ NBD_FLAG_SEND_CACHE_BIT = 10, /* Send CACHE (prefetch) */
+};
+
+#define NBD_FLAG_HAS_FLAGS (1 << NBD_FLAG_HAS_FLAGS_BIT)
+#define NBD_FLAG_READ_ONLY (1 << NBD_FLAG_READ_ONLY_BIT)
+#define NBD_FLAG_SEND_FLUSH (1 << NBD_FLAG_SEND_FLUSH_BIT)
+#define NBD_FLAG_SEND_FUA (1 << NBD_FLAG_SEND_FUA_BIT)
+#define NBD_FLAG_ROTATIONAL (1 << NBD_FLAG_ROTATIONAL_BIT)
+#define NBD_FLAG_SEND_TRIM (1 << NBD_FLAG_SEND_TRIM_BIT)
+#define NBD_FLAG_SEND_WRITE_ZEROES (1 << NBD_FLAG_SEND_WRITE_ZEROES_BIT)
+#define NBD_FLAG_SEND_DF (1 << NBD_FLAG_SEND_DF_BIT)
+#define NBD_FLAG_CAN_MULTI_CONN (1 << NBD_FLAG_CAN_MULTI_CONN_BIT)
+#define NBD_FLAG_SEND_RESIZE (1 << NBD_FLAG_SEND_RESIZE_BIT)
+#define NBD_FLAG_SEND_CACHE (1 << NBD_FLAG_SEND_CACHE_BIT)
/* New-style handshake (global) flags, sent from server to client, and
control what will happen during handshake phase. */
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 941ba729c2..a9c85db58e 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -279,37 +279,25 @@ static int qemu_nbd_client_list(SocketAddress *saddr, QCryptoTLSCreds *tls,
printf(" description: %s\n", list[i].description);
}
if (list[i].flags & NBD_FLAG_HAS_FLAGS) {
+ static const char *const flag_names[] = {
+ [NBD_FLAG_READ_ONLY_BIT] = "readonly",
+ [NBD_FLAG_SEND_FLUSH_BIT] = "flush",
+ [NBD_FLAG_SEND_FUA_BIT] = "fua",
+ [NBD_FLAG_ROTATIONAL_BIT] = "rotational",
+ [NBD_FLAG_SEND_TRIM_BIT] = "trim",
+ [NBD_FLAG_SEND_WRITE_ZEROES_BIT] = "zeroes",
+ [NBD_FLAG_SEND_DF_BIT] = "df",
+ [NBD_FLAG_CAN_MULTI_CONN_BIT] = "multi",
+ [NBD_FLAG_SEND_RESIZE_BIT] = "resize",
+ [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");
- }
- if (list[i].flags & NBD_FLAG_SEND_FLUSH) {
- printf(" flush");
- }
- if (list[i].flags & NBD_FLAG_SEND_FUA) {
- printf(" fua");
- }
- if (list[i].flags & NBD_FLAG_ROTATIONAL) {
- printf(" rotational");
- }
- if (list[i].flags & NBD_FLAG_SEND_TRIM) {
- printf(" trim");
- }
- if (list[i].flags & NBD_FLAG_SEND_WRITE_ZEROES) {
- printf(" zeroes");
- }
- if (list[i].flags & NBD_FLAG_SEND_DF) {
- printf(" df");
- }
- if (list[i].flags & NBD_FLAG_CAN_MULTI_CONN) {
- printf(" multi");
- }
- if (list[i].flags & NBD_FLAG_SEND_RESIZE) {
- printf(" resize");
- }
- if (list[i].flags & NBD_FLAG_SEND_CACHE) {
- printf(" cache");
+ for (size_t bit = 0; bit < ARRAY_SIZE(flag_names); bit++) {
+ if (flag_names[bit] && (list[i].flags & (1 << bit))) {
+ printf(" %s", flag_names[bit]);
+ }
}
printf(" )\n");
}
--
2.20.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 <mreitz@redhat.com>
> ---
> 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 <eblake@redhat.com>
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
© 2016 - 2024 Red Hat, Inc.