On 11/22/2017 04:19 AM, Vladimir Sementsov-Ogievskiy wrote: > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
The subject line says what and sort of implies why, but the commit message body is rather sparse. > --- > nbd/server.c | 34 ++++++++++++++++++++++------------ > 1 file changed, 22 insertions(+), 12 deletions(-) Not a net win in lines of code, so a bit more justification may be helpful. > > diff --git a/nbd/server.c b/nbd/server.c > index bccc0274e7..c9445a89e9 100644 > --- a/nbd/server.c > +++ b/nbd/server.c > @@ -139,6 +139,19 @@ static void nbd_client_receive_next_request(NBDClient > *client); > > */ > > +static inline int nbd_opt_read(NBDClient *client, void *buffer, size_t size, > + Error **errp) > +{ > + client->optlen -= size; > + return qio_channel_read_all(client->ioc, buffer, size, errp) < 0 ? -EIO > : 0; Should this code check that client->optlen >= size, and issue the appropriate error message if not? That may centralize some of the error checking repeated elsewhere. > +} > + > +static inline int nbd_opt_drop(NBDClient *client, size_t size, Error **errp) > +{ > + client->optlen -= size; > + return nbd_drop(client->ioc, size, errp); Same question on size validation. > +} > + > /* Send a reply header, including length, but no payload. > * Return -errno on error, 0 on success. */ > static int nbd_negotiate_send_rep_len(NBDClient *client, uint32_t type, > @@ -299,7 +312,7 @@ static int nbd_negotiate_handle_export_name(NBDClient > *client, > error_setg(errp, "Bad length received"); > return -EINVAL; > } > - if (nbd_read(client->ioc, name, client->optlen, errp) < 0) { > + if (nbd_opt_read(client, name, client->optlen, errp) < 0) { > error_prepend(errp, "read failed: "); > return -EINVAL; Here's an example caller that had a previous size validation. > } > @@ -383,40 +396,36 @@ static int nbd_negotiate_handle_info(NBDClient *client, > uint16_t myflags, > msg = "overall request too short"; > goto invalid; > } > - if (nbd_read(client->ioc, &namelen, sizeof(namelen), errp) < 0) { > + if (nbd_opt_read(client, &namelen, sizeof(namelen), errp) < 0) { > return -EIO; And again, a size validation right before the call. > } > be32_to_cpus(&namelen); > - client->optlen -= sizeof(namelen); Okay, part of the refactoring means you don't have to remember to track remaining size separately from reading; that's a nice feature. I suspect it is also possible to assert that client->optlen is zero before reading the next opt (I'm reviewing the patch in order, we'll see if I come back here). [1] > if (namelen > client->optlen - sizeof(requests) || > (client->optlen - namelen) % 2) > { > msg = "name length is incorrect"; > goto invalid; > } > - if (nbd_read(client->ioc, name, namelen, errp) < 0) { > + if (nbd_opt_read(client, name, namelen, errp) < 0) { > return -EIO; > } Another size check prior to the call (actually checking multiple conditions)... > name[namelen] = '\0'; > - client->optlen -= namelen; > trace_nbd_negotiate_handle_export_name_request(name); > > - if (nbd_read(client->ioc, &requests, sizeof(requests), errp) < 0) { > + if (nbd_opt_read(client, &requests, sizeof(requests), errp) < 0) { > return -EIO; ...and no direct size check before this call, because the earlier size checks already covered it. Arguably, the in-place size check error messages may be slightly more precise, but any message at all about unexpected sizing is appropriate (given that only broken clients should be sending incorrect sizes) - so I'm still leaning towards a stronger refactoring that puts the size check in the helper function. > @@ -521,7 +530,7 @@ static int nbd_negotiate_handle_info(NBDClient *client, > uint16_t myflags, > return rc; > > invalid: > - if (nbd_drop(client->ioc, client->optlen, errp) < 0) { > + if (nbd_opt_drop(client, client->optlen, errp) < 0) { > return -EIO; > } > return nbd_negotiate_send_rep_err(client, NBD_REP_ERR_INVALID, > @@ -715,7 +724,7 @@ static int nbd_negotiate_options(NBDClient *client, > uint16_t myflags, > return -EINVAL; > > default: > - if (nbd_drop(client->ioc, length, errp) < 0) { > + if (nbd_opt_drop(client, length, errp) < 0) { Isn't length == client->optlen here? If so, can we omit the length parameter to nbd_opt_drop(), and instead have it defined as dropping to the end of the current option? > @@ -821,6 +830,7 @@ static int nbd_negotiate_options(NBDClient *client, > uint16_t myflags, > if (ret < 0) { > return ret; > } > + assert(client->optlen == 0); [1] Ah, you did add the assertion. I'm going to propose a variant on this patch, to cover the points I made above about ease-of-use (and to hopefully show a net win in lines of code). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature