Make use of the new nbd_get_export_description to list a description when connecting to a single export. At present, only qemu-nbd offers the needed information; it would require a patch to nbdkit to do likewise (although such a patch is not difficult, given that we recently added .list_exports). --- generator/API.ml | 3 +++ examples/server-flags.c | 23 +++++++++++++++++ info/Makefile.am | 1 + info/info-description.sh | 56 ++++++++++++++++++++++++++++++++++++++++ info/nbdinfo.c | 16 +++++++++--- 5 files changed, 95 insertions(+), 4 deletions(-) create mode 100755 info/info-description.sh diff --git a/generator/API.ml b/generator/API.ml index eff0ac4..0c8ef30 100644 --- a/generator/API.ml +++ b/generator/API.ml @@ -322,6 +322,7 @@ Note that even when full info is requested, the server is not obligated to reply with all information that libnbd requested. Similarly, libnbd will ignore any optional server information that libnbd has not yet been taught to recognize."; + example = Some "examples/server-flags.c"; see_also = [Link "get_full_info"; Link "get_canonical_export_name"; Link "get_export_description"]; }; @@ -352,6 +353,7 @@ C<\"\">). Some servers are unlikely to report a canonical name unless the client specifically hinted about wanting it, via L<nbd_set_full_info(3)>."; + example = Some "examples/server-flags.c"; see_also = [Link "set_full_info"; Link "get_export_name"]; }; @@ -368,6 +370,7 @@ the server. Some servers are unlikely to report description unless the client specifically hinted about wanting it, via L<nbd_set_full_info(3)>. For L<qemu-nbd(8)>, a description is set with I<-D>."; + example = Some "examples/server-flags.c"; see_also = [Link "set_full_info"]; }; diff --git a/examples/server-flags.c b/examples/server-flags.c index d06f02f..d156ace 100644 --- a/examples/server-flags.c +++ b/examples/server-flags.c @@ -16,6 +16,7 @@ int main (int argc, char *argv[]) { struct nbd_handle *nbd; + char *str; int flag; if (argc != 2) { @@ -30,6 +31,14 @@ main (int argc, char *argv[]) exit (EXIT_FAILURE); } + /* Request full information. */ +#if LIBNBD_HAVE_NBD_SET_FULL_INFO /* Added in 1.4 */ + if (nbd_set_full_info (nbd, true) == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } +#endif + /* Connect to the NBD server over a * Unix domain socket. */ @@ -38,6 +47,20 @@ main (int argc, char *argv[]) exit (EXIT_FAILURE); } + /* See if the server provided extra details, + * using functions added in 1.4 + */ +#if LIBNBD_HAVE_NBD_GET_EXPORT_DESCRIPTION + str = nbd_get_canonical_export_name (nbd); + if (str) + printf ("canonical_name = %s\n", str); + free (str); + str = nbd_get_export_description (nbd); + if (str) + printf ("description = %s\n", str); + free (str); +#endif + /* Read and print the flags. */ #define PRINT_FLAG(flag_fn) \ flag = flag_fn (nbd); \ diff --git a/info/Makefile.am b/info/Makefile.am index 22c0c28..05b8137 100644 --- a/info/Makefile.am +++ b/info/Makefile.am @@ -66,6 +66,7 @@ TESTS += \ info-oldstyle.sh \ info-size.sh \ info-text.sh \ + info-description.sh \ $(NULL) check-valgrind: diff --git a/info/info-description.sh b/info/info-description.sh new file mode 100755 index 0000000..d367a25 --- /dev/null +++ b/info/info-description.sh @@ -0,0 +1,56 @@ +#!/usr/bin/env bash +# nbd client library in userspace +# Copyright (C) 2020 Red Hat Inc. +# +# This library is free software; you can redistribute it and/or +# modify it under the terms of the GNU Lesser General Public +# License as published by the Free Software Foundation; either +# version 2 of the License, or (at your option) any later version. +# +# This library is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +# Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser General Public +# License along with this library; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + +. ../tests/functions.sh + +set -e +set -x + +# XXX Use nbdkit once it supports NBD_INFO_DESCRIPTION +requires qemu-nbd --version +requires truncate --version + +img=info-description.img +out=info-description.out +pid=info-description.pid +sock=`mktemp -u` +cleanup_fn rm -f $img $out $pid $sock +rm -f $img $out $pid $sock + +truncate -s 1M $img +qemu-nbd -f raw -t --socket=$sock --pid-file=$pid -x "hello" -D "world" $img & +cleanup_fn kill $! + +# Wait for qemu-nbd to start up. +for i in {1..60}; do + if test -f $pid; then + break + fi + sleep 1 +done +if ! test -f $pid; then + echo "$0: qemu-nbd did not start up" + exit 1 +fi + +$VG nbdinfo "nbd+unix:///hello?socket=$sock" > $out +cat $out + +grep 'export="hello":' $out +grep 'description: world' $out +grep 'export-size: 1048576' $out diff --git a/info/nbdinfo.c b/info/nbdinfo.c index 454e5c5..b54dfd4 100644 --- a/info/nbdinfo.c +++ b/info/nbdinfo.c @@ -99,6 +99,7 @@ main (int argc, char *argv[]) int64_t size; const char *protocol; int tls_negotiated; + char *desc; for (;;) { c = getopt_long (argc, argv, short_options, long_options, NULL); @@ -191,6 +192,8 @@ main (int argc, char *argv[]) */ if (list_all) nbd_set_list_exports (nbd, true); + else if (!size_only) + nbd_set_full_info (nbd, true); if (nbd_connect_uri (nbd, argv[optind]) == -1) { if (!list_all || nbd_get_errno () == ENOTSUP) { @@ -244,9 +247,11 @@ main (int argc, char *argv[]) printf ("\"TLS\": %s,\n", tls_negotiated ? "true" : "false"); } - if (!list_all) - /* XXX We would need libnbd to request NBD_INFO_DESCRIPTION */ - list_one_export (nbd, NULL, true, true); + if (!list_all) { + desc = nbd_get_export_description (nbd); + list_one_export (nbd, desc, true, true); + free (desc); + } else list_all_exports (nbd, argv[optind]); @@ -277,7 +282,10 @@ list_one_export (struct nbd_handle *nbd, const char *desc, exit (EXIT_FAILURE); } - export_name = nbd_get_export_name (nbd); + /* Prefer the server's version of the name, if available */ + export_name = nbd_get_canonical_export_name (nbd); + if (export_name == NULL) + export_name = nbd_get_export_name (nbd); if (export_name == NULL) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); -- 2.28.0

In particular, any query that can be determined after getting gflags from the server or after a response to NBD_OPT* can be now done earlier than Connected, and any configuration that affects behavior of NBD_OPT_* can be set later than rather just during Created. In order to support this change: nbd_get_protocol also needs to hoist the setting of h->protocol earlier in the state machine. Also, whereas nbd_connect_uri used to unconditionally take precedence over nbd_set_export_name, now nbd_set_export_name can be used during negotiation to override the export name portion of a URI. Of course, until a later patch actually lets us enter the NEGOTIATED state, you can't test the differences yet. --- generator/API.ml | 33 +++++++++++++++++++-------------- generator/states-newstyle.c | 12 ++++++------ 2 files changed, 25 insertions(+), 20 deletions(-) diff --git a/generator/API.ml b/generator/API.ml index 0990d48..a363117 100644 --- a/generator/API.ml +++ b/generator/API.ml @@ -272,7 +272,7 @@ C<\"nbd2\">, etc."; "set_export_name", { default_call with args = [ String "export_name" ]; ret = RErr; - permitted_states = [ Created ]; + permitted_states = [ Created; Negotiating ]; shortdesc = "set the export name"; longdesc = "\ For servers which require an export name or can serve different @@ -287,7 +287,7 @@ The encoding of export names is always UTF-8. This call may be skipped if using L<nbd_connect_uri(3)> to connect to a URI that includes an export name."; - see_also = [Link "get_export_name"; Link "connect_uri"]; + see_also = [Link "get_export_name"; Link "connect_uri"]; }; "get_export_name", { @@ -307,7 +307,7 @@ of an empty string C<\"\">)"; "set_full_info", { default_call with args = [Bool "request"]; ret = RErr; - permitted_states = [ Created ]; + permitted_states = [ Created; Negotiating ]; shortdesc = "control whether NBD_OPT_GO requests extra details"; longdesc = "\ By default, when connecting to an export, libnbd only requests the @@ -441,7 +441,7 @@ on a particular connection use L<nbd_get_tls_negotiated(3)> instead."; "get_tls_negotiated", { default_call with args = []; ret = RBool; - permitted_states = [ Connected; Closed ]; + permitted_states = [ Negotiating; Connected; Closed ]; shortdesc = "find out if TLS was negotiated on a connection"; longdesc = "\ After connecting you may call this to find out if the @@ -610,7 +610,7 @@ L<nbd_get_structured_replies_negotiated(3)> instead."; "get_structured_replies_negotiated", { default_call with args = []; ret = RBool; - permitted_states = [ Connected; Closed ]; + permitted_states = [ Negotiating; Connected; Closed ]; shortdesc = "see if structured replies are in use"; longdesc = "\ After connecting you may call this to find out if the connection is @@ -735,7 +735,7 @@ Return true if list exports mode was enabled on this handle."; "get_nr_list_exports", { default_call with args = []; ret = RInt; - permitted_states = [ Connected; Closed; Dead ]; + permitted_states = [ Negotiating; Connected; Closed; Dead ]; shortdesc = "return the number of exports returned by the server"; longdesc = "\ If list exports mode was enabled on the handle and you connected @@ -749,7 +749,7 @@ L<nbd_set_list_exports(3)>."; "get_list_export_name", { default_call with args = [ Int "i" ]; ret = RString; - permitted_states = [ Connected; Closed; Dead ]; + permitted_states = [ Negotiating; Connected; Closed; Dead ]; shortdesc = "return the i'th export name"; longdesc = "\ If list exports mode was enabled on the handle and you connected @@ -761,7 +761,7 @@ from the list returned by the server."; "get_list_export_description", { default_call with args = [ Int "i" ]; ret = RString; - permitted_states = [ Connected; Closed; Dead ]; + permitted_states = [ Negotiating; Connected; Closed; Dead ]; shortdesc = "return the i'th export description"; longdesc = "\ If list exports mode was enabled on the handle and you connected @@ -776,7 +776,7 @@ is set with I<-D>."; "add_meta_context", { default_call with args = [ String "name" ]; ret = RErr; - permitted_states = [ Created ]; + permitted_states = [ Created; Negotiating ]; shortdesc = "ask server to negotiate metadata context"; longdesc = "\ During connection libnbd can negotiate zero or more metadata @@ -885,10 +885,14 @@ parameter in NBD URIs is allowed."; shortdesc = "connect to NBD URI"; longdesc = "\ Connect (synchronously) to an NBD server and export by specifying -the NBD URI. This call parses the URI and may call +the NBD URI. This call parses the URI and calls L<nbd_set_export_name(3)> and L<nbd_set_tls(3)> and other -calls as needed, followed by -L<nbd_connect_tcp(3)> or L<nbd_connect_unix(3)>. +calls as needed, followed by L<nbd_connect_tcp(3)> or +L<nbd_connect_unix(3)>. However, it is possible to override the +export name portion of a URI by using C<nbd_set_opt_mode> to +enable option mode, then using L<nbd_set_export_name(3)> as part +of later negotiation. + This call returns when the connection has been made. =head2 Example URIs supported @@ -1027,7 +1031,8 @@ test whether this is the case with L<nbd_supports_uri(3)>. Support for URIs that require TLS will fail if libnbd was not compiled with gnutls; you can test whether this is the case with L<nbd_supports_tls(3)>."; - see_also = [URLLink "https://github.com/NetworkBlockDevice/nbd/blob/master/doc/uri.md"]; + see_also = [URLLink "https://github.com/NetworkBlockDevice/nbd/blob/master/doc/uri.md"; + Link "set_export_name"; Link "set_tls"]; }; "connect_unix", { @@ -1326,7 +1331,7 @@ are free to pass in other contexts." "get_protocol", { default_call with args = []; ret = RStaticString; - permitted_states = [ Connected; Closed ]; + permitted_states = [ Negotiating; Connected; Closed ]; shortdesc = "return the NBD protocol variant"; longdesc = "\ Return the NBD protocol variant in use on the connection. At diff --git a/generator/states-newstyle.c b/generator/states-newstyle.c index 573f724..8da4617 100644 --- a/generator/states-newstyle.c +++ b/generator/states-newstyle.c @@ -1,5 +1,5 @@ /* nbd client library in userspace: state machine - * Copyright (C) 2013-2019 Red Hat Inc. + * Copyright (C) 2013-2020 Red Hat Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -136,6 +136,11 @@ STATE_MACHINE { return 0; } + if ((h->gflags & LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE) == 0) + h->protocol = "newstyle"; + else + h->protocol = "newstyle-fixed"; + cflags = h->gflags; h->sbuf.cflags = htobe32 (cflags); h->wbuf = &h->sbuf; @@ -156,11 +161,6 @@ STATE_MACHINE { return 0; NEWSTYLE.FINISHED: - if ((h->gflags & LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE) == 0) - h->protocol = "newstyle"; - else - h->protocol = "newstyle-fixed"; - SET_NEXT_STATE (%.READY); return 0; -- 2.28.0

It is finally time to introduce our first negotiating option command. With this change, we can now enter NEWSTYLE.START more than once; as such, it needs to know whether it is the first entry (proceed with gflags/cflags, TLS, and structured reply) or a later entry (all nbd_opt_* will cause an IssueCommand event to kick the state machine out of NEGOTIATING, at which point we want to jump to the sub-state appropriate for that option). This is done by setting h->current_opt prior to entering NEWSTYLE. We also need a couple of new states to implement NBD_OPT_ABORT; this was not deemed worth a separate sub-group and new states-newstyle-opt-abort.c file, so I just inlined it in NEWSTYLE instead. One nice benefit of these new states: when NBD_OPT_GO fails and we are not in opt mode, or when we require TLS but the server does not support it, we can now hang up gracefully instead of abruptly disconnecting from a newstyle-fixed server. This is observable when using qemu-nbd, in that we now silence the former noise from qemu: $ truncate --size=512 /tmp/f $ ./run nbdsh -c ' import errno try: h.connect_systemd_socket_activation(["qemu-nbd", "-f", "raw", "-x", "b", "/tmp/f"]) except nbd.Error as e: assert e.errnum == errno.ENOENT ' qemu-nbd: option negotiation failed: Failed to read opts magic: Unexpected end-of-file before all bytes were read The aio_opt_abort counterpart does not need a completion closure, for the same reason that aio_disconnect does not have one - the whole point of this command is a clean shutdown, so you aren't going to rely on a server reply, and there are no parameters that need cleanup. Yes, it's a bit awkward that with just this patch, enabling opt_mode means you HAVE to kill the connection without a data phase; that will be rectified in the next patch adding nbd_opt_go. But breaking the patches up makes review easier. --- lib/internal.h | 1 + generator/API.ml | 35 ++++++- generator/state_machine.ml | 26 +++++ generator/states-magic.c | 6 +- generator/states-newstyle-opt-go.c | 2 +- generator/states-newstyle-opt-starttls.c | 4 +- .../states-newstyle-opt-structured-reply.c | 7 +- generator/states-newstyle.c | 59 +++++++++++- lib/opt.c | 34 +++++++ tests/Makefile.am | 7 ++ tests/errors.c | 11 ++- tests/newstyle-limited.c | 26 +++++ tests/opt-abort.c | 95 +++++++++++++++++++ .gitignore | 1 + 14 files changed, 302 insertions(+), 12 deletions(-) create mode 100644 tests/opt-abort.c diff --git a/lib/internal.h b/lib/internal.h index 5f495fb..03baacd 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -101,6 +101,7 @@ struct nbd_handle { /* Option negotiation mode. */ bool opt_mode; + uint8_t current_opt; /* List exports mode. */ bool list_exports; diff --git a/generator/API.ml b/generator/API.ml index 3dd94f6..cbc1c33 100644 --- a/generator/API.ml +++ b/generator/API.ml @@ -699,10 +699,11 @@ negotiating everything on your behalf using settings made before starting the connection. To leave the mode and proceed on to the ready state, you must use nbd_opt_go successfully; a failed C<nbd_opt_go> returns to the negotiating state to allow a change of -export name before trying again. You may also use nbd_opt_abort +export name before trying again. You may also use L<nbd_opt_abort(3)> to end the connection without finishing negotiation."; example = Some "examples/list-exports.c"; - see_also = [Link "get_opt_mode"; Link "aio_is_negotiating"]; + see_also = [Link "get_opt_mode"; Link "aio_is_negotiating"; + Link "opt_abort"]; }; "get_opt_mode", { @@ -715,6 +716,19 @@ Return true if option negotiation mode was enabled on this handle."; see_also = [Link "set_opt_mode"]; }; + "opt_abort", { + default_call with + args = []; ret = RErr; + permitted_states = [ Negotiating ]; + shortdesc = "end negotiation and close the connection"; + longdesc = "\ +Request that the server finish negotiation, gracefully if possible, then +close the connection. This can only be used if L<nbd_set_opt_mode(3)> +enabled option mode."; + example = Some "examples/list-exports.c"; + see_also = [Link "set_opt_mode"; Link "aio_opt_abort"]; + }; + "set_list_exports", { default_call with args = [Bool "list"]; ret = RErr; @@ -1883,6 +1897,21 @@ and completed the NBD handshake by calling L<nbd_aio_is_ready(3)>, on the connection."; }; + "aio_opt_abort", { + default_call with + args = []; ret = RErr; + permitted_states = [ Negotiating ]; + shortdesc = "end negotiation and close the connection"; + longdesc = "\ +Request that the server finish negotiation, gracefully if possible, then +close the connection. This can only be used if L<nbd_set_opt_mode(3)> +enabled option mode. + +To determine when the request completes, wait for +L<nbd_aio_is_connecting(3)> to return false."; + see_also = [Link "set_opt_mode"; Link "opt_abort"]; + }; + "aio_pread", { default_call with args = [ BytesPersistOut ("buf", "count"); UInt64 "offset" ]; @@ -2504,6 +2533,8 @@ let first_version = [ "set_opt_mode", (1, 4); "get_opt_mode", (1, 4); "aio_is_negotiating", (1, 4); + "opt_abort", (1, 4); + "aio_opt_abort", (1, 4); (* These calls are proposed for a future version of libnbd, but * have not been added to any released version so far. diff --git a/generator/state_machine.ml b/generator/state_machine.ml index 8525520..2d28c2e 100644 --- a/generator/state_machine.ml +++ b/generator/state_machine.ml @@ -278,6 +278,8 @@ and newstyle_state_machine = [ (* Options. These state groups are always entered unconditionally, * in this order. The START state in each group will check if the * state needs to run and skip to the next state in the list if not. + * When opt_mode is set, control is returned to the user in state + * NEGOTIATING after OPT_STRUCTURED_REPLY or any failed OPT_GO. *) Group ("OPT_STARTTLS", newstyle_opt_starttls_state_machine); Group ("OPT_LIST", newstyle_opt_list_state_machine); @@ -286,6 +288,30 @@ and newstyle_state_machine = [ Group ("OPT_GO", newstyle_opt_go_state_machine); Group ("OPT_EXPORT_NAME", newstyle_opt_export_name_state_machine); + (* When NBD_OPT_GO fails, or when opt_mode is enabled, option parsing + * can be cleanly ended without moving through the %READY state. + *) + State { + default_state with + name = "PREPARE_OPT_ABORT"; + comment = "Prepare to send NBD_OPT_ABORT"; + external_events = []; + }; + + State { + default_state with + name = "SEND_OPT_ABORT"; + comment = "Send NBD_OPT_ABORT to end negotiation"; + external_events = [ NotifyWrite, "" ]; + }; + + State { + default_state with + name = "SEND_OPTION_SHUTDOWN"; + comment = "Sending write shutdown notification to the remote server"; + external_events = [ NotifyWrite, "" ]; + }; + (* When option parsing has successfully finished negotiation * it will jump to this state for final steps before moving to * the %READY state. diff --git a/generator/states-magic.c b/generator/states-magic.c index 944728d..644bf73 100644 --- a/generator/states-magic.c +++ b/generator/states-magic.c @@ -1,5 +1,5 @@ /* nbd client library in userspace: state machine - * Copyright (C) 2013-2019 Red Hat Inc. + * Copyright (C) 2013-2020 Red Hat Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -42,8 +42,10 @@ STATE_MACHINE { } version = be64toh (h->sbuf.new_handshake.version); - if (version == NBD_NEW_VERSION) + if (version == NBD_NEW_VERSION) { + assert (h->current_opt == 0); SET_NEXT_STATE (%.NEWSTYLE.START); + } else if (version == NBD_OLD_VERSION) SET_NEXT_STATE (%.OLDSTYLE.START); else { diff --git a/generator/states-newstyle-opt-go.c b/generator/states-newstyle-opt-go.c index d696cae..1a59ae7 100644 --- a/generator/states-newstyle-opt-go.c +++ b/generator/states-newstyle-opt-go.c @@ -240,7 +240,7 @@ STATE_MACHINE { set_error (0, "handshake: unknown reply from NBD_OPT_GO: 0x%" PRIx32, reply); } - SET_NEXT_STATE (%.DEAD); + SET_NEXT_STATE (%^PREPARE_OPT_ABORT); break; case NBD_REP_ACK: SET_NEXT_STATE (%^FINISHED); diff --git a/generator/states-newstyle-opt-starttls.c b/generator/states-newstyle-opt-starttls.c index 7162c7a..d107b74 100644 --- a/generator/states-newstyle-opt-starttls.c +++ b/generator/states-newstyle-opt-starttls.c @@ -1,5 +1,5 @@ /* nbd client library in userspace: state machine - * Copyright (C) 2013-2019 Red Hat Inc. + * Copyright (C) 2013-2020 Red Hat Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -93,7 +93,7 @@ STATE_MACHINE { * then we can continue unencrypted. */ if (h->tls == LIBNBD_TLS_REQUIRE) { - SET_NEXT_STATE (%.DEAD); + SET_NEXT_STATE (%^PREPARE_OPT_ABORT); set_error (ENOTSUP, "handshake: server refused TLS, " "but handle TLS setting is 'require' (2)"); return 0; diff --git a/generator/states-newstyle-opt-structured-reply.c b/generator/states-newstyle-opt-structured-reply.c index 58a76db..71a4952 100644 --- a/generator/states-newstyle-opt-structured-reply.c +++ b/generator/states-newstyle-opt-structured-reply.c @@ -1,5 +1,5 @@ /* nbd client library in userspace: state machine - * Copyright (C) 2013-2019 Red Hat Inc. + * Copyright (C) 2013-2020 Red Hat Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -84,7 +84,10 @@ STATE_MACHINE { } /* Next option. */ - SET_NEXT_STATE (%^OPT_SET_META_CONTEXT.START); + if (h->opt_mode) + SET_NEXT_STATE (%.NEGOTIATING); + else + SET_NEXT_STATE (%^OPT_SET_META_CONTEXT.START); return 0; } /* END STATE MACHINE */ diff --git a/generator/states-newstyle.c b/generator/states-newstyle.c index 8da4617..6613f9f 100644 --- a/generator/states-newstyle.c +++ b/generator/states-newstyle.c @@ -112,6 +112,26 @@ handle_reply_error (struct nbd_handle *h) STATE_MACHINE { NEWSTYLE.START: + if (h->opt_mode) { + /* NEWSTYLE can be entered multiple times, from MAGIC.CHECK_MAGIC and + * during various nbd_opt_* calls during NEGOTIATION. Each previous + * state has informed us what we still need to do. + */ + switch (h->current_opt) { + case NBD_OPT_ABORT: + if ((h->gflags & LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE) == 0) { + SET_NEXT_STATE (%.DEAD); + set_error (ENOTSUP, "handshake: server is not using fixed newstyle"); + return 0; + } + SET_NEXT_STATE (%PREPARE_OPT_ABORT); + return 0; + case 0: + break; + default: + abort (); + } + } h->rbuf = &h->sbuf; h->rlen = sizeof h->sbuf.gflags; SET_NEXT_STATE (%RECV_GFLAGS); @@ -153,13 +173,48 @@ STATE_MACHINE { case -1: SET_NEXT_STATE (%.DEAD); return 0; case 0: /* Start sending options. */ - if ((h->gflags & LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE) == 0) - SET_NEXT_STATE (%OPT_EXPORT_NAME.START); + if ((h->gflags & LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE) == 0) { + if (h->opt_mode) + SET_NEXT_STATE (%.NEGOTIATING); + else + SET_NEXT_STATE (%OPT_EXPORT_NAME.START); + } else SET_NEXT_STATE (%OPT_STARTTLS.START); } return 0; + NEWSTYLE.PREPARE_OPT_ABORT: + assert ((h->gflags & LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE) != 0); + h->sbuf.option.version = htobe64 (NBD_NEW_VERSION); + h->sbuf.option.option = htobe32 (NBD_OPT_ABORT); + h->sbuf.option.optlen = htobe32 (0); + h->wbuf = &h->sbuf; + h->wlen = sizeof h->sbuf.option; + SET_NEXT_STATE (%SEND_OPT_ABORT); + return 0; + + NEWSTYLE.SEND_OPT_ABORT: + switch (send_from_wbuf (h)) { + case -1: SET_NEXT_STATE (%.DEAD); return 0; + case 0: + SET_NEXT_STATE (%SEND_OPTION_SHUTDOWN); + } + return 0; + + NEWSTYLE.SEND_OPTION_SHUTDOWN: + /* We don't care if the server replies to NBD_OPT_ABORT. However, + * unless we are in opt mode, we want to preserve the error message + * from a failed OPT_GO by moving to DEAD instead. + */ + if (h->sock->ops->shut_writes (h, h->sock)) { + if (h->opt_mode) + SET_NEXT_STATE (%.CLOSED); + else + SET_NEXT_STATE (%.DEAD); + } + return 0; + NEWSTYLE.FINISHED: SET_NEXT_STATE (%.READY); return 0; diff --git a/lib/opt.c b/lib/opt.c index 306a2e9..6243553 100644 --- a/lib/opt.c +++ b/lib/opt.c @@ -39,3 +39,37 @@ nbd_unlocked_get_opt_mode (struct nbd_handle *h) { return h->opt_mode; } + +static int +wait_for_option (struct nbd_handle *h) +{ + while (nbd_internal_is_state_connecting (get_next_state (h))) { + if (nbd_unlocked_poll (h, -1) == -1) + return -1; + } + + return 0; +} + +/* Issue NBD_OPT_ABORT and wait for the state change. */ +int +nbd_unlocked_opt_abort (struct nbd_handle *h) +{ + int r = nbd_unlocked_aio_opt_abort (h); + + if (r == -1) + return r; + + return wait_for_option (h); +} + +/* Issue NBD_OPT_ABORT without waiting. */ +int +nbd_unlocked_aio_opt_abort (struct nbd_handle *h) +{ + h->current_opt = NBD_OPT_ABORT; + + if (nbd_internal_run (h, cmd_issue) == -1) + debug (h, "option queued, ignoring state machine failure"); + return 0; +} diff --git a/tests/Makefile.am b/tests/Makefile.am index 46c819a..76b370a 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -163,6 +163,7 @@ check_PROGRAMS += \ can-not-cache-flag \ oldstyle \ newstyle-limited \ + opt-abort \ connect-unix \ connect-tcp \ aio-parallel \ @@ -198,6 +199,7 @@ TESTS += \ can-not-cache-flag \ oldstyle \ newstyle-limited \ + opt-abort \ connect-unix \ connect-tcp \ aio-parallel.sh \ @@ -373,6 +375,11 @@ newstyle_limited_CPPFLAGS = -I$(top_srcdir)/include newstyle_limited_CFLAGS = $(WARNINGS_CFLAGS) newstyle_limited_LDADD = $(top_builddir)/lib/libnbd.la +opt_abort_SOURCES = opt-abort.c +opt_abort_CPPFLAGS = -I$(top_srcdir)/include +opt_abort_CFLAGS = $(WARNINGS_CFLAGS) +opt_abort_LDADD = $(top_builddir)/lib/libnbd.la + connect_unix_SOURCES = connect-unix.c connect_unix_CPPFLAGS = -I$(top_srcdir)/include connect_unix_CFLAGS = $(WARNINGS_CFLAGS) diff --git a/tests/errors.c b/tests/errors.c index cde0f48..1ae6555 100644 --- a/tests/errors.c +++ b/tests/errors.c @@ -1,5 +1,5 @@ /* NBD client library in userspace - * Copyright (C) 2013-2019 Red Hat Inc. + * Copyright (C) 2013-2020 Red Hat Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -178,6 +178,15 @@ main (int argc, char *argv[]) } check (EINVAL, "nbd_connect_command: "); + /* nbd_opt_abort can only be called during negotiation. */ + if (nbd_opt_abort (nbd) != -1) { + fprintf (stderr, "%s: test failed: " + "nbd_opt_abort did not reject attempt in wrong state\n", + argv[0]); + exit (EXIT_FAILURE); + } + check (EINVAL, "nbd_opt_abort: "); + /* Try to notify that writes are ready when we aren't blocked on POLLOUT */ if (nbd_aio_notify_write (nbd) != -1) { fprintf (stderr, "%s: test failed: " diff --git a/tests/newstyle-limited.c b/tests/newstyle-limited.c index 424da19..cdda474 100644 --- a/tests/newstyle-limited.c +++ b/tests/newstyle-limited.c @@ -121,6 +121,32 @@ main (int argc, char *argv[]) } nbd_close (nbd); + /* Next try. Requesting opt_mode works, but opt_go is the only + * option that can succeed (via NBD_OPT_EXPORT_NAME); opt_abort is + * special-cased but moves to DEAD rather than CLOSED. + */ + nbd = nbd_create (); + if (nbd == NULL || + nbd_set_opt_mode (nbd, true) == -1 || + nbd_connect_command (nbd, args) == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + if (!nbd_aio_is_negotiating (nbd)) { + fprintf (stderr, "unexpected state after negotiating\n"); + exit (EXIT_FAILURE); + } + /* XXX Test nbd_opt_list when it is implemented */ + if (nbd_opt_abort (nbd) == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + if (!nbd_aio_is_dead (nbd)) { + fprintf (stderr, "unexpected state after opt_abort\n"); + exit (EXIT_FAILURE); + } + nbd_close (nbd); + /* And another try: an incorrect export name kills the connection, * rather than allowing a second try. */ diff --git a/tests/opt-abort.c b/tests/opt-abort.c new file mode 100644 index 0000000..3acc1fb --- /dev/null +++ b/tests/opt-abort.c @@ -0,0 +1,95 @@ +/* NBD client library in userspace + * Copyright (C) 2013-2020 Red Hat Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +/* Test behavior of nbd_opt_abort. */ + +#include <config.h> + +#include <stdio.h> +#include <stdlib.h> +#include <stdint.h> +#include <inttypes.h> +#include <string.h> +#include <errno.h> + +#include <libnbd.h> + +int +main (int argc, char *argv[]) +{ + struct nbd_handle *nbd; + int64_t r; + const char *s; + char *args[] = { "nbdkit", "-s", "--exit-with-parent", "-v", + "memory", "size=512", NULL }; + + /* Get into negotiating state. */ + nbd = nbd_create (); + if (nbd == NULL || + nbd_set_opt_mode (nbd, true) == -1 || + nbd_connect_command (nbd, args) == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + + /* Protocol should be "newstyle-fixed", with structured replies already + * negotiated. + */ + if (nbd_aio_is_negotiating (nbd) != true) { + fprintf (stderr, "unexpected state after connection\n"); + exit (EXIT_FAILURE); + } + s = nbd_get_protocol (nbd); + if (strcmp (s, "newstyle-fixed") != 0) { + fprintf (stderr, + "incorrect protocol \"%s\", expected \"newstyle-fixed\"\n", s); + exit (EXIT_FAILURE); + } + if ((r = nbd_get_structured_replies_negotiated (nbd)) != 1) { + fprintf (stderr, + "incorrect structured replies %" PRId64 ", expected 1\n", r); + exit (EXIT_FAILURE); + } + + /* Quitting negotiation should be graceful. */ + if (nbd_opt_abort (nbd) == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + if (nbd_aio_is_closed (nbd) != true) { + fprintf (stderr, "unexpected state after abort\n"); + exit (EXIT_FAILURE); + } + + /* As negotiation never finished, we have no size. */ + if ((r = nbd_get_size (nbd)) != -1) { + fprintf (stderr, "%s: test failed: incorrect size, " + "actual %" PRIi64 ", expected -1", + argv[0], r); + exit (EXIT_FAILURE); + } + if ((r = nbd_get_errno ()) != EINVAL) { + fprintf (stderr, "%s: test failed: unexpected errno, " + "actual %" PRIi64 ", expected %d EINVAL", + argv[0], r, EINVAL); + exit (EXIT_FAILURE); + } + + nbd_close (nbd); + exit (EXIT_SUCCESS); +} diff --git a/.gitignore b/.gitignore index 44b4655..6fdb362 100644 --- a/.gitignore +++ b/.gitignore @@ -172,6 +172,7 @@ Makefile.in /tests/meta-base-allocation /tests/newstyle-limited /tests/oldstyle +/tests/opt-abort /tests/pki/ /tests/read-only-flag /tests/read-write-flag -- 2.28.0

