Re: [Libguestfs] [PATCH 0/2] Remove virt-p2v from libguestfs
ACK series, thanks. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top ___ Libguestfs mailing list Libguestfs@redhat.com https://www.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] [PATCH 2/2] Remove remaining virt-p2v bits
Remove (almost) all the remaining bits related to virt-p2v.
---
.gitignore | 4 -
Makefile.am | 4 +-
bash/Makefile.am | 4 -
bash/virt-alignment-scan | 18 --
common/miniexpect/Makefile.am| 51
common/miniexpect/README | 31 --
common/miniexpect/miniexpect.c | 489 --
common/miniexpect/miniexpect.h | 110 ---
common/miniexpect/miniexpect.pod | 496 ---
configure.ac | 5 +-
contrib/README | 3 -
docs/C_SOURCE_FILES | 2 -
docs/guestfs-building.pod| 55 +---
docs/guestfs-hacking.pod | 83 --
generator/authors.ml | 9 -
generator/authors.mli| 1 -
m4/guestfs-v2v.m4| 52 +---
po-docs/podfiles | 1 -
po/POTFILES | 1 -
run.in | 7 -
20 files changed, 6 insertions(+), 1420 deletions(-)
delete mode 100644 common/miniexpect/Makefile.am
delete mode 100644 common/miniexpect/README
delete mode 100644 common/miniexpect/miniexpect.c
delete mode 100644 common/miniexpect/miniexpect.h
delete mode 100644 common/miniexpect/miniexpect.pod
diff --git a/.gitignore b/.gitignore
index ff45d9d73..d10f30fd4 100644
--- a/.gitignore
+++ b/.gitignore
@@ -64,9 +64,6 @@ Makefile.in
/bash/virt-inspector
/bash/virt-log
/bash/virt-ls
-/bash/virt-p2v-make-disk
-/bash/virt-p2v-make-kickstart
-/bash/virt-p2v-make-kiwi
/bash/virt-resize
/bash/virt-sysprep
/bash/virt-sparsify
@@ -129,7 +126,6 @@ Makefile.in
/common/errnostring/errnostring-gperf.c
/common/errnostring/errnostring-gperf.gperf
/common/errnostring/errnostring.h
-/common/miniexpect/miniexpect.3
/common/mlaugeas/.depend
/common/mlgettext/.depend
/common/mlgettext/common_gettext.ml
diff --git a/Makefile.am b/Makefile.am
index 1b7c98319..1cc21961a 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -328,7 +328,7 @@ ChangeLog: configure.ac
docs/C_SOURCE_FILES: configure.ac
rm -f $@ $@-t
find $(DIST_SUBDIRS) -name '*.[ch]' | \
- grep -v -E
'^(builder/index-parse\.|builder/index-scan\.|common/mllibvirt/libvirt_c\.c|examples/|gnulib/|gobject/|java/com_redhat_et_libguestfs|perl/|p2v/|php/extension/config\.h|ruby/ext/guestfs/extconf\.h|tests/|test-data/)'
| \
+ grep -v -E
'^(builder/index-parse\.|builder/index-scan\.|common/mllibvirt/libvirt_c\.c|examples/|gnulib/|gobject/|java/com_redhat_et_libguestfs|perl/|php/extension/config\.h|ruby/ext/guestfs/extconf\.h|tests/|test-data/)'
| \
grep -v -E '/(guestfs|rc)_protocol\.' | \
grep -v -E '.*/errnostring\.' | \
grep -v -E '.*-gperf\.' | \
@@ -340,7 +340,7 @@ po/POTFILES: configure.ac
rm -f $@ $@-t
cd $(srcdir); \
find $(DIST_SUBDIRS) -name '*.c' -o -name '*.pl' -o -name '*.pm' | \
- grep -v -E
'^(examples|gnulib|perl/(blib|examples)|p2v|po-docs|tests|test-data)/' | \
+ grep -v -E
'^(examples|gnulib|perl/(blib|examples)|po-docs|tests|test-data)/' | \
grep -v -E '/((guestfs|rc)_protocol\.c)$$' | \
grep -v -E '^python/utils\.c$$' | \
grep -v -E '^perl/lib/Sys/Guestfs\.c$$' | \
diff --git a/bash/Makefile.am b/bash/Makefile.am
index 4f595cd04..61b37deba 100644
--- a/bash/Makefile.am
+++ b/bash/Makefile.am
@@ -44,9 +44,6 @@ symlinks = \
virt-inspector \
virt-log \
virt-ls \
- virt-p2v-make-disk \
- virt-p2v-make-kickstart \
- virt-p2v-make-kiwi \
virt-resize \
virt-sparsify \
virt-sysprep \
@@ -79,7 +76,6 @@ guestunmount \
virt-builder virt-cat virt-customize virt-df virt-dib virt-diff \
virt-edit virt-filesystems virt-format virt-get-kernel virt-inspector \
virt-log virt-ls \
-virt-p2v-make-disk virt-p2v-make-kickstart virt-p2v-make-kiwi \
virt-resize virt-sparsify virt-sysprep \
virt-tail:
rm -f $@
diff --git a/bash/virt-alignment-scan b/bash/virt-alignment-scan
index 1014677f6..3d4c114e9 100644
--- a/bash/virt-alignment-scan
+++ b/bash/virt-alignment-scan
@@ -169,24 +169,6 @@ _virt_ls ()
} &&
complete -o default -F _virt_ls virt-ls
-_virt_p2v_make_disk ()
-{
-_guestfs_virttools "virt-p2v-make-disk" 1
-} &&
-complete -o default -F _virt_p2v_make_disk virt-p2v-make-disk
-
-_virt_p2v_make_kickstart ()
-{
-_guestfs_virttools "virt-p2v-make-kickstart" 1
-} &&
-complete -o default -F _virt_p2v_make_kickstart virt-p2v-make-kickstart
-
-_virt_p2v_make_kiwi ()
-{
-_guestfs_virttools "virt-p2v-make-kiwi" 1
-} &&
-complete -o default -F _virt_p2v_make_kiwi virt-p2v-make-kiwi
-
_virt_resize ()
{
_guestfs_virttools "virt-resize" 0
diff --git a/common/miniexpect/Makefile.am b/common/miniexpect/Makefile.am
deleted file mode 100644
index f223b8d79..0
--- a/common/miniexpect/Makefile.am
+++ /dev/null
@@ -1,51 +0,0 @@
-# libguestfs virt-p2v
-# Copyright (C) 2009-2019 Red Hat Inc.
-#
[Libguestfs] [PATCH 0/2] Remove virt-p2v from libguestfs
Now that virt-p2v has its own repository [1] and releases [2], it is time to remove it from libguestfs. [1] https://github.com/libguestfs/virt-p2v [2] http://download.libguestfs.org/virt-p2v/ Pino Toscano (2): Remove virt-p2v Remove remaining virt-p2v bits .gitignore|4 - Makefile.am |7 +- bash/Makefile.am |4 - bash/virt-alignment-scan | 18 - common/miniexpect/Makefile.am | 51 - common/miniexpect/README | 31 - common/miniexpect/miniexpect.c| 489 common/miniexpect/miniexpect.h| 110 - common/miniexpect/miniexpect.pod | 496 configure.ac | 18 +- contrib/README|3 - docs/C_SOURCE_FILES |2 - docs/guestfs-building.pod | 55 +- docs/guestfs-hacking.pod | 83 - generator/authors.ml |9 - generator/authors.mli |1 - generator/main.ml |2 - installcheck.sh.in|3 - m4/guestfs-v2v.m4 | 52 +- p2v/.gitignore| 49 - p2v/Makefile.am | 376 --- p2v/contrib/aux-scripts/do-build.sh | 196 -- p2v/contrib/build-p2v-iso.sh | 155 -- ...BLE-AUTOMATIC-REMOTE-PORT-ALLOCATION.patch | 54 - ...-5-ONLY-QEMU-NBD-1.4-HAS-NO-f-OPTION.patch | 34 - p2v/contrib/test-p2v-iso.sh | 63 - p2v/conversion.c | 668 - p2v/cpuid.c | 222 -- p2v/dependencies.m4 | 181 -- p2v/generate-p2v-authors.pl | 54 - p2v/generate-p2v-config.pl| 915 --- p2v/gui-gtk2-compat.h | 117 - p2v/gui-gtk3-compat.h | 140 - p2v/gui.c | 2295 - p2v/inhibit.c | 154 -- p2v/issue | 16 - p2v/kernel-cmdline.c | 196 -- p2v/kernel.c | 158 -- p2v/kiwi-config.sh| 73 - p2v/kiwi-config.xml.in| 92 - p2v/launch-virt-p2v | 51 - p2v/main.c| 583 - p2v/nbd.c | 840 -- p2v/p2v.h | 136 - p2v/p2v.ks.in | 193 -- p2v/p2v.service | 38 - p2v/physical-xml.c| 304 --- p2v/rtc.c | 165 -- p2v/ssh.c | 1203 - p2v/test-virt-p2v-cmdline.sh | 53 - p2v/test-virt-p2v-docs.sh | 24 - p2v/test-virt-p2v-nbdkit.sh | 59 - p2v/test-virt-p2v-pxe.sh | 96 - p2v/test-virt-p2v-pxe.sshd_config.in | 43 - p2v/test-virt-p2v-scp.sh | 62 - p2v/test-virt-p2v-ssh.sh | 61 - p2v/test-virt-p2v.sh | 57 - p2v/utils.c | 256 -- p2v/virt-p2v-make-disk.in | 267 -- p2v/virt-p2v-make-disk.pod| 218 -- p2v/virt-p2v-make-kickstart.in| 241 -- p2v/virt-p2v-make-kickstart.pod | 339 --- p2v/virt-p2v-make-kiwi.in | 233 -- p2v/virt-p2v-make-kiwi.pod| 184 -- p2v/virt-p2v.pod | 757 -- p2v/whole-file.c | 95 - po-docs/language.mk |3 - po-docs/podfiles |6 - po/POTFILES |1 - run.in|7 - 70 files changed, 6 insertions(+), 14215 deletions(-) delete mode 100644 common/miniexpect/Makefile.am delete mode 100644 common/miniexpect/README delete mode 100644 common/miniexpect/miniexpect.c delete mode 100644 common/miniexpect/miniexpect.h delete mode 100644 common/miniexpect/miniexpect.pod delete mode 100644 p2v/.gitignore delete mode 100644 p2v/Makefile.am delete mode 100644 p2v/contrib/aux-scripts/do-build.sh delete mode 100755 p2v/contrib/build-p2v-iso.sh delete mode 100644 p2v/contrib/patches/0001-RHEL-5-ONLY-DISABLE-AUTOMATIC-REMOTE-PORT-ALLOCATION.patch delete mode 100644 p2v/contrib/patches/0002-RHEL-5-ONLY-QEMU-NBD-1.4-HAS-NO-f-OPTION.patch delete mode 100755 p2v/contrib/test-p2v-iso.sh delete
Re: [Libguestfs] [PATCH nbdkit] server: Add nbdkit_export_name() to allow export name to be read.
On Tue, Sep 10, 2019 at 08:11:10AM -0500, Eric Blake wrote:
> > +=head1 EXPORT NAME
> > +
> > +If the client negotiated an NBD export name with nbdkit then plugins
> > +may read this from any connected callbacks. Nbdkit's normal behaviour
> > +is to accept any export name passed by the client, log it in debug
> > +output, but otherwise ignore it. By using C
> > +plugins may choose to filter by export name or serve different
> > +content.
>
> We may want to add text here and/or in .can_multi_conn to clarify that
> advertising multi-conn only matters to clients connecting to the SAME
> export name; it is safe to advertise multi-conn even when serving
> different content to different export names.
Good point ...
> > +
> > +=head2 C
> > +
> > + const char *nbdkit_export_name (void);
> > +
> > +Return the optional NBD export name if one was negotiated with the
> > +current client (this uses thread-local magic so no parameter is
> > +required). The returned string is only valid while the client is
> > +connected, so if you need to store it in the plugin you must copy it.
> > +
> > +The export name is a free-form text string, it is not necessarily a
> > +path or filename and it does not need to begin with a C<'/'>
> > +character. The NBD protocol describes the empty string (C<"">) as a
> > +representing a "default export" or to be used in cases where the
> > +export name does not make sense.
> > +
> > +This may return C which does I indicate an error:
> > +
> > +=over 4
> > +
> > +=item *
> > +
> > +It only returns the export name when there is a connected client.
> > +
> > +=item *
> > +
> > +If the server is using the oldstyle protocol the client does not send
> > +an export name.
>
> Or, we could just blindly state that in oldstyle mode, the client always
> behaves as if it connected to the export named "". After all, qemu 3.1
> was where we changed things to allow a client to request an explicit
> export name of "" yet still connect to an oldstyle server in that case.
Yes, we can do this, and then reserve NULL for an actual error
(calling in a non-connected state).
> > +++ b/server/protocol-handshake-newstyle.c
> > @@ -274,11 +274,17 @@ negotiate_handshake_newstyle_options (struct
> > connection *conn)
> >if (conn_recv_full (conn, data, optlen,
> >"read: %s: %m", name_of_nbd_opt (option)) == -1)
> > return -1;
> > - /* Apart from printing it, ignore the export name. */
> > + /* Print the export name and save it in the connection. */
> >data[optlen] = '\0';
> > - debug ("newstyle negotiation: %s: "
> > - "client requested export '%s' (ignored)",
> > + debug ("newstyle negotiation: %s: client requested export '%s'",
> > name_of_nbd_opt (option), data);
> > + free (conn->exportname);
> > + conn->exportname = malloc (optlen+1);
> > + if (conn->exportname == NULL) {
> > +nbdkit_error ("malloc: %m");
> > +return -1;
> > + }
> > + memcpy (conn->exportname, data, optlen+1);
>
> Why malloc/memcpy instead of strdup() (two instances)?
When I wrote that I believed that [because NBD is using Pascal-style
len + data] we could handle arbitrary 8 bit sequences, eg. UTF-16LE
would be valid. However I've just checked the NBD protocol document
and I see that we specify strings must be UTF-8, so strcpy would be
sensible here.
> Otherwise the idea looks reasonable. I replied in the other thread about
> how we could expose this to the sh plugin's open, even if we don't bump
> to v3 API yet.
I'm trying to do the minimum to solve Xiubo's problem (AIUI).
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [nbdkit] Access export name from plugins
On Tue, Sep 10, 2019 at 07:58:49AM -0500, Eric Blake wrote:
> On 9/10/19 5:01 AM, Richard W.M. Jones wrote:
> > Of course at the moment nbdkit parses the NBD export name option but
> > doesn't really do anything with it (except logging it).
> >
> > I wonder if we should make this available to plugins, in case they
> > wish to serve different content to different clients based on the
> > export name. Note I'm not suggesting that we use this feature in any
> > existing plugins.
> >
> > If we wanted to do this there seem like two possible ways to do it:
>
> at least two ways.
>
> >
> > (1) Add a call, like nbdkit_export_name, which plugins could call from
> > any connected method to get the current export name, eg:
> >
> > static void *
> > myplugin_open (int readonly)
> > {
> > const char *export = nbdkit_export_name ();
> >
> > ... Do something based on the export name ...
> > }
>
> I would also consider adding an optional callback:
>
> struct nbdkit_export {
> char *name;
> size_t size;
> // maybe other fields for supporting NBD_OPT_INFO...
> };
>
> const nbdkit_export *(*list_exports) (void);
I see this as needed too, but solving a slightly different problem
from the one which Xiubo Li has.
However there's an interesting point here: Should the list of exports
be handled separately or should it be integrated?
For example, we could imagine first implementing nbdkit_export_name as
I described, and ignoring the NBD_OPT_LIST problem. Later we decide
to implement NBD_OPT_LIST by adding a .list_exports callback. But a
(bad) plugin could answer with a completely different list of exports
from what it supports in the .open call.
Or we could try to find a way to tie these two together, but I'm not
clear at the moment how we would do that.
(I'm inclined to think this is over-designing things and it doesn't
matter very much if a plugin gives the wrong answer when asked to list
exports.)
[...]
> Note that with sh plugins, we could still make things work with v2, as
> follows:
>
> sh_open() calls nbdkit_export_name() unconditionally, prior to forming
> the command line for calling into the script. Then we alter the command
> line we pass to the script:
>
> /path/to/script open
>
> where the sh plugin always gets to know what export name the client
> chose, thanks to the new exportname parameter. Compatibility-wise,
> older scripts fall in one of two categories: either they ignore
> unexpected parameters (no change required), or they choke because
> exportname was unexpected (but it's a one-line change to the script to
> deal with it). New scripts run against an older nbdkit that did not
> provide the exportname parameter will see "$3" as empty (if the script
> runs under 'set -u', then spell things "${3-}").
sh plugins really should ignore extra parameters they don't know
about. We should probably say that in the man page.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine. Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/
___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH nbdkit] server: Add nbdkit_export_name() to allow export name to be read.
On 9/10/19 6:20 AM, Richard W.M. Jones wrote:
> This allows plugins (or filters) to read the export name which was
> passed to the server from the client.
>
> UNFINISHED:
>
> - Needs tests
> ---
> TODO | 8 ++
> docs/nbdkit-plugin.pod | 39
> server/connections.c | 1 +
> server/internal.h| 1 +
> server/protocol-handshake-newstyle.c | 30 -
> server/public.c | 10 +++
> 6 files changed, 77 insertions(+), 12 deletions(-)
>
> diff --git a/TODO b/TODO
> index 49b60b1..2468d74 100644
> --- a/TODO
> +++ b/TODO
> @@ -62,6 +62,12 @@ General ideas for improvements
>and also look at the implementation of the -swap option in
>nbd-client.
>
> +* Clients should be able to list export names supported by plugins.
> + Current behaviour is not really correct: We only list the -e
> + parameter from the command line, which is different from the export
> + name(s) that a plugin might want to support. Probably we should
> + deprecate the -e option entirely since it does nothing useful.
See my idea in the other thread for how we could add a callback for
plugins to advertise the list of exports that they want exposed to clients.
> +++ b/docs/nbdkit-plugin.pod
> @@ -362,6 +362,45 @@ requested, or -1 after calling C if there
> is no point in
> continuing the current command. Attempts to sleep more than
> C seconds are treated as an error.
>
> +=head1 EXPORT NAME
> +
> +If the client negotiated an NBD export name with nbdkit then plugins
> +may read this from any connected callbacks. Nbdkit's normal behaviour
> +is to accept any export name passed by the client, log it in debug
> +output, but otherwise ignore it. By using C
> +plugins may choose to filter by export name or serve different
> +content.
We may want to add text here and/or in .can_multi_conn to clarify that
advertising multi-conn only matters to clients connecting to the SAME
export name; it is safe to advertise multi-conn even when serving
different content to different export names.
> +
> +=head2 C
> +
> + const char *nbdkit_export_name (void);
> +
> +Return the optional NBD export name if one was negotiated with the
> +current client (this uses thread-local magic so no parameter is
> +required). The returned string is only valid while the client is
> +connected, so if you need to store it in the plugin you must copy it.
> +
> +The export name is a free-form text string, it is not necessarily a
> +path or filename and it does not need to begin with a C<'/'>
> +character. The NBD protocol describes the empty string (C<"">) as a
> +representing a "default export" or to be used in cases where the
> +export name does not make sense.
> +
> +This may return C which does I indicate an error:
> +
> +=over 4
> +
> +=item *
> +
> +It only returns the export name when there is a connected client.
> +
> +=item *
> +
> +If the server is using the oldstyle protocol the client does not send
> +an export name.
Or, we could just blindly state that in oldstyle mode, the client always
behaves as if it connected to the export named "". After all, qemu 3.1
was where we changed things to allow a client to request an explicit
export name of "" yet still connect to an oldstyle server in that case.
> +++ b/server/protocol-handshake-newstyle.c
> @@ -274,11 +274,17 @@ negotiate_handshake_newstyle_options (struct connection
> *conn)
>if (conn_recv_full (conn, data, optlen,
>"read: %s: %m", name_of_nbd_opt (option)) == -1)
> return -1;
> - /* Apart from printing it, ignore the export name. */
> + /* Print the export name and save it in the connection. */
>data[optlen] = '\0';
> - debug ("newstyle negotiation: %s: "
> - "client requested export '%s' (ignored)",
> + debug ("newstyle negotiation: %s: client requested export '%s'",
> name_of_nbd_opt (option), data);
> + free (conn->exportname);
> + conn->exportname = malloc (optlen+1);
> + if (conn->exportname == NULL) {
> +nbdkit_error ("malloc: %m");
> +return -1;
> + }
> + memcpy (conn->exportname, data, optlen+1);
Why malloc/memcpy instead of strdup() (two instances)?
Otherwise the idea looks reasonable. I replied in the other thread about
how we could expose this to the sh plugin's open, even if we don't bump
to v3 API yet.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [nbdkit] Access export name from plugins
On 9/10/19 5:01 AM, Richard W.M. Jones wrote:
> Of course at the moment nbdkit parses the NBD export name option but
> doesn't really do anything with it (except logging it).
>
> I wonder if we should make this available to plugins, in case they
> wish to serve different content to different clients based on the
> export name. Note I'm not suggesting that we use this feature in any
> existing plugins.
>
> If we wanted to do this there seem like two possible ways to do it:
at least two ways.
>
> (1) Add a call, like nbdkit_export_name, which plugins could call from
> any connected method to get the current export name, eg:
>
> static void *
> myplugin_open (int readonly)
> {
> const char *export = nbdkit_export_name ();
>
> ... Do something based on the export name ...
> }
I would also consider adding an optional callback:
struct nbdkit_export {
char *name;
size_t size;
// maybe other fields for supporting NBD_OPT_INFO...
};
const nbdkit_export *(*list_exports) (void);
A plugin that does not implement the callback advertises whatever export
name was passed on the command line with -e (or defaults to advertising
just ""), and accepts any name, or we could even state that NBD_OPT_LIST
fails unless the callback is present.
A plugin that DOES implement the callback causes -e to fail (the plugin
is now in charge of the export names, instead of -e), and makes
NBD_OPT_LIST and NBD_OPT_INFO advertise the list returned by
.list_exports. The plugin is responsible for returning an array of zero
or more structures terminated by an entry where .name is NULL (although
returning zero entries means the plugin is not currently accepting
clients, as no export name from the client will match). The
.list_exports() callback would be called _prior_ to .open on a
per-connection basis (thus, there is no 'void *handle' parameter,
because the connection has not yet been established; but because it is
called once per connecting client, the resulting list could be
dynamically modified such as providing a list of filenames present in a
directory).
We'd have to figure out lifetimes - would the plugin return a malloc()d
list that nbdkit frees, or would we need a pair of functions, where the
.list_exports function returns a const list, and then nbdkit calls
.free_exports (if defined) to let the plugin clean up any allocations.
(The latter is slightly nicer, in case the plugin is using some other
memory management scheme, such as a language binding, rather than
requiring the copying of data into a form where nbdkit calling free() is
safe).
>
> The implementation of this is straightforward. It simply reads the
> exportname global from the server and returns it. It also doesn't
> change the existing API at all.
>
> (2) A better way might be to bump the API version to 3 and that would
> allow us to change the open() callback:
>
> static void *
> myplugin_open (int readonly, const char *exportname)
> {
> ... Do something based on the export name ...
> }
>
> This is cleaner but is obviously a larger change. Also if we were
> going to bump the API version I'd want to at the same time do all the
> other things we are planning for API 3, and that potentially makes it
> a much bigger deal:
>
> https://github.com/libguestfs/nbdkit/blob/b485ade71464fc351828e2fcce54464709bf234d/TODO#L166
Yeah, bumping to API 3 is a much bigger deal; we can get most of the
benefits of just letting the plugin know WHICH export name was requested
without making that bump.
>
> It has the advantage of making it easier to access the export name
> from sh plugins, since those don't have access to nbdkit_* API calls
> (or at least we haven't thought about how we would do that).
Note that with sh plugins, we could still make things work with v2, as
follows:
sh_open() calls nbdkit_export_name() unconditionally, prior to forming
the command line for calling into the script. Then we alter the command
line we pass to the script:
/path/to/script open
where the sh plugin always gets to know what export name the client
chose, thanks to the new exportname parameter. Compatibility-wise,
older scripts fall in one of two categories: either they ignore
unexpected parameters (no change required), or they choke because
exportname was unexpected (but it's a one-line change to the script to
deal with it). New scripts run against an older nbdkit that did not
provide the exportname parameter will see "$3" as empty (if the script
runs under 'set -u', then spell things "${3-}").
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] [PATCH nbdkit] server: Add nbdkit_export_name() to allow export name to be read.
This allows plugins (or filters) to read the export name which was
passed to the server from the client.
UNFINISHED:
- Needs tests
---
TODO | 8 ++
docs/nbdkit-plugin.pod | 39
server/connections.c | 1 +
server/internal.h| 1 +
server/protocol-handshake-newstyle.c | 30 -
server/public.c | 10 +++
6 files changed, 77 insertions(+), 12 deletions(-)
diff --git a/TODO b/TODO
index 49b60b1..2468d74 100644
--- a/TODO
+++ b/TODO
@@ -62,6 +62,12 @@ General ideas for improvements
and also look at the implementation of the -swap option in
nbd-client.
+* Clients should be able to list export names supported by plugins.
+ Current behaviour is not really correct: We only list the -e
+ parameter from the command line, which is different from the export
+ name(s) that a plugin might want to support. Probably we should
+ deprecate the -e option entirely since it does nothing useful.
+
Suggestions for plugins
---
@@ -190,3 +196,5 @@ using ‘#define NBDKIT_API_VERSION ’.
value) strings. nbdkit should know the possible keys for the plugin
and filters, and the type of the values, and both check and parse
them for the plugin.
+
+* Modify open() API so it takes an export name parameter.
diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod
index bb162e4..53687ad 100644
--- a/docs/nbdkit-plugin.pod
+++ b/docs/nbdkit-plugin.pod
@@ -362,6 +362,45 @@ requested, or -1 after calling C if there is
no point in
continuing the current command. Attempts to sleep more than
C seconds are treated as an error.
+=head1 EXPORT NAME
+
+If the client negotiated an NBD export name with nbdkit then plugins
+may read this from any connected callbacks. Nbdkit's normal behaviour
+is to accept any export name passed by the client, log it in debug
+output, but otherwise ignore it. By using C
+plugins may choose to filter by export name or serve different
+content.
+
+=head2 C
+
+ const char *nbdkit_export_name (void);
+
+Return the optional NBD export name if one was negotiated with the
+current client (this uses thread-local magic so no parameter is
+required). The returned string is only valid while the client is
+connected, so if you need to store it in the plugin you must copy it.
+
+The export name is a free-form text string, it is not necessarily a
+path or filename and it does not need to begin with a C<'/'>
+character. The NBD protocol describes the empty string (C<"">) as a
+representing a "default export" or to be used in cases where the
+export name does not make sense.
+
+This may return C which does I indicate an error:
+
+=over 4
+
+=item *
+
+It only returns the export name when there is a connected client.
+
+=item *
+
+If the server is using the oldstyle protocol the client does not send
+an export name.
+
+=back
+
=head1 CALLBACKS
=head2 C<.name>
diff --git a/server/connections.c b/server/connections.c
index b582764..a1fea54 100644
--- a/server/connections.c
+++ b/server/connections.c
@@ -385,6 +385,7 @@ free_connection (struct connection *conn)
pthread_mutex_destroy (>status_lock);
free (conn->handles);
+ free (conn->exportname);
free (conn);
}
diff --git a/server/internal.h b/server/internal.h
index 9314e8f..3bfc1a7 100644
--- a/server/internal.h
+++ b/server/internal.h
@@ -180,6 +180,7 @@ struct connection {
struct b_conn_handle *handles;
size_t nr_handles;
+ char *exportname;
uint32_t cflags;
uint16_t eflags;
bool using_tls;
diff --git a/server/protocol-handshake-newstyle.c
b/server/protocol-handshake-newstyle.c
index 9ddc319..e1301a0 100644
--- a/server/protocol-handshake-newstyle.c
+++ b/server/protocol-handshake-newstyle.c
@@ -274,11 +274,17 @@ negotiate_handshake_newstyle_options (struct connection
*conn)
if (conn_recv_full (conn, data, optlen,
"read: %s: %m", name_of_nbd_opt (option)) == -1)
return -1;
- /* Apart from printing it, ignore the export name. */
+ /* Print the export name and save it in the connection. */
data[optlen] = '\0';
- debug ("newstyle negotiation: %s: "
- "client requested export '%s' (ignored)",
+ debug ("newstyle negotiation: %s: client requested export '%s'",
name_of_nbd_opt (option), data);
+ free (conn->exportname);
+ conn->exportname = malloc (optlen+1);
+ if (conn->exportname == NULL) {
+nbdkit_error ("malloc: %m");
+return -1;
+ }
+ memcpy (conn->exportname, data, optlen+1);
/* We have to finish the handshake by sending handshake_finish. */
if (finish_newstyle_options (conn, ) == -1)
@@ -413,19 +419,19 @@ negotiate_handshake_newstyle_options (struct connection
*conn)
continue;
}
-/* As with NBD_OPT_EXPORT_NAME we print the export name and then
-
[Libguestfs] [PATCH nbdkit] server: Add nbdkit_export_name() to allow export name to be read.
This is the sort of thing I had in mind for option (1) here: https://www.redhat.com/archives/libguestfs/2019-September/msg00047.html It does reveal that the way we currently list exports is naive to say the least ... Rich. ___ Libguestfs mailing list Libguestfs@redhat.com https://www.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [nbdkit] Access export name from plugins
On Tue, Sep 10, 2019 at 11:01:10AM +0100, Richard W.M. Jones wrote: > The implementation of this is straightforward. It simply reads the > exportname global from the server and returns it. It also doesn't > change the existing API at all. Sorry - of course I don't mean returning the global exportname, but returning the per-thread exportname negotiated for the current connection. The implementation is still relatively simply however. 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 ___ Libguestfs mailing list Libguestfs@redhat.com https://www.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] [nbdkit] Access export name from plugins
Of course at the moment nbdkit parses the NBD export name option but
doesn't really do anything with it (except logging it).
I wonder if we should make this available to plugins, in case they
wish to serve different content to different clients based on the
export name. Note I'm not suggesting that we use this feature in any
existing plugins.
If we wanted to do this there seem like two possible ways to do it:
(1) Add a call, like nbdkit_export_name, which plugins could call from
any connected method to get the current export name, eg:
static void *
myplugin_open (int readonly)
{
const char *export = nbdkit_export_name ();
... Do something based on the export name ...
}
The implementation of this is straightforward. It simply reads the
exportname global from the server and returns it. It also doesn't
change the existing API at all.
(2) A better way might be to bump the API version to 3 and that would
allow us to change the open() callback:
static void *
myplugin_open (int readonly, const char *exportname)
{
... Do something based on the export name ...
}
This is cleaner but is obviously a larger change. Also if we were
going to bump the API version I'd want to at the same time do all the
other things we are planning for API 3, and that potentially makes it
a much bigger deal:
https://github.com/libguestfs/nbdkit/blob/b485ade71464fc351828e2fcce54464709bf234d/TODO#L166
It has the advantage of making it easier to access the export name
from sh plugins, since those don't have access to nbdkit_* API calls
(or at least we haven't thought about how we would do that).
Thoughts?
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines. Supports shell scripting,
bindings from many languages. http://libguestfs.org
___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH] v2v: rhv-upload-plugin - improve wait logic after finalize (RHBZ#1680361)
On Monday, 9 September 2019 18:00:35 CEST Richard W.M. Jones wrote:
> From: Daniel Erez
>
> After invoking transfer_service.finalize, check operation status by
> examining DiskStatus. This is done instead of failing after a
> predefined timeout regardless the status.
>
> Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1680361
> Signed-off-by: Richard W.M. Jones
> Tested-by: Ilanit Stein
> ---
> v2v/rhv-upload-plugin.py | 19 +--
> 1 file changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/v2v/rhv-upload-plugin.py b/v2v/rhv-upload-plugin.py
> index 9e71021..8f13ce1 100644
> --- a/v2v/rhv-upload-plugin.py
> +++ b/v2v/rhv-upload-plugin.py
> @@ -549,16 +549,23 @@ def close(h):
> # waiting for the transfer object to cease to exist, which
> # falls through to the exception case and then we can
> # continue.
> -endt = time.time() + timeout
> +disk_id = disk.id
> +start = time.time()
> try:
> while True:
> time.sleep(1)
> -tmp = transfer_service.get()
> -if time.time() > endt:
> -raise RuntimeError("timed out waiting for transfer "
> - "to finalize")
> +disk_service = h['disk_service']
> +disk = disk_service.get()
> +if disk.status == types.DiskStatus.LOCKED:
> +if time.time() > start + timeout:
> +raise RuntimeError("timed out waiting for transfer "
> + "to finalize")
> +continue
> +if disk.status == types.DiskStatus.OK:
> +debug("finalized after %s seconds" % (time.time() -
> start))
> +break
> except sdk.NotFoundError:
> -pass
> +raise RuntimeError("transfer failed: disk %s not found" %
> disk_id)
Seems to be OK (although I did not test it).
--
Pino Toscano
signature.asc
Description: This is a digitally signed message part.
___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs
