Re: [PATCH v6 04/17] nbd: Prepare for 64-bit request effect lengths

[Vladimir Sementsov-Ogievskiy]

On 05.09.23 17:24, Eric Blake wrote:
> On Mon, Sep 04, 2023 at 07:15:04PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > On 29.08.23 20:58, Eric Blake wrote:
> > > Widen the length field of NBDRequest to 64-bits, although we can
> > > assert that all current uses are still under 32 bits: either because
> > > of NBD_MAX_BUFFER_SIZE which is even smaller (and where size_t can
> > > still be appropriate, even on 32-bit platforms), or because nothing
> > > ever puts us into NBD_MODE_EXTENDED yet (and while future patches will
> > > allow larger transactions, the lengths in play here are still capped
> > > at 32-bit).  There are no semantic changes, other than a typo fix in a
> > > couple of error messages.
> > >
> > > Signed-off-by: Eric Blake <eblake@redhat.com>
> > > ---
> > >
> > > v6: fix sign extension bug
> > >
> > > v5: tweak commit message, adjust a few more spots [Vladimir]
> > >
> > > v4: split off enum changes to earlier patches [Vladimir]
> >
> > [..]
> >
> > > --- a/nbd/server.c
> > > +++ b/nbd/server.c
> > > @@ -1165,7 +1165,7 @@ static int nbd_negotiate_options(NBDClient *client, Error 
> > > **errp)
> > >            client->optlen = length;
> > >
> > >            if (length > NBD_MAX_BUFFER_SIZE) {
> > > -            error_setg(errp, "len (%" PRIu32" ) is larger than max len (%u)",
> > > +            error_setg(errp, "len (%" PRIu32 ") is larger than max len (%u)",
> > >                           length, NBD_MAX_BUFFER_SIZE);
> > >                return -EINVAL;
> > >            }
> > > @@ -1441,7 +1441,7 @@ static int coroutine_fn nbd_receive_request(NBDClient 
> > > *client, NBDRequest *reque
> > >        request->type   = lduw_be_p(buf + 6);
> > >        request->cookie = ldq_be_p(buf + 8);
> > >        request->from   = ldq_be_p(buf + 16);
> > > -    request->len    = ldl_be_p(buf + 24);
> > > +    request->len    = (uint32_t)ldl_be_p(buf + 24); /* widen 32 to 64 bits */
> >
> > should it be "(uint64_t)" ?
>
> No. ldl_be_p() returns an int.  Widening int to 64 bits sign-extends;
> we have to first make it unsigned (by casting to uint32_t) so that we
> then have an unsigned value that widens by zero-extension.
>
> Maybe we should fix ldl_bd_p() and friends to favor unsigned values.
> 'git grep ldul_be' has zero hits; even though Peter just touched the
> docs patch claiming it exists:
>
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg00645.html
>
>
> > >        trace_nbd_receive_request(magic, request->flags, request->type,
> > >                                  request->from, request->len);
> > > @@ -1899,7 +1899,7 @@ static int coroutine_fn 
> > > nbd_co_send_simple_reply(NBDClient *client,
> > >                                                     NBDRequest *request,
> > >                                                     uint32_t error,
> > >                                                     void *data,
> > > -                                                 size_t len,
> > > +                                                 uint64_t len,
> > >                                                     Error **errp)
> > >    {
> > >        NBDSimpleReply reply;
> > > @@ -1910,6 +1910,7 @@ static int coroutine_fn 
> > > nbd_co_send_simple_reply(NBDClient *client,
> > >        };
> > >
> > >        assert(!len || !nbd_err);
> > > +    assert(len <= NBD_MAX_STRING_SIZE);
> >
> > NBD_MAX_BUFFER_SIZE ?
>
> No. MAX_STRING_SIZE is 4k, MAX_BUFFER_SIZE is 32M.  The smaller size
> is the correct bound here (an error message has to be transmitted as a
> single string, and the recipient does not expect more than a 4k error
> message).

I miss something.. Why it's an error message? It's may be a simple reply for 
CMD_READ, where len is request->len


--
Best regards,
Vladimir