[libvirt] [PATCH v8 14/21] backup: Prepare for Unix sockets in QMP nbd-server-start

[Peter Krempa]

On Wed, Apr 17, 2019 at 09:09:14 -0500, Eric Blake wrote:
> Migration always uses a TCP socket for NBD servers, because we don't
> support same-host migration. But upcoming pull-mode incremental backup
> needs to also support a Unix socket, for retrieving the backup from
> the same host. Support this by plumbing virStorageNetHostDef through
> the monitor calls, since that is the same struct that backup_conf.c
> chose for tracking both TCP and Unix sockets.
> 
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
>  src/qemu/qemu_monitor.h      |  6 +++---
>  src/qemu/qemu_monitor_json.h |  3 +--
>  src/qemu/qemu_migration.c    |  7 ++++++-
>  src/qemu/qemu_monitor.c      |  7 +++----
>  src/qemu/qemu_monitor_json.c | 23 +++++++++++++++++------
>  tests/qemumonitorjsontest.c  | 30 +++++++++++++++++++++++++++++-
>  6 files changed, 59 insertions(+), 17 deletions(-)

[...]

> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index babcbde878..694ed90622 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -3924,15 +3924,14 @@ qemuMonitorGetSEVCapabilities(qemuMonitorPtr mon,
> 
>  int
>  qemuMonitorNBDServerStart(qemuMonitorPtr mon,
> -                          const char *host,
> -                          unsigned int port,
> +                          const virStorageNetHostDef *server,
>                            const char *tls_alias)
>  {
> +    VIR_DEBUG("server=%p tls_alias=%s", server, NULLSTR(tls_alias));

Logging the pointer is useless unless you have the core dump/running
process with breakpoint. The struct is simple enough so that we should
be able to log all relevant bits:

    VIR_DEBUG("host=%s port=%u socket=%s tls_alias=%s",
              NULLSTR(server->name), server->port,
              NULLSTR(server->socket), NULLSTR(tls_alias));


> 
>      QEMU_CHECK_MONITOR(mon);

[...]

> diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
> index f5ad3f6a73..dd3259494a 100644
> --- a/tests/qemumonitorjsontest.c
> +++ b/tests/qemumonitorjsontest.c
> @@ -1350,7 +1350,6 @@ GEN_TEST_FUNC(qemuMonitorJSONBlockCommit, "vdb", "/foo/bar1", "/foo/bar2", "back
>  GEN_TEST_FUNC(qemuMonitorJSONDrivePivot, "vdb")
>  GEN_TEST_FUNC(qemuMonitorJSONScreendump, "devicename", 1, "/foo/bar")
>  GEN_TEST_FUNC(qemuMonitorJSONOpenGraphics, "spice", "spicefd", false)
> -GEN_TEST_FUNC(qemuMonitorJSONNBDServerStart, "localhost", 12345, "test-alias")

This should be able to test even the struct which needs to be passed
here if you declare it prior to this.

Also I'd consider testing the unix variand since you are adding support
for it.

Additionally this patch is not removing the DO_TEST_GEN for this
function. Those macros should only be used in pairs and this would
become the first one to break it.

>  GEN_TEST_FUNC(qemuMonitorJSONNBDServerAdd, "vda", true)
>  GEN_TEST_FUNC(qemuMonitorJSONDetachCharDev, "serial1")
>  GEN_TEST_FUNC(qemuMonitorJSONBlockdevTrayOpen, "foodev", true)
> @@ -1358,6 +1357,35 @@ GEN_TEST_FUNC(qemuMonitorJSONBlockdevTrayClose, "foodev")
>  GEN_TEST_FUNC(qemuMonitorJSONBlockdevMediumRemove, "foodev")
>  GEN_TEST_FUNC(qemuMonitorJSONBlockdevMediumInsert, "foodev", "newnode")
> 
> +static int
> +testQemuMonitorJSONqemuMonitorJSONNBDServerStart(const void *data)
> +{
> +    virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data;
> +    qemuMonitorTestPtr test = qemuMonitorTestNewSimple(true, xmlopt);
> +    int ret = -1;
> +    virStorageNetHostDef server = {
> +        .name = (char *)"localhost",
> +        .port = 12345,
> +        .transport = VIR_STORAGE_NET_HOST_TRANS_TCP,
> +    };
> +
> +    if (!test)
> +        return -1;
> +
> +    if (qemuMonitorTestAddItem(test, "nbd-server-start", "{\"return\":{}}") < 0)
> +        goto cleanup;

This test does not test anything interresting unless you use
qemuMonitorTestNewSchema above. The test call you've removed actually
used it.

> +
> +    if (qemuMonitorJSONNBDServerStart(qemuMonitorTestGetMonitor(test),
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190426/a89528cb/attachment-0001.sig>