| Message ID | 20230608135653.2918540-3-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qemu patches for 64-bit NBD extensions | expand |
On Thu, Jun 08, 2023 at 08:56:31AM -0500, Eric Blake wrote: > We had a mix of struct declarataions followed by typedefs, and direct declarations > struct definitions as part of a typedef. Pick a single style. Also > float a couple of opaque typedefs earlier in the file, as a later > patch wants to refer NBDExport* in NBDRequest. No semantic impact. The curse of writing a commit message and then rebasing to a different idea; in patch 22, I had originally intended to make NBDMetaContexts a concrete type in nbd.h (which depends on NBDExport*, and would be directly used in NBDRequest, which in turn is declared before the pre-patch mention of NBDExport), but then changed my mind to instead have NBDMetaContexts itself also be an opaque type with NBDRequest only using NBDMetaContexts*. And I missed floating the typedef for NBDClientConnection to the same point, because we somewhat separated opaque types along the lines of which .c files provide various functions and opaque types. > @@ -26,24 +26,25 @@ > #include "qapi/error.h" > #include "qemu/bswap.h" > > +typedef struct NBDExport NBDExport; > +typedef struct NBDClient NBDClient; > + Preferences on how I should tweak that aspect of this patch? Options: - Don't float NBDExport or NBDClient, and drop that part of the commit message. However, the later patch that adds the typedef for NBDMetaContexts still has to do it earlier than the definition of NBDRequest, rather than alongside the other opaque types relevant to server.c - Also float NBDClientConnection up here, and reword the commit message along the lines of: Also float forward declarations of opaque types to the top of the file, rather than interspersed with function declarations, which will help a future patch that wants to expose yet another opaque type that will be referenced in NBDRequest. - something else?
On 08.06.23 17:17, Eric Blake wrote: > On Thu, Jun 08, 2023 at 08:56:31AM -0500, Eric Blake wrote: >> We had a mix of struct declarataions followed by typedefs, and direct > > declarations > >> struct definitions as part of a typedef. Pick a single style. Also >> float a couple of opaque typedefs earlier in the file, as a later >> patch wants to refer NBDExport* in NBDRequest. No semantic impact. > > The curse of writing a commit message and then rebasing to a different > idea; in patch 22, I had originally intended to make NBDMetaContexts a > concrete type in nbd.h (which depends on NBDExport*, and would be > directly used in NBDRequest, which in turn is declared before the > pre-patch mention of NBDExport), but then changed my mind to instead > have NBDMetaContexts itself also be an opaque type with NBDRequest > only using NBDMetaContexts*. And I missed floating the typedef for > NBDClientConnection to the same point, because we somewhat separated > opaque types along the lines of which .c files provide various > functions and opaque types. > >> @@ -26,24 +26,25 @@ >> #include "qapi/error.h" >> #include "qemu/bswap.h" >> >> +typedef struct NBDExport NBDExport; >> +typedef struct NBDClient NBDClient; >> + > > Preferences on how I should tweak that aspect of this patch? Options: > > - Don't float NBDExport or NBDClient, and drop that part of the commit > message. However, the later patch that adds the typedef for > NBDMetaContexts still has to do it earlier than the definition of > NBDRequest, rather than alongside the other opaque types relevant to > server.c > > - Also float NBDClientConnection up here, and reword the commit > message along the lines of: Also float forward declarations of > opaque types to the top of the file, rather than interspersed with > function declarations, which will help a future patch that wants to > expose yet another opaque type that will be referenced in > NBDRequest. Sounds good to me. Anyway: Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > > - something else? >
diff --git a/include/block/nbd.h b/include/block/nbd.h index a4c98169c39..9c3ceae5ba5 100644 --- a/include/block/nbd.h +++ b/include/block/nbd.h @@ -1,5 +1,5 @@ /* - * Copyright (C) 2016-2022 Red Hat, Inc. + * Copyright Red Hat * Copyright (C) 2005 Anthony Liguori <anthony@codemonkey.ws> * * Network Block Device @@ -26,24 +26,25 @@ #include "qapi/error.h" #include "qemu/bswap.h" +typedef struct NBDExport NBDExport; +typedef struct NBDClient NBDClient; + extern const BlockExportDriver blk_exp_nbd; /* Handshake phase structs - this struct is passed on the wire */ -struct NBDOption { +typedef struct NBDOption { uint64_t magic; /* NBD_OPTS_MAGIC */ uint32_t option; /* NBD_OPT_* */ uint32_t length; -} QEMU_PACKED; -typedef struct NBDOption NBDOption; +} QEMU_PACKED NBDOption; -struct NBDOptionReply { +typedef struct NBDOptionReply { uint64_t magic; /* NBD_REP_MAGIC */ uint32_t option; /* NBD_OPT_* */ uint32_t type; /* NBD_REP_* */ uint32_t length; -} QEMU_PACKED; -typedef struct NBDOptionReply NBDOptionReply; +} QEMU_PACKED NBDOptionReply; typedef struct NBDOptionReplyMetaContext { NBDOptionReply h; /* h.type = NBD_REP_META_CONTEXT, h.length > 4 */ @@ -56,14 +57,13 @@ typedef struct NBDOptionReplyMetaContext { * Note: these are _NOT_ the same as the network representation of an NBD * request and reply! */ -struct NBDRequest { +typedef struct NBDRequest { uint64_t handle; uint64_t from; uint32_t len; uint16_t flags; /* NBD_CMD_FLAG_* */ uint16_t type; /* NBD_CMD_* */ -}; -typedef struct NBDRequest NBDRequest; +} NBDRequest; typedef struct NBDSimpleReply { uint32_t magic; /* NBD_SIMPLE_REPLY_MAGIC */ @@ -282,7 +282,7 @@ static inline bool nbd_reply_type_is_error(int type) #define NBD_ESHUTDOWN 108 /* Details collected by NBD_OPT_EXPORT_NAME and NBD_OPT_GO */ -struct NBDExportInfo { +typedef struct NBDExportInfo { /* Set by client before nbd_receive_negotiate() */ bool request_sizes; char *x_dirty_bitmap; @@ -310,8 +310,7 @@ struct NBDExportInfo { char *description; int n_contexts; char **contexts; -}; -typedef struct NBDExportInfo NBDExportInfo; +} NBDExportInfo; int nbd_receive_negotiate(AioContext *aio_context, QIOChannel *ioc, QCryptoTLSCreds *tlscreds, @@ -330,9 +329,6 @@ int nbd_client(int fd); int nbd_disconnect(int fd); int nbd_errno_to_system_errno(int err); -typedef struct NBDExport NBDExport; -typedef struct NBDClient NBDClient; - void nbd_export_set_on_eject_blk(BlockExport *exp, BlockBackend *blk); AioContext *nbd_export_aio_context(NBDExport *exp);
We had a mix of struct declarataions followed by typedefs, and direct struct definitions as part of a typedef. Pick a single style. Also float a couple of opaque typedefs earlier in the file, as a later patch wants to refer NBDExport* in NBDRequest. No semantic impact. Signed-off-by: Eric Blake <eblake@redhat.com> --- v4: new patch --- include/block/nbd.h | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-)