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
_______________________________________________
ipxe-devel mailing list
ipxe-devel@lists.ipxe.org
https://lists.ipxe.org/mailman/listinfo.cgi/ipxe-devel