On 8/17/20 6:06 AM, Richard W.M. Jones wrote: Sure, makes sense. Correct - later patches add NBD_OPT_ABORT, NBD_OPT_GO, NBD_OPT_LIST, and NBD_OPT_INFO. I've also thought about, but haven't yet coded anything, for also giving the user control over NBD_OPT_STARTTLS (for tls=1 connections), NBD_OPT_STRUCTURED_REPLIES (mainly for interoperability testing), and NBD_OPT_INFO_META_CONTEXT. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org

There's more improvements coming, but this is enough to prove that we've got a working option mode. We can now select an export name from the same connection requesting the list. --- examples/list-exports.c | 64 ++++++++++++++++++++++++++++------------- 1 file changed, 44 insertions(+), 20 deletions(-) diff --git a/examples/list-exports.c b/examples/list-exports.c index 643e611..daea2ab 100644 --- a/examples/list-exports.c +++ b/examples/list-exports.c @@ -4,9 +4,27 @@ * $ qemu-nbd -x "hello" -t -k /tmp/sock disk.img * $ ./run examples/list-exports /tmp/sock * [0] hello - * Which export to connect to? 0 + * Which export to connect to (-1 to quit)? 0 * Connecting to hello ... * /tmp/sock: hello: size = 2048 bytes + * + * To test this with nbdkit (requires 1.22): + * $ nbdkit -U /tmp/sock sh - <<\EOF + * case $1 in + * list_exports) echo NAMES; echo foo; echo foobar ;; + * open) echo "$3" ;; + * get_size) echo "$2" | wc -c ;; + * pread) echo "$2" | dd skip=$4 count=$3 \ + * iflag=skip_bytes,count_bytes ;; + * *) exit 2 ;; + * esac + * EOF + * $ ./run examples/list-exports /tmp/sock + * [0] foo + * [1] foobar + * Which export to connect to (-1 to quit)? 1 + * Connecting to foobar ... + * /tmp/sock: foobar: size = 7 bytes */ #include <stdio.h> @@ -20,7 +38,7 @@ int main (int argc, char *argv[]) { - struct nbd_handle *nbd, *nbd2; + struct nbd_handle *nbd; int r, i; char *name, *desc; int64_t size; @@ -30,14 +48,15 @@ main (int argc, char *argv[]) exit (EXIT_FAILURE); } - /* Create the libnbd handle for querying exports. */ + /* Create the libnbd handle. */ nbd = nbd_create (); if (nbd == NULL) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } - /* Set the list exports mode in the handle. */ + /* Set opt mode and request list exports. */ + nbd_set_opt_mode (nbd, true); nbd_set_list_exports (nbd, true); /* Connect to the NBD server over a @@ -52,8 +71,8 @@ main (int argc, char *argv[]) fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } - - if (nbd_get_nr_list_exports (nbd) == 0) { + if (!nbd_aio_is_negotiating (nbd) || + nbd_get_nr_list_exports (nbd) == 0) { fprintf (stderr, "Server does not support " "listing exports.\n"); exit (EXIT_FAILURE); @@ -73,29 +92,34 @@ main (int argc, char *argv[]) } printf ("Which export to connect to? "); if (scanf ("%d", &i) != 1) exit (EXIT_FAILURE); + if (i == -1) { + if (nbd_opt_abort (nbd) == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + nbd_close (nbd); + exit (EXIT_SUCCESS); + } name = nbd_get_list_export_name (nbd, i); + if (name == NULL) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } printf ("Connecting to %s ...\n", name); - nbd_close (nbd); - /* Connect again to the chosen export. */ - nbd2 = nbd_create (); - if (nbd2 == NULL) { + /* Resume connecting to the chosen export. */ + if (nbd_set_export_name (nbd, name) == -1 || + nbd_opt_go (nbd) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } - - if (nbd_set_export_name (nbd2, name) == -1) { - fprintf (stderr, "%s\n", nbd_get_error ()); - exit (EXIT_FAILURE); - } - - if (nbd_connect_unix (nbd2, argv[1]) == -1) { - fprintf (stderr, "%s\n", nbd_get_error ()); + if (!nbd_aio_is_ready (nbd)) { + fprintf (stderr, "server closed early\n"); exit (EXIT_FAILURE); } /* Read the size in bytes and print it. */ - size = nbd_get_size (nbd2); + size = nbd_get_size (nbd); if (size == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); @@ -104,7 +128,7 @@ main (int argc, char *argv[]) argv[1], name, size); /* Close the libnbd handle. */ - nbd_close (nbd2); + nbd_close (nbd); free (name); -- 2.28.0

