Re: [PATCH v4 2/7] nbd/server: Plumb in new args to nbd_client_add()

[Eric Blake]

On Wed, Aug 07, 2024 at 06:58:36PM GMT, Daniel P. Berrangé wrote:
> On Wed, Aug 07, 2024 at 12:43:28PM -0500, Eric Blake wrote:
> > Upcoming patches to fix a CVE need to track an opaque pointer passed
> > in by the owner of a client object, as well as reequest for a time

s/reequest/request/

> > limit on how fast negotiation must complete.  Prepare for that by
> > changing the signature of nbd_client_new() and adding an accessor to
> > get at the opaque pointer, although for now the two servers
> > (qemu-nbd.c and blockdev-nbd.c) do not change behavior.
> > 
> > Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> > Signed-off-by: Eric Blake <eblake@redhat.com>
> > ---
> >  include/block/nbd.h | 11 ++++++++++-
> >  blockdev-nbd.c      |  6 ++++--
> >  nbd/server.c        | 20 +++++++++++++++++---
> >  qemu-nbd.c          |  4 +++-
> >  4 files changed, 34 insertions(+), 7 deletions(-)
> > 
> > diff --git a/include/block/nbd.h b/include/block/nbd.h
> > index 4e7bd6342f9..5fe14786414 100644
> > --- a/include/block/nbd.h
> > +++ b/include/block/nbd.h
> > @@ -33,6 +33,12 @@ typedef struct NBDMetaContexts NBDMetaContexts;
> > 
> >  extern const BlockExportDriver blk_exp_nbd;
> > 
> > +/*
> > + * NBD_DEFAULT_HANDSHAKE_LIMIT: Number of seconds in which client must
> > + * succeed at NBD_OPT_GO before being forcefully dropped as too slow.
> > + */
> > +#define NBD_DEFAULT_HANDSHAKE_LIMIT 10
> 
> Suggest
> 
> s/NBD_DEFAULT_HANDSHAKE_LIMIT/NBD_DEFAULT_HANDSHAKE_MAX_SECS/

I like it.

> 
> 
> > +
> >  /* Handshake phase structs - this struct is passed on the wire */
> > 
> >  typedef struct NBDOption {
> > @@ -403,9 +409,12 @@ AioContext *nbd_export_aio_context(NBDExport *exp);
> >  NBDExport *nbd_export_find(const char *name);
> > 
> >  void nbd_client_new(QIOChannelSocket *sioc,
> > +                    uint32_t handshake_limit,
> 
> s/handshake_limit/handshake_max_secs/
> 
> to make the units of the parameter self-documenting.
> 
> Since this is a non-functional suggestion
> 
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

Will adjust the series with the fallout.

At this point, I'm leaning towards queuing 1-5 in an upcoming pull
request, but leaving 6-7 for the 9.2 development cycle.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org