Re: [PATCH v2 3/5] a standone-alone tool to directly share disk image file via vhost-user protocol

[Coiby Xu]

> > > Yes, I think at least for the moment it should work fine this way.
> > > Eventually, I'd like to integrate it with --export (and associated QMP
> > > commands, which are still to be created), too. Maybe at that point we
> > > want to make the QOM object not user creatable any more.
> >
> > Does it mean TYPE_USER_CREATABLE interface in QOM will become
> > deprecated in the future? I'm curious what are the reasons for making
> > QOM object no user creatable? Because we may still need to start
> > vhost-user block device backend through HMP or QMP instead of stating
> > it as a standalone-alone daemon.

> Not in general, but if we have something like a block-export-add QMP
> command, the QOM interface would be redundant. We could still leave it
> there and have both a low-level and a high-level interface, but whether
> we would want to is something we still have to decide.

I see. So QOM interface will still be used as a low-level interface.
In the draft version, vhost-user-blk-server is started using specific
command vhost_user_server_start/vhost_user_server_stop which proivide
interfaces easier for implementing block-export-add QMP command. But
in later versions, only object_add/object_del syntax is supported to
start/stop vhost-user-blk-server. I'll keep an eye on how the storage
daemon develops and adapt my code accordingly.


On Sun, Feb 2, 2020 at 5:33 PM Kevin Wolf  wrote:
>
> Am 31.01.2020 um 17:42 hat Coiby Xu geschrieben:
> > > Yes, I think at least for the moment it should work fine this way.
> > > Eventually, I'd like to integrate it with --export (and associated QMP
> > > commands, which are still to be created), too. Maybe at that point we
> > > want to make the QOM object not user creatable any more.
> >
> > Does it mean TYPE_USER_CREATABLE interface in QOM will become
> > deprecated in the future? I'm curious what are the reasons for making
> > QOM object no user creatable? Because we may still need to start
> > vhost-user block device backend through HMP or QMP instead of stating
> > it as a standalone-alone daemon.
>
> Not in general, but if we have something like a block-export-add QMP
> command, the QOM interface would be redundant. We could still leave it
> there and have both a low-level and a high-level interface, but whether
> we would want to is something we still have to decide.
>
> > > As for test cases, do you think it would be hard to just modify the
> > > tests to send an explicit 'quit' command to the daemon?
> >
> > Accroding to 
> > https://patchew.org/QEMU/20191017130204.16131-1-kw...@redhat.com/20191017130204.16131-10-kw...@redhat.com/,
> >
> > > +static bool exit_requested = false;
> > > +
> > > +void qemu_system_killed(int signal, pid_t pid)
> > > +{
> > > +exit_requested = true;
> > > +}
> >
> > if exit_requested = true, qemu-storage-daemon will exit the main loop
> > and then quit. So is calling qemu_system_killed by what you means "to
> > send an explicit 'quit' command to the daemon"?
>
> qemu_system_killed() is call in the signal handlers for, amongst others,
> SIGTERM and SIGINT. This is one way to stop the storage daemon (for
> manual use, sending SIGINT with Ctrl-C is probably the easiest way).
>
> What I actually meant is the 'quit' QMP command which will cause
> qmp_quit() to be run, which contains the same code. But if sending a
> signal is more convenient, that's just as good.
>
> Kevin
>
> > On Fri, Jan 17, 2020 at 6:12 PM Kevin Wolf  wrote:
> > >
> > > Am 17.01.2020 um 09:12 hat Coiby Xu geschrieben:
> > > > Excellent! I will add an option (or object property) for
> > > > vhost-user-blk server oject which will tell the daemon process to exit
> > > > when the client disconnects, thus "make check-qtest" will not get held
> > > > by this daemon process. After that since Kevin's qemu-storage-daemon
> > > > support "-object" option
> > > > (https://patchew.org/QEMU/20191017130204.16131-1-kw...@redhat.com/20191017130204.16131-3-kw...@redhat.com/)
> > > > and vhost-user-server is a user-creatable QOM object, it will work out
> > > > of the box.
> > >
> > > Yes, I think at least for the moment it should work fine this way.
> > > Eventually, I'd like to integrate it with --export (and associated QMP
> > > commands, which are still to be created), too. Maybe at that point we
> > > want to make the QOM object not user creatable any more.
> > >
> > > Would it make sense to prefix the object type name with "x-" so we can
> > > later retire it from the external user interface without a deprecation
> > > period?
> > >
> > > As for test cases, do you think it would be hard to just modify the
> > > tests to send an explicit 'quit' command to the daemon?
> > >
> > > > I'm curious when will be formal version of qemu-storage-daemon
> > > > finished so I can take advantage of it? Or should I apply the RFC
> > > > PATCHes to my working branch directly and submit them together with
> > > > the patches on vhost-user-blk server feature when posting v3?
> > >
> > > It's the 

