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 ...
    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 );
   ref_put ( &nbd->refcnt );
   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.

ipxe-devel mailing list

Reply via email to