| Message ID | 20170203154757.36140-5-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
On 02/03/2017 09:47 AM, Vladimir Sementsov-Ogievskiy wrote: > Split out nbd_receive_simple_option to be reused for structured reply > option. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > nbd/client.c | 54 +++++++++++++++++++++++++++++++++++------------------- > nbd/nbd-internal.h | 14 ++++++++++++++ > 2 files changed, 49 insertions(+), 19 deletions(-) > > +++ b/nbd/nbd-internal.h > @@ -96,6 +96,20 @@ > #define NBD_ENOSPC 28 > #define NBD_ESHUTDOWN 108 > > +static inline const char *nbd_opt_name(int opt) > +{ > + switch (opt) { > + case NBD_OPT_EXPORT_NAME: return "export_name"; Does this really get past checkpatch? > + case NBD_OPT_ABORT: return "abort"; > + case NBD_OPT_LIST: return "list"; > + case NBD_OPT_PEEK_EXPORT: return "peek_export"; > + case NBD_OPT_STARTTLS: return "tls"; Why just 'tls' instead of 'starttls'? > + case NBD_OPT_STRUCTURED_REPLY: return "structured_reply"; > + } > + > + return "<unknown option>"; Can you please consider making this include the %d representation of the unknown option; perhaps by snprintf'ing into static storage? While it is unlikely that a well-behaved server will respond to a client with an option the client doesn't recognize, it is much more likely that this reverse lookup function will be used in a server to respond to an unknown option from a client. In fact, I might have split this into two patches: one providing nbd_opt_name() and using it throughout the code base where appropriate, and the other refactoring starttls in the client. I'm not sure if the reverse lookup function needs to be inline in the header; it could reasonably live in nbd/common.c, particularly if you are going to take my advice to have it format a message for unknown values.
07.02.2017 19:32, Eric Blake wrote: > On 02/03/2017 09:47 AM, Vladimir Sementsov-Ogievskiy wrote: >> Split out nbd_receive_simple_option to be reused for structured reply >> option. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> nbd/client.c | 54 +++++++++++++++++++++++++++++++++++------------------- >> nbd/nbd-internal.h | 14 ++++++++++++++ >> 2 files changed, 49 insertions(+), 19 deletions(-) >> >> +++ b/nbd/nbd-internal.h >> @@ -96,6 +96,20 @@ >> #define NBD_ENOSPC 28 >> #define NBD_ESHUTDOWN 108 >> >> +static inline const char *nbd_opt_name(int opt) >> +{ >> + switch (opt) { >> + case NBD_OPT_EXPORT_NAME: return "export_name"; > Does this really get past checkpatch? > >> + case NBD_OPT_ABORT: return "abort"; >> + case NBD_OPT_LIST: return "list"; >> + case NBD_OPT_PEEK_EXPORT: return "peek_export"; >> + case NBD_OPT_STARTTLS: return "tls"; > Why just 'tls' instead of 'starttls'? > >> + case NBD_OPT_STRUCTURED_REPLY: return "structured_reply"; >> + } >> + >> + return "<unknown option>"; > Can you please consider making this include the %d representation of the > unknown option; perhaps by snprintf'ing into static storage? While it Hmm.. The caller should free it in this case. Currently, the get_opt_name is called only on error path or if tracing enabled. How to achieve this with variable string return value not complicating the code a lot? When there is unknown message there should be other mechanism to inform user > is unlikely that a well-behaved server will respond to a client with an > option the client doesn't recognize, it is much more likely that this > reverse lookup function will be used in a server to respond to an > unknown option from a client. > > In fact, I might have split this into two patches: one providing > nbd_opt_name() and using it throughout the code base where appropriate, > and the other refactoring starttls in the client. > > I'm not sure if the reverse lookup function needs to be inline in the > header; it could reasonably live in nbd/common.c, particularly if you > are going to take my advice to have it format a message for unknown values. >
On 02/09/2017 12:20 AM, Vladimir Sementsov-Ogievskiy wrote: > 07.02.2017 19:32, Eric Blake wrote: >> On 02/03/2017 09:47 AM, Vladimir Sementsov-Ogievskiy wrote: >>> Split out nbd_receive_simple_option to be reused for structured reply >>> option. >>> + return "<unknown option>"; >> Can you please consider making this include the %d representation of the >> unknown option; perhaps by snprintf'ing into static storage? While it > > Hmm.. The caller should free it in this case. Only if you print it into malloc'd space. I think that printing it into static storage may be sufficient (although then we have a race if more than one thread is trying to use that static storage at the same time - but do we ever have more than one thread trying to handle an error at the same time?).
09.02.2017 17:41, Eric Blake wrote: > On 02/09/2017 12:20 AM, Vladimir Sementsov-Ogievskiy wrote: >> 07.02.2017 19:32, Eric Blake wrote: >>> On 02/03/2017 09:47 AM, Vladimir Sementsov-Ogievskiy wrote: >>>> Split out nbd_receive_simple_option to be reused for structured reply >>>> option. >>>> + return "<unknown option>"; >>> Can you please consider making this include the %d representation of the >>> unknown option; perhaps by snprintf'ing into static storage? While it >> Hmm.. The caller should free it in this case. > Only if you print it into malloc'd space. I think that printing it into > static storage may be sufficient (although then we have a race if more > than one thread is trying to use that static storage at the same time - > but do we ever have more than one thread trying to handle an error at > the same time?). > This race would be if one thread decides to print two option names in one message. Or save one option in a var, then print other, then print var.
On 02/10/2017 05:23 AM, Vladimir Sementsov-Ogievskiy wrote: > 09.02.2017 17:41, Eric Blake wrote: >> On 02/09/2017 12:20 AM, Vladimir Sementsov-Ogievskiy wrote: >>> 07.02.2017 19:32, Eric Blake wrote: >>>> On 02/03/2017 09:47 AM, Vladimir Sementsov-Ogievskiy wrote: >>>>> Split out nbd_receive_simple_option to be reused for structured reply >>>>> option. >>>>> + return "<unknown option>"; >>>> Can you please consider making this include the %d representation of >>>> the >>>> unknown option; perhaps by snprintf'ing into static storage? While it >>> Hmm.. The caller should free it in this case. >> Only if you print it into malloc'd space. I think that printing it into >> static storage may be sufficient (although then we have a race if more >> than one thread is trying to use that static storage at the same time - >> but do we ever have more than one thread trying to handle an error at >> the same time?). >> > > This race would be if one thread decides to print two option names in > one message. Or save one option in a var, then print other, then print var. NBD_OPT_ are supposed to be handled sequentially. The NBD spec does NOT allow for a single client to send a second NBD_OPT_ until after the first one has received a final response. So the only time we would have two threads dealing with things in nbd/client.c during handshake is if we have a single qemu process connecting as client to two different NBD servers in parallel, where both servers issue an otherwise unknown opt response at the same time, which I don't see as likely enough to be worth avoiding static storage. If you're still worried about the race (I'm not), to the point that you don't want to use static storage just to avoid g_malloc(), then another option is to make nbd_opt_name() take an input parameter for a buffer and max size, and let the caller provide stack-based storage for computing the resulting message (similar to how strerror_r does things).
On 02/11/2017 01:30 PM, Eric Blake wrote: >>>>> On 02/03/2017 09:47 AM, Vladimir Sementsov-Ogievskiy wrote: >>>>>> Split out nbd_receive_simple_option to be reused for structured reply >>>>>> option. >>>>>> + return "<unknown option>"; >>>>> Can you please consider making this include the %d representation of >>>>> the >>>>> unknown option; perhaps by snprintf'ing into static storage? While it > If you're still worried about the race (I'm not), to the point that you > don't want to use static storage just to avoid g_malloc(), then another > option is to make nbd_opt_name() take an input parameter for a buffer > and max size, and let the caller provide stack-based storage for > computing the resulting message (similar to how strerror_r does things). Or, as long as the caller prints the value as well as the reverse-mapped name, that would work too. I'm going to add such a reverse-mapping for my NBD_INFO_* patches about to land later today, so I'll make my counter-proposal for NBD_OPT_* along those lines as part of my series.
diff --git a/nbd/client.c b/nbd/client.c index de5c9366c7..6caf6bda6d 100644 --- a/nbd/client.c +++ b/nbd/client.c @@ -395,39 +395,55 @@ static int nbd_receive_query_exports(QIOChannel *ioc, } } -static QIOChannel *nbd_receive_starttls(QIOChannel *ioc, - QCryptoTLSCreds *tlscreds, - const char *hostname, Error **errp) +static int nbd_receive_simple_option(QIOChannel *ioc, int opt, + bool abort_on_notsup, Error **errp) { nbd_opt_reply reply; - QIOChannelTLS *tioc; - struct NBDTLSHandshakeData data = { 0 }; - TRACE("Requesting TLS from server"); - if (nbd_send_option_request(ioc, NBD_OPT_STARTTLS, 0, NULL, errp) < 0) { - return NULL; + TRACE("Requesting '%s' option from server", nbd_opt_name(opt)); + if (nbd_send_option_request(ioc, opt, 0, NULL, errp) < 0) { + return -1; } - TRACE("Getting TLS reply from server"); - if (nbd_receive_option_reply(ioc, NBD_OPT_STARTTLS, &reply, errp) < 0) { - return NULL; + TRACE("Getting '%s' option reply from server", nbd_opt_name(opt)); + if (nbd_receive_option_reply(ioc, opt, &reply, errp) < 0) { + return -1; } if (reply.type != NBD_REP_ACK) { - error_setg(errp, "Server rejected request to start TLS %" PRIx32, - reply.type); - nbd_send_opt_abort(ioc); - return NULL; + error_setg(errp, "Server rejected request for '%s' option: %" PRIx32, + nbd_opt_name(opt), reply.type); + if (abort_on_notsup) { + nbd_send_opt_abort(ioc); + } + return -1; } if (reply.length != 0) { - error_setg(errp, "Start TLS response was not zero %" PRIu32, - reply.length); - nbd_send_opt_abort(ioc); + error_setg(errp, "'%s' option response was not zero %" PRIu32, + nbd_opt_name(opt), reply.length); + if (abort_on_notsup) { + nbd_send_opt_abort(ioc); + } + return -1; + } + + TRACE("%s 'option' approved", nbd_opt_name(opt)); + return 0; +} + +static QIOChannel *nbd_receive_starttls(QIOChannel *ioc, + QCryptoTLSCreds *tlscreds, + const char *hostname, Error **errp) +{ + QIOChannelTLS *tioc; + struct NBDTLSHandshakeData data = { 0 }; + + if (nbd_receive_simple_option(ioc, NBD_OPT_STARTTLS, true, errp) < 0) { return NULL; } - TRACE("TLS request approved, setting up TLS"); + TRACE("Setting up TLS"); tioc = qio_channel_tls_new_client(ioc, tlscreds, hostname, errp); if (!tioc) { return NULL; diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h index 489eeaf887..3284bfc85a 100644 --- a/nbd/nbd-internal.h +++ b/nbd/nbd-internal.h @@ -96,6 +96,20 @@ #define NBD_ENOSPC 28 #define NBD_ESHUTDOWN 108 +static inline const char *nbd_opt_name(int opt) +{ + switch (opt) { + case NBD_OPT_EXPORT_NAME: return "export_name"; + case NBD_OPT_ABORT: return "abort"; + case NBD_OPT_LIST: return "list"; + case NBD_OPT_PEEK_EXPORT: return "peek_export"; + case NBD_OPT_STARTTLS: return "tls"; + case NBD_OPT_STRUCTURED_REPLY: return "structured_reply"; + } + + return "<unknown option>"; +} + static inline ssize_t read_sync(QIOChannel *ioc, void *buffer, size_t size) { struct iovec iov = { .iov_base = buffer, .iov_len = size };
Split out nbd_receive_simple_option to be reused for structured reply option. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- nbd/client.c | 54 +++++++++++++++++++++++++++++++++++------------------- nbd/nbd-internal.h | 14 ++++++++++++++ 2 files changed, 49 insertions(+), 19 deletions(-)