Re: [PATCH v2 3/5] a standone-alone tool to directly share disk image file via vhost-user protocol

[Kevin Wolf]

Am 31.01.2020 um 17:42 hat Coiby Xu geschrieben:
> > Yes, I think at least for the moment it should work fine this way.
> > Eventually, I'd like to integrate it with --export (and associated QMP
> > commands, which are still to be created), too. Maybe at that point we
> > want to make the QOM object not user creatable any more.
> 
> Does it mean TYPE_USER_CREATABLE interface in QOM will become
> deprecated in the future? I'm curious what are the reasons for making
> QOM object no user creatable? Because we may still need to start
> vhost-user block device backend through HMP or QMP instead of stating
> it as a standalone-alone daemon.

Not in general, but if we have something like a block-export-add QMP
command, the QOM interface would be redundant. We could still leave it
there and have both a low-level and a high-level interface, but whether
we would want to is something we still have to decide.

> > As for test cases, do you think it would be hard to just modify the
> > tests to send an explicit 'quit' command to the daemon?
> 
> Accroding to 
> https://patchew.org/QEMU/20191017130204.16131-1-kw...@redhat.com/20191017130204.16131-10-kw...@redhat.com/,
> 
> > +static bool exit_requested = false;
> > +
> > +void qemu_system_killed(int signal, pid_t pid)
> > +{
> > +exit_requested = true;
> > +}
> 
> if exit_requested = true, qemu-storage-daemon will exit the main loop
> and then quit. So is calling qemu_system_killed by what you means "to
> send an explicit 'quit' command to the daemon"?

qemu_system_killed() is call in the signal handlers for, amongst others,
SIGTERM and SIGINT. This is one way to stop the storage daemon (for
manual use, sending SIGINT with Ctrl-C is probably the easiest way).

What I actually meant is the 'quit' QMP command which will cause
qmp_quit() to be run, which contains the same code. But if sending a
signal is more convenient, that's just as good.

Kevin

> On Fri, Jan 17, 2020 at 6:12 PM Kevin Wolf  wrote:
> >
> > Am 17.01.2020 um 09:12 hat Coiby Xu geschrieben:
> > > Excellent! I will add an option (or object property) for
> > > vhost-user-blk server oject which will tell the daemon process to exit
> > > when the client disconnects, thus "make check-qtest" will not get held
> > > by this daemon process. After that since Kevin's qemu-storage-daemon
> > > support "-object" option
> > > (https://patchew.org/QEMU/20191017130204.16131-1-kw...@redhat.com/20191017130204.16131-3-kw...@redhat.com/)
> > > and vhost-user-server is a user-creatable QOM object, it will work out
> > > of the box.
> >
> > Yes, I think at least for the moment it should work fine this way.
> > Eventually, I'd like to integrate it with --export (and associated QMP
> > commands, which are still to be created), too. Maybe at that point we
> > want to make the QOM object not user creatable any more.
> >
> > Would it make sense to prefix the object type name with "x-" so we can
> > later retire it from the external user interface without a deprecation
> > period?
> >
> > As for test cases, do you think it would be hard to just modify the
> > tests to send an explicit 'quit' command to the daemon?
> >
> > > I'm curious when will be formal version of qemu-storage-daemon
> > > finished so I can take advantage of it? Or should I apply the RFC
> > > PATCHes to my working branch directly and submit them together with
> > > the patches on vhost-user-blk server feature when posting v3?
> >
> > It's the next thing I'm planning to work on after completing the
> > coroutine-base QMP handlers (which I hope to get finished very soon).
> >
> > For the time being I would suggest that you put any patches that depend
> > on qemu-storage-daemon (if you do need it) at the end of your series so
> > that we could apply the first part even if the storage daemon isn't in
> > yet.
> >
> > The latest version of my patches is at:
> >
> > git://repo.or.cz/qemu/kevin.git storage-daemon
> >
> > But if you just need something for testing your code, I think it would
> > even make sense if you kept your standalone tool around (even though
> > we'll never merge it) and we'll deal with integration in the storage
> > daemon once both parts are ready.
> >
> > Kevin
> >
> 
> 
> -- 
> Best regards,
> Coiby
> 

Re: [PATCH v2 3/5] a standone-alone tool to directly share disk image file via vhost-user protocol

[Coiby Xu]

Hi Kevin,

> Yes, I think at least for the moment it should work fine this way.
> Eventually, I'd like to integrate it with --export (and associated QMP
> commands, which are still to be created), too. Maybe at that point we
> want to make the QOM object not user creatable any more.

Does it mean TYPE_USER_CREATABLE interface in QOM will become
deprecated in the future? I'm curious what are the reasons for making
QOM object no user creatable? Because we may still need to start
vhost-user block device backend through HMP or QMP instead of stating
it as a standalone-alone daemon.

> As for test cases, do you think it would be hard to just modify the
> tests to send an explicit 'quit' command to the daemon?

Accroding to 
https://patchew.org/QEMU/20191017130204.16131-1-kw...@redhat.com/20191017130204.16131-10-kw...@redhat.com/,

> +static bool exit_requested = false;
> +
> +void qemu_system_killed(int signal, pid_t pid)
> +{
> +exit_requested = true;
> +}

if exit_requested = true, qemu-storage-daemon will exit the main loop
and then quit. So is calling qemu_system_killed by what you means "to
send an explicit 'quit' command to the daemon"?

On Fri, Jan 17, 2020 at 6:12 PM Kevin Wolf  wrote:
>
> Am 17.01.2020 um 09:12 hat Coiby Xu geschrieben:
> > Excellent! I will add an option (or object property) for
> > vhost-user-blk server oject which will tell the daemon process to exit
> > when the client disconnects, thus "make check-qtest" will not get held
> > by this daemon process. After that since Kevin's qemu-storage-daemon
> > support "-object" option
> > (https://patchew.org/QEMU/20191017130204.16131-1-kw...@redhat.com/20191017130204.16131-3-kw...@redhat.com/)
> > and vhost-user-server is a user-creatable QOM object, it will work out
> > of the box.
>
> Yes, I think at least for the moment it should work fine this way.
> Eventually, I'd like to integrate it with --export (and associated QMP
> commands, which are still to be created), too. Maybe at that point we
> want to make the QOM object not user creatable any more.
>
> Would it make sense to prefix the object type name with "x-" so we can
> later retire it from the external user interface without a deprecation
> period?
>
> As for test cases, do you think it would be hard to just modify the
> tests to send an explicit 'quit' command to the daemon?
>
> > I'm curious when will be formal version of qemu-storage-daemon
> > finished so I can take advantage of it? Or should I apply the RFC
> > PATCHes to my working branch directly and submit them together with
> > the patches on vhost-user-blk server feature when posting v3?
>
> It's the next thing I'm planning to work on after completing the
> coroutine-base QMP handlers (which I hope to get finished very soon).
>
> For the time being I would suggest that you put any patches that depend
> on qemu-storage-daemon (if you do need it) at the end of your series so
> that we could apply the first part even if the storage daemon isn't in
> yet.
>
> The latest version of my patches is at:
>
> git://repo.or.cz/qemu/kevin.git storage-daemon
>
> But if you just need something for testing your code, I think it would
> even make sense if you kept your standalone tool around (even though
> we'll never merge it) and we'll deal with integration in the storage
> daemon once both parts are ready.
>
> Kevin
>


-- 
Best regards,
Coiby

Re: [PATCH v2 3/5] a standone-alone tool to directly share disk image file via vhost-user protocol

[Kevin Wolf]

Am 17.01.2020 um 09:12 hat Coiby Xu geschrieben:
> Excellent! I will add an option (or object property) for
> vhost-user-blk server oject which will tell the daemon process to exit
> when the client disconnects, thus "make check-qtest" will not get held
> by this daemon process. After that since Kevin's qemu-storage-daemon
> support "-object" option
> (https://patchew.org/QEMU/20191017130204.16131-1-kw...@redhat.com/20191017130204.16131-3-kw...@redhat.com/)
> and vhost-user-server is a user-creatable QOM object, it will work out
> of the box.

Yes, I think at least for the moment it should work fine this way.
Eventually, I'd like to integrate it with --export (and associated QMP
commands, which are still to be created), too. Maybe at that point we
want to make the QOM object not user creatable any more.

Would it make sense to prefix the object type name with "x-" so we can
later retire it from the external user interface without a deprecation
period?

As for test cases, do you think it would be hard to just modify the
tests to send an explicit 'quit' command to the daemon?

> I'm curious when will be formal version of qemu-storage-daemon
> finished so I can take advantage of it? Or should I apply the RFC
> PATCHes to my working branch directly and submit them together with
> the patches on vhost-user-blk server feature when posting v3?

It's the next thing I'm planning to work on after completing the
coroutine-base QMP handlers (which I hope to get finished very soon).

For the time being I would suggest that you put any patches that depend
on qemu-storage-daemon (if you do need it) at the end of your series so
that we could apply the first part even if the storage daemon isn't in
yet.

The latest version of my patches is at:

git://repo.or.cz/qemu/kevin.git storage-daemon

But if you just need something for testing your code, I think it would
even make sense if you kept your standalone tool around (even though
we'll never merge it) and we'll deal with integration in the storage
daemon once both parts are ready.

Kevin

Re: [PATCH v2 3/5] a standone-alone tool to directly share disk image file via vhost-user protocol

[Coiby Xu]

Excellent! I will add an option (or object property) for
vhost-user-blk server oject which will tell the daemon process to exit
when the client disconnects, thus "make check-qtest" will not get held
by this daemon process. After that since Kevin's qemu-storage-daemon
support "-object" option
(https://patchew.org/QEMU/20191017130204.16131-1-kw...@redhat.com/20191017130204.16131-3-kw...@redhat.com/)
and vhost-user-server is a user-creatable QOM object, it will work out
of the box.

I'm curious when will be formal version of qemu-storage-daemon
finished so I can take advantage of it? Or should I apply the RFC
PATCHes to my working branch directly and submit them together with
the patches on vhost-user-blk server feature when posting v3?



On Thu, Jan 16, 2020 at 10:04 PM Stefan Hajnoczi  wrote:
>
> On Tue, Jan 14, 2020 at 10:06:18PM +0800, Coiby Xu wrote:
> > vhost-user-blk can have played as vhost-user backend but it only supports 
> > raw file and don't support VIRTIO_BLK_T_DISCARD and 
> > VIRTIO_BLK_T_WRITE_ZEROES operations on raw file (ioctl(fd, BLKDISCARD) is 
> > only valid for real block device).
> >
> > Signed-off-by: Coiby Xu 
> > ---
> >  qemu-vu.c | 264 ++
> >  1 file changed, 264 insertions(+)
> >  create mode 100644 qemu-vu.c
>
> Kevin has been working on qemu-storage-daemon, a tool for running NBD
> exports, block jobs, and other storage features that are not part of a
> guest.  I think qemu-storage-daemon would be the appropriate tool for
> running vhost-user-blk servers.  A dedicated binary is not necessary.
>
> Stefan



--
Best regards,
Coiby

Re: [PATCH v2 3/5] a standone-alone tool to directly share disk image file via vhost-user protocol

[Stefan Hajnoczi]

On Tue, Jan 14, 2020 at 10:06:18PM +0800, Coiby Xu wrote:
> vhost-user-blk can have played as vhost-user backend but it only supports raw 
> file and don't support VIRTIO_BLK_T_DISCARD and VIRTIO_BLK_T_WRITE_ZEROES 
> operations on raw file (ioctl(fd, BLKDISCARD) is only valid for real block 
> device).
> 
> Signed-off-by: Coiby Xu 
> ---
>  qemu-vu.c | 264 ++
>  1 file changed, 264 insertions(+)
>  create mode 100644 qemu-vu.c

Kevin has been working on qemu-storage-daemon, a tool for running NBD
exports, block jobs, and other storage features that are not part of a
guest.  I think qemu-storage-daemon would be the appropriate tool for
running vhost-user-blk servers.  A dedicated binary is not necessary.

Stefan


signature.asc
Description: PGP signature

[PATCH v2 3/5] a standone-alone tool to directly share disk image file via vhost-user protocol

[Coiby Xu]

vhost-user-blk can have played as vhost-user backend but it only supports raw 
file and don't support VIRTIO_BLK_T_DISCARD and VIRTIO_BLK_T_WRITE_ZEROES 
operations on raw file (ioctl(fd, BLKDISCARD) is only valid for real block 
device).

Signed-off-by: Coiby Xu 
---
 qemu-vu.c | 264 ++
 1 file changed, 264 insertions(+)
 create mode 100644 qemu-vu.c

diff --git a/qemu-vu.c b/qemu-vu.c
new file mode 100644
index 00..25c32c2c6d
--- /dev/null
+++ b/qemu-vu.c
@@ -0,0 +1,264 @@
+/*
+ *  Copyright (C) 2020  Coiby Xu 
+ *
+ *  Vhost-user-blk device backend
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; under version 2 of the License.
+ *
+ *  This program 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 General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, see .
+ */
+
+#include "qemu/osdep.h"
+#include 
+#include 
+#include "block/vhost-user.h"
+#include "qemu-common.h"
+#include "qapi/error.h"
+#include "qemu/cutils.h"
+#include "sysemu/block-backend.h"
+#include "block/block_int.h"
+#include "qemu/main-loop.h"
+#include "qemu/module.h"
+#include "qemu/option.h"
+#include "qemu/error-report.h"
+#include "qapi/qmp/qdict.h"
+#include "qapi/qmp/qstring.h"
+#include "qom/object_interfaces.h"
+#include "io/net-listener.h"
+#include "qemu-version.h"
+
+#define QEMU_VU_OPT_CACHE 256
+
+#define QEMU_VU_OPT_AIO   257
+
+static char *srcpath;
+
+static void usage(const char *name)
+{
+(printf) (
+"Usage: %s [OPTIONS] FILE\n"
+"  or:  %s -L [OPTIONS]\n"
+"QEMU Vhost-user Server Utility\n"
+"\n"
+"  -h, --helpdisplay this help and exit\n"
+"  -V, --version output version information and exit\n"
+"\n"
+"Connection properties:\n"
+"  -k, --socket=PATH path to the unix socket\n"
+"\n"
+"General purpose options:\n"
+"  -e, -- exit-panic When the panic callback is called, the program\n"
+"will exit. Useful for make check-qtest.\n"
+"\n"
+"Block device options:\n"
+"  -f, --format=FORMAT   set image format (raw, qcow2, ...)\n"
+"  -r, --read-only   export read-only\n"
+"  -n, --nocache disable host cache\n"
+"  --cache=MODE  set cache mode (none, writeback, ...)\n"
+"  --aio=MODEset AIO mode (native or threads)\n"
+"\n"
+QEMU_HELP_BOTTOM "\n"
+, name, name);
+}
+
+static void version(const char *name)
+{
+printf(
+"%s " QEMU_FULL_VERSION "\n"
+"Written by Coiby Xu, based on qemu-nbd by Anthony Liguori\n"
+"\n"
+QEMU_COPYRIGHT "\n"
+"This is free software; see the source for copying conditions.  There is NO\n"
+"warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.\n"
+, name);
+}
+
+static VubDev *vub_device;
+
+static void vus_shutdown(void)
+{
+job_cancel_sync_all();
+bdrv_close_all();
+vub_free(vub_device, false);
+}
+
+int main(int argc, char **argv)
+{
+BlockBackend *blk;
+BlockDriverState *bs;
+bool readonly = false;
+char *sockpath = NULL;
+int64_t fd_size;
+const char *sopt = "hVrnvek:f:";
+struct option lopt[] = {
+{ "help", no_argument, NULL, 'h' },
+{ "version", no_argument, NULL, 'V' },
+{ "exit-panic", no_argument, NULL, 'e' },
+{ "socket", required_argument, NULL, 'k' },
+{ "read-only", no_argument, NULL, 'r' },
+{ "nocache", no_argument, NULL, 'n' },
+{ "cache", required_argument, NULL, QEMU_VU_OPT_CACHE },
+{ "aio", required_argument, NULL, QEMU_VU_OPT_AIO },
+{ "format", required_argument, NULL, 'f' },
+{ NULL, 0, NULL, 0 }
+};
+int ch;
+int opt_ind = 0;
+int flags = BDRV_O_RDWR;
+bool seen_cache = false;
+bool seen_aio = false;
+const char *fmt = NULL;
+Error *local_err = NULL;
+QDict *options = NULL;
+bool writethrough = true;
+bool exit_panic = false;
+
+error_init(argv[0]);
+
+module_call_init(MODULE_INIT_QOM);
+qemu_init_exec_dir(argv[0]);
+
+while ((ch = getopt_long(argc, argv, sopt, lopt, _ind)) != -1) {
+switch (ch) {
+case 'e':
+exit_panic = true;
+break;
+case 'n':
+optarg = (char *) "none";
+/* fallthrough */
+case QEMU_VU_OPT_CACHE:
+if (seen_cache) {
+error_report("-n and --cache can only be specified once");
+exit(EXIT_FAILURE);
+}
+seen_cache = true;
+if (bdrv_parse_cache_mode(optarg, , ) == -1) {
+