On Fri, Aug 14, 2020 at 05:00:28PM -0500, Eric Blake wrote: Yes, good to get rid of this horrible bit of code! Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v

This continues the changes for adding NBD_OPT_LIST support. Now, instead of libnbd malloc'ing storage itself, the user passes a callback that can handle name/description pairs however it likes, and we get rid of the artificial cap at 10000 exports. However, the user will probably end up malloc'ing a list themselves, as we can't call nbd_set_export_name, or even request NBD_OPT_INFO (in a future patch), from the context of the callback. The choice to have a completion callback makes it possible to reflect server errors back to the user. However, as this is during negotiation phase, when nothing else is running in parallel, we don't need the user to call additional APIs just to retire the results; instead, return values in the callbacks are ignored, and omitting a completion callback loses out on any server error. In the future, we may want to add a synchronous wrapper (maybe just for C?) that makes it easier to grab a malloc'd list of export names (probably not descriptions), but that requires more work to the generator to figure out whether we would return a char** list (with NULL terminator) or return int with a char*** parameter. --- lib/internal.h | 27 ++++---- generator/API.ml | 100 +++++++++++---------------- generator/states-newstyle-opt-list.c | 76 +++++++++----------- generator/states.c | 18 ++++- lib/handle.c | 64 +---------------- lib/opt.c | 86 ++++++++++++++++------- tests/newstyle-limited.c | 10 ++- examples/list-exports.c | 69 ++++++++++++------ interop/list-exports.c | 68 ++++++++++-------- info/nbdinfo.c | 75 ++++++++++++++------ 10 files changed, 316 insertions(+), 277 deletions(-) diff --git a/lib/internal.h b/lib/internal.h index 3d059cc..b723622 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -72,6 +72,15 @@ struct export { char *description; }; +struct command_cb { + union { + nbd_extent_callback extent; + nbd_chunk_callback chunk; + nbd_list_callback list; + } fn; + nbd_completion_callback completion; +}; + struct nbd_handle { /* Unique name assigned to this handle for debug messages * (to avoid having to print actual pointers). @@ -102,10 +111,7 @@ struct nbd_handle { /* Option negotiation mode. */ bool opt_mode; uint8_t current_opt; - - /* Results of nbd_opt_list. */ - size_t nr_exports; - struct export *exports; + struct command_cb opt_cb; /* Full info mode. */ bool full_info; @@ -186,7 +192,7 @@ struct nbd_handle { union { struct { struct nbd_fixed_new_option_reply_server server; - char str[NBD_MAX_STRING * 2]; /* name and description */ + char str[NBD_MAX_STRING * 2 + 1]; /* name, description, NUL */ } __attribute__((packed)) server; struct nbd_fixed_new_option_reply_info_export export; struct nbd_fixed_new_option_reply_info_block_size block_size; @@ -324,14 +330,6 @@ struct socket { const struct socket_ops *ops; }; -struct command_cb { - union { - nbd_extent_callback extent; - nbd_chunk_callback chunk; - } fn; - nbd_completion_callback completion; -}; - struct command { struct command *next; uint16_t flags; @@ -420,6 +418,9 @@ extern bool nbd_internal_is_state_processing (enum state state); extern bool nbd_internal_is_state_dead (enum state state); extern bool nbd_internal_is_state_closed (enum state state); +/* opt.c */ +extern void nbd_internal_free_option (struct nbd_handle *h); + /* protocol.c */ extern int nbd_internal_errno_of_nbd_error (uint32_t error); extern const char *nbd_internal_name_of_nbd_cmd (uint16_t type); diff --git a/generator/API.ml b/generator/API.ml index c660960..52970d3 100644 --- a/generator/API.ml +++ b/generator/API.ml @@ -127,8 +127,12 @@ let extent_closure = { "nr_entries"); CBMutable (Int "error") ] } +let list_closure = { + cbname = "list"; + cbargs = [ CBString "name"; CBString "description" ] +} let all_closures = [ chunk_closure; completion_closure; - debug_closure; extent_closure ] + debug_closure; extent_closure; list_closure ] (* Enums. *) let tls_enum = { @@ -757,23 +761,24 @@ enabled option mode."; "opt_list", { default_call with - args = []; ret = RErr; + args = [ Closure list_closure ]; ret = RInt; permitted_states = [ Negotiating ]; shortdesc = "request the server to list all exports during negotiation"; longdesc = "\ Request that the server list all exports that it supports. This can only be used if L<nbd_set_opt_mode(3)> enabled option mode. -In this mode, during connection we query the server for the list -of NBD exports and collect them in the handle. You can query -the list of exports provided by the server by calling -L<nbd_get_nr_list_exports(3)> and L<nbd_get_list_export_name(3)>. -After choosing the export you want, set the export name -(L<nbd_set_export_name(3)>), then finish connecting with L<nbd_opt_go(3)>. +The <list> function is called once per advertised export, with any +C<user_data> passed to this function, and with C<name> and C<description> +supplied by the server. Many servers omit descriptions, in which +case C<description> will be an empty string. Remember that it is not +safe to call L<nbd_set_export_name(3)> from within the context of the +callback function; rather, your code must copy any C<name> needed for +later use after this function completes. At present, the return value +of the callback is ignored, although a return of -1 should be avoided. -Some servers do not support listing exports at all. In -that case the connect call will fail with errno C<ENOTSUP> -and L<nbd_get_nr_list_exports(3)> will return 0. +For convenience, when this function succeeds, it returns the number +of exports that were advertised by the server. Not all servers understand this request, and even when it is understood, the server might intentionally send an empty list to avoid being an @@ -782,56 +787,15 @@ results. Thus, this function may succeed even when no exports are reported, or may fail but have a non-empty list. Likewise, the NBD protocol does not specify an upper bound for the number of exports that might be advertised, so client code should be aware that -a server may send a lengthy list; libnbd truncates the server reply -after 10000 exports. +a server may send a lengthy list. For L<nbd-server(1)> you will need to allow clients to make list requests by adding C<allowlist=true> to the C<[generic]> section of F</etc/nbd-server/config>. For L<qemu-nbd(8)>, a description is set with I<-D>."; example = Some "examples/list-exports.c"; - see_also = [Link "set_opt_mode"; Link "opt_go"; - Link "get_nr_list_exports"; Link "get_list_export_name"]; - }; - - "get_nr_list_exports", { - default_call with - args = []; ret = RInt; - permitted_states = [ Negotiating; Connected; Closed; Dead ]; - shortdesc = "return the number of exports returned by the server"; - longdesc = "\ -If option mode is in effect and you called L<nbd_opt_list(3)>, -this returns the number of exports returned by the server. This -may be 0 or incomplete for reasons given in L<nbd_opt_list(3)>."; - see_also = [Link "opt_list"; Link "get_list_export_name"; - Link "get_list_export_description"]; - }; - - "get_list_export_name", { - default_call with - args = [ Int "i" ]; ret = RString; - permitted_states = [ Negotiating; Connected; Closed; Dead ]; - shortdesc = "return the i'th export name"; - longdesc = "\ -If L<nbd_opt_list(3)> succeeded with option mode enabled, -this can be used to return the i'th export name -from the list returned by the server."; - see_also = [Link "opt_list"; Link "get_list_export_description"]; - }; - - "get_list_export_description", { - default_call with - args = [ Int "i" ]; ret = RString; - permitted_states = [ Negotiating; Connected; Closed; Dead ]; - shortdesc = "return the i'th export description"; - longdesc = "\ -If L<nbd_opt_list(3)> succeeded with option mode enabled, -this can be used to return the i'th export description -from the list returned by the server, which may be an empty string. - -Many servers omit a description. For L<qemu-nbd(8)>, a description -is set with I<-D>."; - see_also = [Link "set_opt_mode"; Link "opt_go"]; + see_also = [Link "set_opt_mode"; Link "aio_opt_list"; Link "opt_go"; + Link "set_export_name"]; }; "add_meta_context", { @@ -1940,6 +1904,28 @@ L<nbd_aio_is_connecting(3)> to return false."; see_also = [Link "set_opt_mode"; Link "opt_abort"]; }; + "aio_opt_list", { + default_call with + args = [ Closure list_closure ]; + optargs = [ OClosure completion_closure ]; + ret = RErr; + permitted_states = [ Negotiating ]; + shortdesc = "request the server to list all exports during negotiation"; + longdesc = "\ +Request that the server list all exports that it supports. This can +only be used if L<nbd_set_opt_mode(3)> enabled option mode. + +To determine when the request completes, wait for +L<nbd_aio_is_connecting(3)> to return false. Or supply the optional +C<completion_callback> which will be invoked as described in +L<libnbd(3)/Completion callbacks>, except that it is automatically +retired regardless of return value. Note that detecting whether the +server returns an error (as is done by the return value of the +synchronous counterpart) is only possible with a completion +callback."; + see_also = [Link "set_opt_mode"; Link "opt_list"]; + }; + "aio_pread", { default_call with args = [ BytesPersistOut ("buf", "count"); UInt64 "offset" ]; @@ -2548,9 +2534,6 @@ let first_version = [ "set_uri_allow_local_file", (1, 2); (* Added in 1.3.x development cycle, will be stable and supported in 1.4. *) - "get_nr_list_exports", (1, 4); - "get_list_export_name", (1, 4); - "get_list_export_description", (1, 4); "get_block_size", (1, 4); "set_full_info", (1, 4); "get_full_info", (1, 4); @@ -2564,6 +2547,7 @@ let first_version = [ "opt_list", (1, 4); "aio_opt_go", (1, 4); "aio_opt_abort", (1, 4); + "aio_opt_list", (1, 4); (* These calls are proposed for a future version of libnbd, but * have not been added to any released version so far. diff --git a/generator/states-newstyle-opt-list.c b/generator/states-newstyle-opt-list.c index 19b353e..b8d8025 100644 --- a/generator/states-newstyle-opt-list.c +++ b/generator/states-newstyle-opt-list.c @@ -24,7 +24,8 @@ STATE_MACHINE { NEWSTYLE.OPT_LIST.START: assert (h->gflags & LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE); - assert (h->opt_mode && h->exports && !h->nr_exports); + assert (h->opt_mode && h->current_opt == NBD_OPT_LIST); + assert (CALLBACK_IS_NOT_NULL (h->opt_cb.fn.list)); h->sbuf.option.version = htobe64 (NBD_NEW_VERSION); h->sbuf.option.option = htobe32 (NBD_OPT_LIST); h->sbuf.option.optlen = 0; @@ -67,17 +68,21 @@ STATE_MACHINE { uint32_t reply; uint32_t len; uint32_t elen; - struct export exp; - struct export *new_exports; + const char *name; + const char *desc; + char *tmp; + int err; reply = be32toh (h->sbuf.or.option_reply.reply); len = be32toh (h->sbuf.or.option_reply.replylen); switch (reply) { case NBD_REP_SERVER: /* Got one export. */ - if (len > maxpayload) + if (len >= maxpayload) debug (h, "skipping too large export name reply"); else { + /* server.str is oversized for trailing NUL byte convenience */ + h->sbuf.or.payload.server.str[len - 4] = '\0'; elen = be32toh (h->sbuf.or.payload.server.server.export_name_len); if (elen > len - 4 || elen > NBD_MAX_STRING || len - 4 - elen > NBD_MAX_STRING) { @@ -85,42 +90,23 @@ STATE_MACHINE { SET_NEXT_STATE (%.DEAD); return 0; } - /* Copy the export name and description to the handle list. */ - exp.name = strndup (h->sbuf.or.payload.server.str, elen); - if (exp.name == NULL) { - set_error (errno, "strdup"); - SET_NEXT_STATE (%.DEAD); - return 0; + if (elen == len + 4) { + tmp = NULL; + name = h->sbuf.or.payload.server.str; + desc = ""; } - exp.description = strndup (h->sbuf.or.payload.server.str + elen, - len - 4 - elen); - if (exp.description == NULL) { - set_error (errno, "strdup"); - free (exp.name); - SET_NEXT_STATE (%.DEAD); - return 0; + else { + tmp = strndup (h->sbuf.or.payload.server.str, elen); + if (tmp == NULL) { + set_error (errno, "strdup"); + SET_NEXT_STATE (%.DEAD); + return 0; + } + name = tmp; + desc = h->sbuf.or.payload.server.str + elen; } - new_exports = realloc (h->exports, - sizeof (*new_exports) * (h->nr_exports+1)); - if (new_exports == NULL) { - set_error (errno, "strdup"); - SET_NEXT_STATE (%.DEAD); - free (exp.name); - free (exp.description); - return 0; - } - h->exports = new_exports; - h->exports[h->nr_exports++] = exp; - } - - /* Just limit this so we don't receive unlimited amounts - * of data from the server. Note each export name can be - * up to 4K. - */ - if (h->nr_exports > 10000) { - set_error (0, "too many export names sent by the server"); - SET_NEXT_STATE (%.DEAD); - return 0; + CALL_CALLBACK (h->opt_cb.fn.list, name, desc); + free (tmp); } /* Wait for more replies. */ @@ -131,19 +117,23 @@ STATE_MACHINE { case NBD_REP_ACK: /* Finished receiving the list. */ - SET_NEXT_STATE (%.NEGOTIATING); - return 0; + err = 0; + break; default: if (handle_reply_error (h) == -1) { SET_NEXT_STATE (%.DEAD); return 0; } - set_error (ENOTSUP, "unexpected response, possibly the server does not " + err = ENOTSUP; + set_error (err, "unexpected response, possibly the server does not " "support listing exports"); - SET_NEXT_STATE (%.NEGOTIATING); - return 0; + break; } + + CALL_CALLBACK (h->opt_cb.completion, &err); + nbd_internal_free_option (h); + SET_NEXT_STATE (%.NEGOTIATING); return 0; } /* END STATE MACHINE */ diff --git a/generator/states.c b/generator/states.c index d2bb9b5..aa40467 100644 --- a/generator/states.c +++ b/generator/states.c @@ -1,5 +1,5 @@ /* nbd client library in userspace: state machine - * Copyright (C) 2013-2019 Red Hat Inc. + * Copyright (C) 2013-2020 Red Hat Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -111,9 +111,19 @@ send_from_wbuf (struct nbd_handle *h) return 0; /* move to next state */ } +/* Forcefully fail any in-flight option */ +static void +abort_option (struct nbd_handle *h) +{ + int err = nbd_get_errno () ? : ENOTCONN; + + CALL_CALLBACK (h->opt_cb.completion, &err); + nbd_internal_free_option (h); +} + /* Forcefully fail any remaining in-flight commands in list */ -void abort_commands (struct nbd_handle *h, - struct command **list) +static void +abort_commands (struct nbd_handle *h, struct command **list) { struct command *next, *cmd; @@ -168,6 +178,7 @@ STATE_MACHINE { DEAD: /* The caller should have used set_error() before reaching here */ assert (nbd_get_error ()); + abort_option (h); abort_commands (h, &h->cmds_to_issue); abort_commands (h, &h->cmds_in_flight); h->in_flight = 0; @@ -178,6 +189,7 @@ STATE_MACHINE { return -1; CLOSED: + abort_option (h); abort_commands (h, &h->cmds_to_issue); abort_commands (h, &h->cmds_in_flight); h->in_flight = 0; diff --git a/lib/handle.c b/lib/handle.c index c308461..7987d59 100644 --- a/lib/handle.c +++ b/lib/handle.c @@ -112,8 +112,6 @@ nbd_create (void) void nbd_close (struct nbd_handle *h) { - size_t i; - nbd_internal_set_error_context ("nbd_close"); if (h == NULL) @@ -126,13 +124,7 @@ nbd_close (struct nbd_handle *h) free (h->bs_entries); nbd_internal_reset_size_and_flags (h); - for (i = 0; i < h->nr_exports; ++i) { - free (h->exports[i].name); - free (h->exports[i].description); - } - free (h->exports); - free (h->canonical_name); - free (h->description); + nbd_internal_free_option (h); free_cmd_list (h->cmds_to_issue); free_cmd_list (h->cmds_in_flight); free_cmd_list (h->cmds_done); @@ -288,60 +280,6 @@ nbd_unlocked_get_export_description (struct nbd_handle *h) return r; } -int -nbd_unlocked_get_nr_list_exports (struct nbd_handle *h) -{ - if (!h->exports) { - set_error (EINVAL, "nbd_opt_list not yet run on this handle"); - return -1; - } - return (int) h->nr_exports; -} - -char * -nbd_unlocked_get_list_export_name (struct nbd_handle *h, - int i) -{ - char *name; - - if (!h->exports) { - set_error (EINVAL, "nbd_opt_list not yet run on this handle"); - return NULL; - } - if (i < 0 || i >= (int) h->nr_exports) { - set_error (EINVAL, "invalid index"); - return NULL; - } - name = strdup (h->exports[i].name); - if (!name) { - set_error (errno, "strdup"); - return NULL; - } - return name; -} - -char * -nbd_unlocked_get_list_export_description (struct nbd_handle *h, - int i) -{ - char *desc; - - if (!h->exports) { - set_error (EINVAL, "nbd_opt_list not yet run on this handle"); - return NULL; - } - if (i < 0 || i >= (int) h->nr_exports) { - set_error (EINVAL, "invalid index"); - return NULL; - } - desc = strdup (h->exports[i].description); - if (!desc) { - set_error (errno, "strdup"); - return NULL; - } - return desc; -} - int nbd_unlocked_add_meta_context (struct nbd_handle *h, const char *name) { diff --git a/lib/opt.c b/lib/opt.c index 7458b4b..1a5f645 100644 --- a/lib/opt.c +++ b/lib/opt.c @@ -26,6 +26,15 @@ #include "internal.h" +/* Internal function which frees an option with callback. */ +void +nbd_internal_free_option (struct nbd_handle *h) +{ + if (h->current_opt == NBD_OPT_LIST) + FREE_CALLBACK (h->opt_cb.fn.list); + FREE_CALLBACK (h->opt_cb.completion); +} + int nbd_unlocked_set_opt_mode (struct nbd_handle *h, bool value) { @@ -78,37 +87,47 @@ nbd_unlocked_opt_abort (struct nbd_handle *h) return wait_for_option (h); } +struct list_helper { + int count; + nbd_list_callback list; + int err; +}; +static int +list_visitor (void *opaque, const char *name, const char *description) +{ + struct list_helper *h = opaque; + if (h->count < INT_MAX) + h->count++; + CALL_CALLBACK (h->list, name, description); + return 0; +} +static int +list_complete (void *opaque, int *err) +{ + struct list_helper *h = opaque; + h->err = *err; + FREE_CALLBACK (h->list); + return 0; +} + /* Issue NBD_OPT_LIST and wait for the reply. */ int -nbd_unlocked_opt_list (struct nbd_handle *h) +nbd_unlocked_opt_list (struct nbd_handle *h, nbd_list_callback list) { - size_t i; + struct list_helper s = { .list = list }; + nbd_list_callback l = { .callback = list_visitor, .user_data = &s }; + nbd_completion_callback c = { .callback = list_complete, .user_data = &s }; - if ((h->gflags & LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE) == 0) { - set_error (ENOTSUP, "server is not using fixed newstyle protocol"); + if (nbd_unlocked_aio_opt_list (h, l, c) == -1) return -1; - } - /* Overwrite any previous results */ - if (h->exports) { - for (i = 0; i < h->nr_exports; ++i) { - free (h->exports[i].name); - free (h->exports[i].description); - } - h->nr_exports = 0; - } - else { - h->exports = malloc (sizeof *h->exports); - if (h->exports == NULL) { - set_error (errno, "malloc"); - return -1; - } + if (wait_for_option (h) == -1) + return -1; + if (s.err) { + set_error (s.err, "server replied with error to list request"); + return -1; } - - h->current_opt = NBD_OPT_LIST; - if (nbd_internal_run (h, cmd_issue) == -1) - debug (h, "option queued, ignoring state machine failure"); - return wait_for_option (h); + return s.count; } /* Issue NBD_OPT_GO (or NBD_OPT_EXPORT_NAME) without waiting. */ @@ -132,3 +151,22 @@ nbd_unlocked_aio_opt_abort (struct nbd_handle *h) debug (h, "option queued, ignoring state machine failure"); return 0; } + +/* Issue NBD_OPT_LIST without waiting. */ +int +nbd_unlocked_aio_opt_list (struct nbd_handle *h, nbd_list_callback list, + nbd_completion_callback complete) +{ + if ((h->gflags & LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE) == 0) { + set_error (ENOTSUP, "server is not using fixed newstyle protocol"); + return -1; + } + + assert (CALLBACK_IS_NULL (h->opt_cb.fn.list)); + h->opt_cb.fn.list = list; + h->opt_cb.completion = complete; + h->current_opt = NBD_OPT_LIST; + if (nbd_internal_run (h, cmd_issue) == -1) + debug (h, "option queued, ignoring state machine failure"); + return 0; +} diff --git a/tests/newstyle-limited.c b/tests/newstyle-limited.c index f2f1cba..ab6c1c2 100644 --- a/tests/newstyle-limited.c +++ b/tests/newstyle-limited.c @@ -76,6 +76,14 @@ pread_cb (void *data, return 0; } +static int +list_cb (void *opaque, const char *name, const char *descriptiong) +{ + /* This callback is unreachable; plain newstyle can't do OPT_LIST */ + fprintf (stderr, "%s: list callback mistakenly reached", progname); + exit (EXIT_FAILURE); +} + int main (int argc, char *argv[]) { @@ -136,7 +144,7 @@ main (int argc, char *argv[]) fprintf (stderr, "unexpected state after negotiating\n"); exit (EXIT_FAILURE); } - if (nbd_opt_list (nbd) != -1) { + if (nbd_opt_list (nbd, (nbd_list_callback) { .callback = list_cb }) != -1) { fprintf (stderr, "nbd_opt_list: expected failure\n"); exit (EXIT_FAILURE); } diff --git a/examples/list-exports.c b/examples/list-exports.c index fcd474b..9204c11 100644 --- a/examples/list-exports.c +++ b/examples/list-exports.c @@ -30,18 +30,52 @@ #include <stdio.h> #include <stdlib.h> #include <stdint.h> +#include <string.h> #include <inttypes.h> #include <errno.h> #include <libnbd.h> +struct export_list { + int i; + char **names; +}; + +/* Callback function for nbd_opt_list */ +static int +list_one (void *opaque, const char *name, + const char *description) +{ + struct export_list *l = opaque; + char **names; + + printf ("[%d] %s\n", l->i, name); + if (*description) + printf(" (%s)\n", description); + names = realloc (l->names, + (l->i + 1) * sizeof *names); + if (!names) { + perror ("realloc"); + exit (EXIT_FAILURE); + } + names[l->i] = strdup (name); + if (!names[l->i]) { + perror ("strdup"); + exit (EXIT_FAILURE); + } + l->names = names; + l->i++; + return 0; +} + int main (int argc, char *argv[]) { struct nbd_handle *nbd; - int r, i; - char *name, *desc; + int i; + const char *name; int64_t size; + struct export_list list = { 0 }; if (argc != 2) { fprintf (stderr, "%s socket\n", argv[0]); @@ -63,8 +97,7 @@ main (int argc, char *argv[]) * end up in option mode, then a * listing is not possible. */ - r = nbd_connect_unix (nbd, argv[1]); - if (r == -1) { + if (nbd_connect_unix (nbd, argv[1]) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } @@ -74,24 +107,16 @@ main (int argc, char *argv[]) exit (EXIT_FAILURE); } - /* Grab the export list. */ - if (nbd_opt_list (nbd) == -1) { + /* Print the export list. */ + if (nbd_opt_list (nbd, + (nbd_list_callback) { + .callback = list_one, + .user_data = &list, }) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } /* Display the list of exports. */ - for (i = 0; - i < nbd_get_nr_list_exports (nbd); - i++) { - name = nbd_get_list_export_name (nbd, i); - printf ("[%d] %s\n", i, name); - desc = nbd_get_list_export_description (nbd, i); - if (desc && *desc) - printf(" (%s)\n", desc); - free (name); - free (desc); - } printf ("Which export to connect to? "); if (scanf ("%d", &i) != 1) exit (EXIT_FAILURE); if (i == -1) { @@ -102,11 +127,11 @@ main (int argc, char *argv[]) nbd_close (nbd); exit (EXIT_SUCCESS); } - name = nbd_get_list_export_name (nbd, i); - if (name == NULL) { - fprintf (stderr, "%s\n", nbd_get_error ()); + if (i < 0 || i >= list.i) { + fprintf (stderr, "index %d out of range", i); exit (EXIT_FAILURE); } + name = list.names[i]; printf ("Connecting to %s ...\n", name); /* Resume connecting to the chosen export. */ @@ -132,7 +157,9 @@ main (int argc, char *argv[]) /* Close the libnbd handle. */ nbd_close (nbd); - free (name); + for (i = 0; i < list.i; i++) + free (list.names[i]); + free (list.names); exit (EXIT_SUCCESS); } diff --git a/interop/list-exports.c b/interop/list-exports.c index 5af1234..44d5664 100644 --- a/interop/list-exports.c +++ b/interop/list-exports.c @@ -27,14 +27,44 @@ #include <libnbd.h> +static const char *exports[] = { EXPORTS }; +static const char *descriptions[] = { DESCRIPTIONS }; +static const size_t nr_exports = sizeof exports / sizeof exports[0]; + +static char *progname; + +static int +check (void *opaque, const char *name, const char *description) +{ + size_t *i = opaque; + if (*i == nr_exports) { + fprintf (stderr, "%s: server returned more exports than expected", + progname); + exit (EXIT_FAILURE); + } + if (strcmp (exports[*i], name) != 0) { + fprintf (stderr, "%s: expected export \"%s\", but got \"%s\"\n", + progname, exports[*i], name); + exit (EXIT_FAILURE); + } + if (strcmp (descriptions[*i], description) != 0) { + fprintf (stderr, "%s: expected description \"%s\", but got \"%s\"\n", + progname, descriptions[*i], description); + exit (EXIT_FAILURE); + } + (*i)++; + return 0; +} + int main (int argc, char *argv[]) { struct nbd_handle *nbd; char tmpfile[] = "/tmp/nbdXXXXXX"; - int fd, r; - size_t i; - char *name, *desc; + int fd; + size_t i = 0; + + progname = argv[0]; /* Create a sparse temporary file. */ fd = mkstemp (tmpfile); @@ -62,7 +92,9 @@ main (int argc, char *argv[]) #else #define NBD_CONNECT nbd_connect_command #endif - if (NBD_CONNECT (nbd, args) == -1 || nbd_opt_list (nbd) == -1) { + if (NBD_CONNECT (nbd, args) == -1 || + nbd_opt_list (nbd, (nbd_list_callback) { + .callback = check, .user_data = &i}) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); unlink (tmpfile); exit (EXIT_FAILURE); @@ -72,34 +104,12 @@ main (int argc, char *argv[]) unlink (tmpfile); /* Check for expected number of exports. */ - const char *exports[] = { EXPORTS }; - const char *descriptions[] = { DESCRIPTIONS }; - const size_t nr_exports = sizeof exports / sizeof exports[0]; - r = nbd_get_nr_list_exports (nbd); - if (r != nr_exports) { - fprintf (stderr, "%s: expected %zu export, but got %d\n", - argv[0], nr_exports, r); + if (i != nr_exports) { + fprintf (stderr, "%s: expected %zu export, but got %zu\n", + argv[0], nr_exports, i); exit (EXIT_FAILURE); } - /* Check the export names and descriptions. */ - for (i = 0; i < nr_exports; ++i) { - name = nbd_get_list_export_name (nbd, (int) i); - if (strcmp (name, exports[i]) != 0) { - fprintf (stderr, "%s: expected export \"%s\", but got \"%s\"\n", - argv[0], exports[i], name); - exit (EXIT_FAILURE); - } - free (name); - desc = nbd_get_list_export_description (nbd, (int) i); - if (strcmp (desc, descriptions[i]) != 0) { - fprintf (stderr, "%s: expected description \"%s\", but got \"%s\"\n", - argv[0], descriptions[i], desc); - exit (EXIT_FAILURE); - } - free (desc); - } - nbd_opt_abort (nbd); nbd_close (nbd); exit (EXIT_SUCCESS); diff --git a/info/nbdinfo.c b/info/nbdinfo.c index cdc0db8..81913dd 100644 --- a/info/nbdinfo.c +++ b/info/nbdinfo.c @@ -36,6 +36,14 @@ static bool probe_content, content_flag, no_content_flag; static bool json_output = false; static bool size_only = false; +static struct export_list { + size_t len; + char **names; + char **descs; +} export_list; + +static int collect_export (void *opaque, const char *name, + const char *desc); static void list_one_export (struct nbd_handle *nbd, const char *desc, bool first, bool last); static void list_all_exports (struct nbd_handle *nbd1, const char *uri); @@ -196,7 +204,8 @@ main (int argc, char *argv[]) } if (list_all) { - if (nbd_opt_list (nbd) == -1) { + if (nbd_opt_list (nbd, (nbd_list_callback) { + .callback = collect_export, .user_data = &export_list}) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } @@ -253,10 +262,46 @@ main (int argc, char *argv[]) printf ("}\n"); } + for (i = 0; i < export_list.len; i++) { + free (export_list.names[i]); + free (export_list.descs[i]); + } + free (export_list.names); + free (export_list.descs); nbd_close (nbd); exit (EXIT_SUCCESS); } +static int +collect_export (void *opaque, const char *name, const char *desc) +{ + struct export_list *l = opaque; + char **names, **descs; + + names = realloc (l->names, (l->len + 1) * sizeof name); + descs = realloc (l->descs, (l->len + 1) * sizeof desc); + if (!names || !descs) { + perror ("realloc"); + exit (EXIT_FAILURE); + } + l->names = names; + l->descs = descs; + l->names[l->len] = strdup (name); + if (!l->names[l->len]) { + perror ("strdup"); + exit (EXIT_FAILURE); + } + if (*desc) { + l->descs[l->len] = strdup (desc); + if (!l->descs[l->len]) { + perror ("strdup"); + exit (EXIT_FAILURE); + } + } + l->len++; + return 0; +} + static void list_one_export (struct nbd_handle *nbd, const char *desc, bool first, bool last) @@ -415,29 +460,17 @@ list_one_export (struct nbd_handle *nbd, const char *desc, static void list_all_exports (struct nbd_handle *nbd1, const char *uri) { - int i; - int count = nbd_get_nr_list_exports (nbd1); + size_t i; - if (count == -1) { - fprintf (stderr, "unable to obtain list of exports: %s\n", - nbd_get_error ()); - exit (EXIT_FAILURE); - } - if (count == 0 && json_output) + if (export_list.len == 0 && json_output) printf ("\t\"exports\": []\n"); - for (i = 0; i < count; ++i) { - char *name, *desc; + for (i = 0; i < export_list.len; ++i) { + const char *name; struct nbd_handle *nbd2; - name = nbd_get_list_export_name (nbd1, i); - if (!name) { - fprintf (stderr, "unable to obtain export name: %s\n", - nbd_get_error ()); - exit (EXIT_FAILURE); - } - /* Connect to the original URI, but using opt mode to alter the export. */ + name = export_list.names[i]; nbd2 = nbd_create (); if (nbd2 == NULL) { fprintf (stderr, "%s\n", nbd_get_error ()); @@ -454,12 +487,10 @@ list_all_exports (struct nbd_handle *nbd1, const char *uri) } /* List the metadata of this export. */ - desc = nbd_get_list_export_description (nbd1, i); - list_one_export (nbd2, desc, i == 0, i + 1 == count); + list_one_export (nbd2, export_list.descs[i], i == 0, + i + 1 == export_list.len); nbd_close (nbd2); - free (desc); - free (name); } } -- 2.28.0

On Fri, Aug 14, 2020 at 05:00:31PM -0500, Eric Blake wrote: I don't know, but I would suggest first of all calling h.set_debug(True) which will display what the C API nbd_opt_go is actually returning. If it's not returning an error then Python bindings won't throw an exception (perhaps it just skips through the states if h->opt_mode is false?). I'm fairly sure the Python bindings themselves should be robust and always throw an exception if the underlying C returns an error, but if you suspect a problem them have a look at the generated code in python/methods.c for your new function. The patch itself looks like a good idea. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW