| Message ID | 20171012095319.136610-12-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | nbd minimal structured read | expand |
On 10/12/2017 04:53 AM, Vladimir Sementsov-Ogievskiy wrote: > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > include/block/nbd.h | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ > nbd/nbd-internal.h | 25 ------------------------- > nbd/client.c | 32 -------------------------------- > 3 files changed, 48 insertions(+), 57 deletions(-) > > +static inline int nbd_errno_to_system_errno(int err) > +{ > + switch (err) { > + case NBD_SUCCESS: > + return 0; > + case NBD_EPERM: > + return EPERM; > + case NBD_EIO: > + return EIO; > + case NBD_ENOMEM: > + return ENOMEM; > + case NBD_ENOSPC: > + return ENOSPC; > + case NBD_ESHUTDOWN: > + return ESHUTDOWN; > + case NBD_EINVAL: > + return EINVAL; > + } > + > + return EINVAL; > +} This lacks a trace... > +++ b/nbd/client.c > @@ -22,38 +22,6 @@ > #include "trace.h" > #include "nbd-internal.h" > > -static int nbd_errno_to_system_errno(int err) > -{ > - int ret; > - switch (err) { > - case NBD_SUCCESS: > - ret = 0; > - break; > - case NBD_EPERM: > - ret = EPERM; > - break; > - case NBD_EIO: > - ret = EIO; > - break; > - case NBD_ENOMEM: > - ret = ENOMEM; > - break; > - case NBD_ENOSPC: > - ret = ENOSPC; > - break; > - case NBD_ESHUTDOWN: > - ret = ESHUTDOWN; > - break; > - default: > - trace_nbd_unknown_error(err); > - /* fallthrough */ > - case NBD_EINVAL: ...that was present here. And you didn't do straight code motion, but modified things on the way (hence why checkpatch complained that your more concise version is suspicious with regards to returning positive errno). Does the function still need to be static inline, or should we just declare a prototype in the header and put the function itself in nbd/common.c?
13.10.2017 21:47, Eric Blake wrote: > On 10/12/2017 04:53 AM, Vladimir Sementsov-Ogievskiy wrote: >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> include/block/nbd.h | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ >> nbd/nbd-internal.h | 25 ------------------------- >> nbd/client.c | 32 -------------------------------- >> 3 files changed, 48 insertions(+), 57 deletions(-) >> >> +static inline int nbd_errno_to_system_errno(int err) >> +{ >> + switch (err) { >> + case NBD_SUCCESS: >> + return 0; >> + case NBD_EPERM: >> + return EPERM; >> + case NBD_EIO: >> + return EIO; >> + case NBD_ENOMEM: >> + return ENOMEM; >> + case NBD_ENOSPC: >> + return ENOSPC; >> + case NBD_ESHUTDOWN: >> + return ESHUTDOWN; >> + case NBD_EINVAL: >> + return EINVAL; >> + } >> + >> + return EINVAL; >> +} > This lacks a trace... > >> +++ b/nbd/client.c >> @@ -22,38 +22,6 @@ >> #include "trace.h" >> #include "nbd-internal.h" >> >> -static int nbd_errno_to_system_errno(int err) >> -{ >> - int ret; >> - switch (err) { >> - case NBD_SUCCESS: >> - ret = 0; >> - break; >> - case NBD_EPERM: >> - ret = EPERM; >> - break; >> - case NBD_EIO: >> - ret = EIO; >> - break; >> - case NBD_ENOMEM: >> - ret = ENOMEM; >> - break; >> - case NBD_ENOSPC: >> - ret = ENOSPC; >> - break; >> - case NBD_ESHUTDOWN: >> - ret = ESHUTDOWN; >> - break; >> - default: >> - trace_nbd_unknown_error(err); >> - /* fallthrough */ >> - case NBD_EINVAL: > ...that was present here. And you didn't do straight code motion, but hmm, sorry. > modified things on the way (hence why checkpatch complained that your > more concise version is suspicious with regards to returning positive > errno). Does the function still need to be static inline, or should we > just declare a prototype in the header and put the function itself in > nbd/common.c? > ok for me to do so.
On 10/12/2017 04:53 AM, Vladimir Sementsov-Ogievskiy wrote: > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > include/block/nbd.h | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ > nbd/nbd-internal.h | 25 ------------------------- > nbd/client.c | 32 -------------------------------- > 3 files changed, 48 insertions(+), 57 deletions(-) > Another thing I noticed looking at this patch: > diff --git a/include/block/nbd.h b/include/block/nbd.h > index dd261f66f0..09e4592971 100644 > --- a/include/block/nbd.h > +++ b/include/block/nbd.h > @@ -77,6 +77,9 @@ typedef struct NBDStructuredReplyChunk { > uint32_t length; /* length of payload */ > } QEMU_PACKED NBDStructuredReplyChunk; > > +#define NBD_SIMPLE_REPLY_MAGIC 0x67446698 > +#define NBD_STRUCTURED_REPLY_MAGIC 0x668e33ef We have some churn here, as we defined this earlier in the series. Also, in an ideal world (although I don't know if we're quite there), we should be able to backport patches for JUST the server, or for JUST the client, in isolation, to talk to an independent implementation on the other side of the wire. To do that, it may be better to define all our new constants in a standalone patch, rather than embedded as part of the server implementation in 9/13. I think at this point, I will take 1-8 as amended, and even prepare a pull request for those, and then post a v4 series based on your work but with some things moved around, and get consensus on my changes, before worrying about the pull request for the second half.
On 10/12/2017 04:53 AM, Vladimir Sementsov-Ogievskiy wrote: > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > include/block/nbd.h | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ > nbd/nbd-internal.h | 25 ------------------------- > nbd/client.c | 32 -------------------------------- > 3 files changed, 48 insertions(+), 57 deletions(-) > By the end of your series, I still don't see any use of > +++ b/include/block/nbd.h > @@ -235,4 +272,15 @@ void nbd_client_put(NBDClient *client); > void nbd_server_start(SocketAddress *addr, const char *tls_creds, > Error **errp); > > +/* nbd_read > + * Reads @size bytes from @ioc. Returns 0 on success. > + */ > +static inline int nbd_read(QIOChannel *ioc, void *buffer, size_t size, > + Error **errp) > +{ > + return qio_channel_read_all(ioc, buffer, size, errp) < 0 ? -EIO : 0; > +} > + > +int nbd_drop(QIOChannel *ioc, size_t size, Error **errp); either of these functions in block/nbd-client.c. I think that's a good thing (we refactored it so that nbd/client.c is doing ALL the reading from the wire, and block/nbd-client.c is relying on nbd/client.c to do the work), but it means this part of the patch is no longer necessary, unless I'm missing something.
diff --git a/include/block/nbd.h b/include/block/nbd.h index dd261f66f0..09e4592971 100644 --- a/include/block/nbd.h +++ b/include/block/nbd.h @@ -77,6 +77,9 @@ typedef struct NBDStructuredReplyChunk { uint32_t length; /* length of payload */ } QEMU_PACKED NBDStructuredReplyChunk; +#define NBD_SIMPLE_REPLY_MAGIC 0x67446698 +#define NBD_STRUCTURED_REPLY_MAGIC 0x668e33ef + typedef struct NBDStructuredRead { NBDStructuredReplyChunk h; uint64_t offset; @@ -182,6 +185,40 @@ enum { #define NBD_SREP_TYPE_ERROR NBD_SREP_ERR(1) #define NBD_SREP_TYPE_ERROR_OFFSET NBD_SREP_ERR(2) +/* NBD errors are based on errno numbers, so there is a 1:1 mapping, + * but only a limited set of errno values is specified in the protocol. + * Everything else is squashed to EINVAL. + */ +#define NBD_SUCCESS 0 +#define NBD_EPERM 1 +#define NBD_EIO 5 +#define NBD_ENOMEM 12 +#define NBD_EINVAL 22 +#define NBD_ENOSPC 28 +#define NBD_ESHUTDOWN 108 + +static inline int nbd_errno_to_system_errno(int err) +{ + switch (err) { + case NBD_SUCCESS: + return 0; + case NBD_EPERM: + return EPERM; + case NBD_EIO: + return EIO; + case NBD_ENOMEM: + return ENOMEM; + case NBD_ENOSPC: + return ENOSPC; + case NBD_ESHUTDOWN: + return ESHUTDOWN; + case NBD_EINVAL: + return EINVAL; + } + + return EINVAL; +} + /* Details collected by NBD_OPT_EXPORT_NAME and NBD_OPT_GO */ struct NBDExportInfo { /* Set by client before nbd_receive_negotiate() */ @@ -235,4 +272,15 @@ void nbd_client_put(NBDClient *client); void nbd_server_start(SocketAddress *addr, const char *tls_creds, Error **errp); +/* nbd_read + * Reads @size bytes from @ioc. Returns 0 on success. + */ +static inline int nbd_read(QIOChannel *ioc, void *buffer, size_t size, + Error **errp) +{ + return qio_channel_read_all(ioc, buffer, size, errp) < 0 ? -EIO : 0; +} + +int nbd_drop(QIOChannel *ioc, size_t size, Error **errp); + #endif diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h index beb30a7a3e..970b560d11 100644 --- a/nbd/nbd-internal.h +++ b/nbd/nbd-internal.h @@ -47,8 +47,6 @@ #define NBD_OLDSTYLE_NEGOTIATE_SIZE (8 + 8 + 8 + 4 + 124) #define NBD_REQUEST_MAGIC 0x25609513 -#define NBD_SIMPLE_REPLY_MAGIC 0x67446698 -#define NBD_STRUCTURED_REPLY_MAGIC 0x668e33ef #define NBD_OPTS_MAGIC 0x49484156454F5054LL #define NBD_CLIENT_MAGIC 0x0000420281861253LL #define NBD_REP_MAGIC 0x0003e889045565a9LL @@ -65,18 +63,6 @@ #define NBD_SET_TIMEOUT _IO(0xab, 9) #define NBD_SET_FLAGS _IO(0xab, 10) -/* NBD errors are based on errno numbers, so there is a 1:1 mapping, - * but only a limited set of errno values is specified in the protocol. - * Everything else is squashed to EINVAL. - */ -#define NBD_SUCCESS 0 -#define NBD_EPERM 1 -#define NBD_EIO 5 -#define NBD_ENOMEM 12 -#define NBD_EINVAL 22 -#define NBD_ENOSPC 28 -#define NBD_ESHUTDOWN 108 - /* nbd_read_eof * Tries to read @size bytes from @ioc. * Returns 1 on success @@ -96,15 +82,6 @@ static inline int nbd_read_eof(QIOChannel *ioc, void *buffer, size_t size, return ret; } -/* nbd_read - * Reads @size bytes from @ioc. Returns 0 on success. - */ -static inline int nbd_read(QIOChannel *ioc, void *buffer, size_t size, - Error **errp) -{ - return qio_channel_read_all(ioc, buffer, size, errp) < 0 ? -EIO : 0; -} - /* nbd_write * Writes @size bytes to @ioc. Returns 0 on success. */ @@ -128,6 +105,4 @@ const char *nbd_rep_lookup(uint32_t rep); const char *nbd_info_lookup(uint16_t info); const char *nbd_cmd_lookup(uint16_t info); -int nbd_drop(QIOChannel *ioc, size_t size, Error **errp); - #endif diff --git a/nbd/client.c b/nbd/client.c index c8702a80b1..f0f3075569 100644 --- a/nbd/client.c +++ b/nbd/client.c @@ -22,38 +22,6 @@ #include "trace.h" #include "nbd-internal.h" -static int nbd_errno_to_system_errno(int err) -{ - int ret; - switch (err) { - case NBD_SUCCESS: - ret = 0; - break; - case NBD_EPERM: - ret = EPERM; - break; - case NBD_EIO: - ret = EIO; - break; - case NBD_ENOMEM: - ret = ENOMEM; - break; - case NBD_ENOSPC: - ret = ENOSPC; - break; - case NBD_ESHUTDOWN: - ret = ESHUTDOWN; - break; - default: - trace_nbd_unknown_error(err); - /* fallthrough */ - case NBD_EINVAL: - ret = EINVAL; - break; - } - return ret; -} - /* Definitions for opaque data types */ static QTAILQ_HEAD(, NBDExport) exports = QTAILQ_HEAD_INITIALIZER(exports);
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- include/block/nbd.h | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ nbd/nbd-internal.h | 25 ------------------------- nbd/client.c | 32 -------------------------------- 3 files changed, 48 insertions(+), 57 deletions(-)