[ipxe-devel] sanboot nbd

[Michael Brown]

On 13/10/16 10:56, James Harper wrote:
> I originally implemented this in my own custom protocol but nbd supports
> the “sparse” IO calls I need, and a standard nbd implementation might be
> more useful to others, maybe?
>
> So firstly, is the attached code on the right track? It’s fairly
> minimal, and needs a lot more error checking, but is it architecturally
> sound? Am I doing the right things with interfaces?
>
> Secondly, is anyone actually interested in an nbd implementation for
> iPXE? It would boot into DOS or another environment that uses int13h,
> but obviously would require drivers for any other OS. Linux can already
> net boot into an nbd root volume without needing nbd support from iPXE,
> and many of the other interesting use cases for nbd are already possible
> via iSCSI.

NBD is definitely much more interesting than a custom protocol.

The overall architecture looks plausible.  As you say, you need to 
include the error handling.  The general model within iPXE is to use 
structured goto:

     if ( ( rc = try_something ( ... ) ) != 0 ) {
        DBGC ( nbd, "NBD %s something failed: %s\n", nbd->name, strerror 
( rc ) );
        goto err_something;
     }
     ... success path ...
     return 0;

     ... failure path ...
     undo_something(...);
   err_something:
     ...
     return rc;

Each possible failure point (e.g. the call to "try_something()") has a 
corresponding label (e.g. "err_something:").  The code to cleanly undo a 
_successful_ operation appears immediately _before_ the label.

As a more concrete example:

    nbd = zalloc ( sizeof ( *nbd ) );
    if ( ! nbd ) {
      rc = -ENOMEM;
      goto err_alloc;
    }
    ref_init ( &nbd->refcnt, NULL );
    ...
    if ( ( rc = nbd_connect ( nbd ) ) != 0 )
       goto err_connect;
    ...
    intf_plug_plug ( &nbd->block, parent );
    ref_put ( &nbd->refcnt );
    return 0;

    nbd_shutdown ( nbd, rc );
  err_connect:
    ref_put ( &nbd->refcnt );
  err_alloc;
    return rc;

Some other points:

- Use the DBG() and DBGC() macros rather than dbg_printf().  This allows 
you to control the debug level via the DEBUG= option on the build 
command line, and ensures that the debug statements are optimised away 
in non-debug builds.

- Get rid of the get_uint16() etc macros and use structs instead.  For 
example:

     struct nbd_option_export_name {
       uint64_t byte_count;
       uint16_t flags;
     } __attribute__ (( packed ));

     struct nbd_option_export_name *opt = ...;
     rx_required = sizeof ( *opt );
     DBGC ( nbd, "NBD flags %04x\n", nbd->name, ntohs ( opt->flags ) );

- You don't need to use copy_from_user() in nbd_dev_tx_resume(); you can 
just use memcpy() directly.  Better still would be to perform only a 
single allocation via xfer_alloc_iob() and construct the TX records 
directly in iobuf->data.

Michael