Skip to content
  • Eric Blake's avatar
    091d0bf3
    nbd/client: Refactor nbd_receive_list() · 091d0bf3
    Eric Blake authored
    
    
    Right now, nbd_receive_list() is only called by
    nbd_receive_query_exports(), which in turn is only called if the
    server lacks NBD_OPT_GO but has working option negotiation, and is
    merely used as a quality-of-implementation trick since servers
    can't give decent errors for NBD_OPT_EXPORT_NAME.  However, servers
    that lack NBD_OPT_GO are becoming increasingly rare (nbdkit was a
    latecomer, in Aug 2018, but qemu has been such a server since commit
    f37708f6 in July 2017 and released in 2.10), so it no longer makes
    sense to micro-optimize that function for performance.
    
    Furthermore, when debugging a server's implementation, tracing the
    full reply (both names and descriptions) is useful, not to mention
    that upcoming patches adding 'qemu-nbd --list' will want to collect
    that data.  And when you consider that a server can send an export
    name up to the NBD protocol length limit of 4k; but our current
    NBD_MAX_NAME_SIZE is only 256, we can't trace all valid server
    names without more storage, but 4k is large enough that the heap
    is better than the stack for long names.
    
    Thus, I'm changing the division of labor, with nbd_receive_list()
    now always malloc'ing a result on success (the malloc is bounded
    by the fact that we reject servers with a reply length larger
    than 32M), and moving the comparison to 'wantname' to the caller.
    
    There is a minor change in behavior where a server with 0 exports
    (an immediate NBD_REP_ACK reply) is now no longer distinguished
    from a server without LIST support (NBD_REP_ERR_UNSUP); this
    information could be preserved with a complication to the calling
    contract to provide a bit more information, but I didn't see the
    point.  After all, the worst that can happen if our guess at a
    match is wrong is that the caller will get a cryptic disconnect
    when NBD_OPT_EXPORT_NAME fails (which is no different from what
    would happen if we had not tried LIST), while treating an empty
    list as immediate failure would prevent connecting to really old
    servers that really did lack LIST.  Besides, NBD servers with 0
    exports are rare (qemu can do it when using QMP nbd-server-start
    without nbd-server-add - but qemu understands NBD_OPT_GO and
    thus won't tickle this change in behavior).
    
    Fix the spelling of foundExport to match coding standards while
    in the area.
    
    Signed-off-by: default avatarEric Blake <eblake@redhat.com>
    Reviewed-by: default avatarRichard W.M. Jones <rjones@redhat.com>
    Reviewed-by: default avatarVladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
    Message-Id: <20190117193658.16413-9-eblake@redhat.com>
    091d0bf3
    nbd/client: Refactor nbd_receive_list()
    Eric Blake authored
    
    
    Right now, nbd_receive_list() is only called by
    nbd_receive_query_exports(), which in turn is only called if the
    server lacks NBD_OPT_GO but has working option negotiation, and is
    merely used as a quality-of-implementation trick since servers
    can't give decent errors for NBD_OPT_EXPORT_NAME.  However, servers
    that lack NBD_OPT_GO are becoming increasingly rare (nbdkit was a
    latecomer, in Aug 2018, but qemu has been such a server since commit
    f37708f6 in July 2017 and released in 2.10), so it no longer makes
    sense to micro-optimize that function for performance.
    
    Furthermore, when debugging a server's implementation, tracing the
    full reply (both names and descriptions) is useful, not to mention
    that upcoming patches adding 'qemu-nbd --list' will want to collect
    that data.  And when you consider that a server can send an export
    name up to the NBD protocol length limit of 4k; but our current
    NBD_MAX_NAME_SIZE is only 256, we can't trace all valid server
    names without more storage, but 4k is large enough that the heap
    is better than the stack for long names.
    
    Thus, I'm changing the division of labor, with nbd_receive_list()
    now always malloc'ing a result on success (the malloc is bounded
    by the fact that we reject servers with a reply length larger
    than 32M), and moving the comparison to 'wantname' to the caller.
    
    There is a minor change in behavior where a server with 0 exports
    (an immediate NBD_REP_ACK reply) is now no longer distinguished
    from a server without LIST support (NBD_REP_ERR_UNSUP); this
    information could be preserved with a complication to the calling
    contract to provide a bit more information, but I didn't see the
    point.  After all, the worst that can happen if our guess at a
    match is wrong is that the caller will get a cryptic disconnect
    when NBD_OPT_EXPORT_NAME fails (which is no different from what
    would happen if we had not tried LIST), while treating an empty
    list as immediate failure would prevent connecting to really old
    servers that really did lack LIST.  Besides, NBD servers with 0
    exports are rare (qemu can do it when using QMP nbd-server-start
    without nbd-server-add - but qemu understands NBD_OPT_GO and
    thus won't tickle this change in behavior).
    
    Fix the spelling of foundExport to match coding standards while
    in the area.
    
    Signed-off-by: default avatarEric Blake <eblake@redhat.com>
    Reviewed-by: default avatarRichard W.M. Jones <rjones@redhat.com>
    Reviewed-by: default avatarVladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
    Message-Id: <20190117193658.16413-9-eblake@redhat.com>
Loading