Re: [Libguestfs] [libnbd PATCH v3 05/22] states: Prepare to receive 64-bit replies

[Eric Blake]

On Thu, Jun 01, 2023 at 08:00:55AM -0500, Eric Blake wrote:
> > > @@ -83,13 +96,18 @@  REPLY.STRUCTURED_REPLY.CHECK:
> > >     * not worth keeping the connection alive.
> > >     */
> > >    if (length > MAX_REQUEST_SIZE + sizeof 
> > > h->sbuf.reply.payload.offset_data) {
> > > -    set_error (0, "invalid server reply length %" PRIu32, length);
> > > +    set_error (0, "invalid server reply length %" PRIu64, length);
> > >      SET_NEXT_STATE (%.DEAD);
> > >      return 0;
> > >    }
> > > +  /* For convenience, we now normalize extended replies into compact,
> > > +   * doable since we validated that payload length fits in 32 bits.
> > > +   */
> > > +  h->sbuf.reply.hdr.structured.length = length;
> > 
> > (8) I'm very confused by this. For an extended reply, this will
> > overwrite the "offset" field.
> 
> At one point, I considered normalizing in the opposite direction (take
> structured replies and widen them into the extended header form); it
> required touching more lines of code, so I didn't pursue it at the
> time.  But I can see how compressing things down (intentionally
> throwing away information we will no longer reference) can be
> confusing without good comments, so at a minimum, I will be adding
> comments, if not outright going with the widening rather than
> narrowing approach.

In fact, doing it in place is bad.  Later code in this function can
trigger a transition to state RESYNC, which assumes
sbuf.reply.hdr.structured.length still holds the wire value, not the
normalized value.  I'm fixing it up by creating h->payload_left,
initialized in START to the endian-corrected wire value, and then
decremented as various states peel off parts of the payload, so that
FINISH can then assert all payload has been accounted for.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org