On 10/30/2017 09:11 PM, Eric Blake wrote: > On 10/30/2017 06:22 PM, Vladimir Sementsov-Ogievskiy wrote: >> 27.10.2017 13:40, Eric Blake wrote: >>> Consolidate the response for a non-zero-length option payload >>> into a new function, nbd_reject_length(). This check will >>> also be used when introducing support for structured replies. >>>
>>> + if (length) { >>> + /* Unconditionally drop the connection if the client >>> + * can't start a TLS negotiation correctly */ >>> + nbd_reject_length(client, length, option, true, >>> errp); >>> + return -EINVAL; >> >> why not to return nbd_reject_length's result? this EINVAL may not >> correspond to errp (about EIO for example).. > > Somewhat true, if nbd_reject_length() fails. But nbd_reject_length() may > also return 0 without setting errp, in which case, maybe this code > should set errp rather than just blindly returning -EINVAL. > >> >> with or without this fixed: >> >> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> >> > > Okay, I'll squash this in, and include it in my pull request to be sent > shortly. D'oh. Long week for me. The whole reason I added a 'bool fatal' parameter was so that I don't have to worry about nbd_reject_length() returning 0. So it is instead better to just do: > --- i/nbd/server.c > +++ w/nbd/server.c > @@ -684,8 +684,13 @@ static int nbd_negotiate_options(NBDClient *client, > uint16_t myflags, > if (length) { > /* Unconditionally drop the connection if the client > * can't start a TLS negotiation correctly */ > - nbd_reject_length(client, length, option, true, errp); > - return -EINVAL; > + return nbd_reject_length(client, length, option, true, > + errp); rather than repeating an error message. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature