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 a nice reusable struct that can track
both TCP and Unix sockets.
Update qemumonitorjsontest to not only test the response to the
command, but to actually verify that the command itself uses the two
correct QMP forms. I'm actually a bit surprised that we are not
utilizing qemuMonitorTestAddItemVerbatim more frequently.
Signed-off-by: Eric Blake <eblake@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 | 13 +++++---
src/qemu/qemu_monitor_json.c | 23 ++++++++++----
tests/qemumonitorjsontest.c | 58 ++++++++++++++++++++++++++++++++++--
6 files changed, 92 insertions(+), 18 deletions(-)
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index dee594fa66..fa84ff821e 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -1094,9 +1094,9 @@ int qemuMonitorGetObjectProps(qemuMonitorPtr mon,
char *qemuMonitorGetTargetArch(qemuMonitorPtr mon);
int qemuMonitorNBDServerStart(qemuMonitorPtr mon,
- const char *host,
- unsigned int port,
- const char *tls_alias);
+ const virStorageNetHostDef *server,
+ const char *tls_alias)
+ ATTRIBUTE_NONNULL(2);
int qemuMonitorNBDServerAdd(qemuMonitorPtr mon,
const char *deviceID,
bool writable);
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index acef1a0a79..e41bdc8c4f 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -459,8 +459,7 @@ int qemuMonitorJSONGetObjectProps(qemuMonitorPtr mon,
char *qemuMonitorJSONGetTargetArch(qemuMonitorPtr mon);
int qemuMonitorJSONNBDServerStart(qemuMonitorPtr mon,
- const char *host,
- unsigned int port,
+ const virStorageNetHostDef *server,
const char *tls_alias);
int qemuMonitorJSONNBDServerAdd(qemuMonitorPtr mon,
const char *deviceID,
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 32b3040473..267a729c6f 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -380,6 +380,10 @@ qemuMigrationDstStartNBDServer(virQEMUDriverPtr driver,
unsigned short port = 0;
char *diskAlias = NULL;
size_t i;
+ virStorageNetHostDef server = {
+ .name = (char *)listenAddr, /* cast away const */
+ .transport = VIR_STORAGE_NET_HOST_TRANS_TCP,
+ };
if (nbdPort < 0 || nbdPort > USHRT_MAX) {
virReportError(VIR_ERR_INVALID_ARG, "%s",
@@ -415,7 +419,8 @@ qemuMigrationDstStartNBDServer(virQEMUDriverPtr driver,
else if (virPortAllocatorAcquire(driver->migrationPorts, &port) < 0)
goto exit_monitor;
- if (qemuMonitorNBDServerStart(priv->mon, listenAddr, port, tls_alias) < 0)
+ server.port = port;
+ if (qemuMonitorNBDServerStart(priv->mon, &server, tls_alias) < 0)
goto exit_monitor;
}
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 6b731cd91a..187513a986 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -3925,15 +3925,20 @@ qemuMonitorGetSEVCapabilities(qemuMonitorPtr mon,
int
qemuMonitorNBDServerStart(qemuMonitorPtr mon,
- const char *host,
- unsigned int port,
+ const virStorageNetHostDef *server,
const char *tls_alias)
{
- VIR_DEBUG("host=%s port=%u tls_alias=%s", host, port, NULLSTR(tls_alias));
+ /* Peek inside the struct for nicer logging */
+ if (server->transport == VIR_STORAGE_NET_HOST_TRANS_TCP)
+ VIR_DEBUG("server={tcp host=%s port=%u} tls_alias=%s",
+ NULLSTR(server->name), server->port, NULLSTR(tls_alias));
+ else
+ VIR_DEBUG("server={unix socket=%s} tls_alias=%s",
+ NULLSTR(server->socket), NULLSTR(tls_alias));
QEMU_CHECK_MONITOR(mon);
- return qemuMonitorJSONNBDServerStart(mon, host, port, tls_alias);
+ return qemuMonitorJSONNBDServerStart(mon, server, tls_alias);
}
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 53a7de8b77..93113d4e8a 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -6684,8 +6684,7 @@ qemuMonitorJSONBuildUnixSocketAddress(const char *path)
int
qemuMonitorJSONNBDServerStart(qemuMonitorPtr mon,
- const char *host,
- unsigned int port,
+ const virStorageNetHostDef *server,
const char *tls_alias)
{
int ret = -1;
@@ -6694,10 +6693,22 @@ qemuMonitorJSONNBDServerStart(qemuMonitorPtr mon,
virJSONValuePtr addr = NULL;
char *port_str = NULL;
- if (virAsprintf(&port_str, "%u", port) < 0)
- return ret;
-
- if (!(addr = qemuMonitorJSONBuildInetSocketAddress(host, port_str)))
+ switch ((virStorageNetHostTransport)server->transport) {
+ case VIR_STORAGE_NET_HOST_TRANS_TCP:
+ if (virAsprintf(&port_str, "%u", server->port) < 0)
+ return ret;
+ addr = qemuMonitorJSONBuildInetSocketAddress(server->name, port_str);
+ break;
+ case VIR_STORAGE_NET_HOST_TRANS_UNIX:
+ addr = qemuMonitorJSONBuildUnixSocketAddress(server->socket);
+ break;
+ case VIR_STORAGE_NET_HOST_TRANS_RDMA:
+ case VIR_STORAGE_NET_HOST_TRANS_LAST:
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("invalid server address"));
+ goto cleanup;
+ }
+ if (!addr)
goto cleanup;
if (!(cmd = qemuMonitorJSONMakeCommand("nbd-server-start",
diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
index 0894e748ae..9d707fcc7c 100644
--- a/tests/qemumonitorjsontest.c
+++ b/tests/qemumonitorjsontest.c
@@ -1348,7 +1348,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")
GEN_TEST_FUNC(qemuMonitorJSONNBDServerAdd, "vda", true)
GEN_TEST_FUNC(qemuMonitorJSONDetachCharDev, "serial1")
GEN_TEST_FUNC(qemuMonitorJSONBlockdevTrayOpen, "foodev", true)
@@ -1356,6 +1355,61 @@ GEN_TEST_FUNC(qemuMonitorJSONBlockdevTrayClose, "foodev")
GEN_TEST_FUNC(qemuMonitorJSONBlockdevMediumRemove, "foodev")
GEN_TEST_FUNC(qemuMonitorJSONBlockdevMediumInsert, "foodev", "newnode")
+static int
+testQemuMonitorJSONqemuMonitorJSONNBDServerStart(const void *opaque)
+{
+ virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr) opaque;
+ qemuMonitorTestPtr test = qemuMonitorTestNewSimple(true, xmlopt);
+ int ret = -1;
+ virStorageNetHostDef server_tcp = {
+ .name = (char *)"localhost",
+ .port = 12345,
+ .transport = VIR_STORAGE_NET_HOST_TRANS_TCP,
+ };
+ virStorageNetHostDef server_unix = {
+ .socket = (char *)"/tmp/sock",
+ .transport = VIR_STORAGE_NET_HOST_TRANS_UNIX,
+ };
+
+ if (!test)
+ return -1;
+
+ if (qemuMonitorTestAddItemVerbatim(test,
+ "{\"execute\":\"nbd-server-start\","
+ " \"arguments\":{\"addr\":{"
+ " \"type\":\"inet\",\"data\":{"
+ " \"host\":\"localhost\","
+ " \"port\":\"12345\"}},"
+ " \"tls-creds\":\"test-alias\"},"
+ " \"id\":\"libvirt-1\"}",
+ NULL, "{\"return\":{}}") < 0)
+ goto cleanup;
+
+ if (qemuMonitorTestAddItemVerbatim(test,
+ "{\"execute\":\"nbd-server-start\","
+ " \"arguments\":{\"addr\":{"
+ " \"type\":\"unix\",\"data\":{"
+ " \"path\":\"/tmp/sock\"}},"
+ " \"tls-creds\":\"test-alias\"},"
+ " \"id\":\"libvirt-2\"}",
+ NULL, "{\"return\":{}}") < 0)
+ goto cleanup;
+
+ if (qemuMonitorJSONNBDServerStart(qemuMonitorTestGetMonitor(test),
+ &server_tcp, "test-alias") < 0)
+ goto cleanup;
+
+ if (qemuMonitorJSONNBDServerStart(qemuMonitorTestGetMonitor(test),
+ &server_unix, "test-alias") < 0)
+ goto cleanup;
+
+ ret = 0;
+
+ cleanup:
+ qemuMonitorTestFree(test);
+ return ret;
+}
+
static bool
testQemuMonitorJSONqemuMonitorJSONQueryCPUsEqual(struct qemuMonitorQueryCpusEntry *a,
struct qemuMonitorQueryCpusEntry *b)
@@ -3014,7 +3068,6 @@ mymain(void)
DO_TEST_GEN(qemuMonitorJSONDrivePivot);
DO_TEST_GEN(qemuMonitorJSONScreendump);
DO_TEST_GEN(qemuMonitorJSONOpenGraphics);
- DO_TEST_GEN(qemuMonitorJSONNBDServerStart);
DO_TEST_GEN(qemuMonitorJSONNBDServerAdd);
DO_TEST_GEN(qemuMonitorJSONDetachCharDev);
DO_TEST_GEN(qemuMonitorJSONBlockdevTrayOpen);
@@ -3036,6 +3089,7 @@ mymain(void)
DO_TEST(qemuMonitorJSONGetDumpGuestMemoryCapability);
DO_TEST(qemuMonitorJSONSendKeyHoldtime);
DO_TEST(qemuMonitorSupportsActiveCommit);
+ DO_TEST(qemuMonitorJSONNBDServerStart);
DO_TEST_CPU_DATA("host");
DO_TEST_CPU_DATA("full");
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Jun 05, 2019 at 22:01:07 -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 a nice reusable struct that can track
> both TCP and Unix sockets.
>
> Update qemumonitorjsontest to not only test the response to the
> command, but to actually verify that the command itself uses the two
> correct QMP forms. I'm actually a bit surprised that we are not
> utilizing qemuMonitorTestAddItemVerbatim more frequently.
>
> Signed-off-by: Eric Blake <eblake@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 | 13 +++++---
> src/qemu/qemu_monitor_json.c | 23 ++++++++++----
> tests/qemumonitorjsontest.c | 58 ++++++++++++++++++++++++++++++++++--
> 6 files changed, 92 insertions(+), 18 deletions(-)
>
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index dee594fa66..fa84ff821e 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -1094,9 +1094,9 @@ int qemuMonitorGetObjectProps(qemuMonitorPtr mon,
> char *qemuMonitorGetTargetArch(qemuMonitorPtr mon);
>
> int qemuMonitorNBDServerStart(qemuMonitorPtr mon,
> - const char *host,
> - unsigned int port,
> - const char *tls_alias);
> + const virStorageNetHostDef *server,
> + const char *tls_alias)
> + ATTRIBUTE_NONNULL(2);
> int qemuMonitorNBDServerAdd(qemuMonitorPtr mon,
> const char *deviceID,
> bool writable);
> diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
> index acef1a0a79..e41bdc8c4f 100644
> --- a/src/qemu/qemu_monitor_json.h
> +++ b/src/qemu/qemu_monitor_json.h
> @@ -459,8 +459,7 @@ int qemuMonitorJSONGetObjectProps(qemuMonitorPtr mon,
> char *qemuMonitorJSONGetTargetArch(qemuMonitorPtr mon);
>
> int qemuMonitorJSONNBDServerStart(qemuMonitorPtr mon,
> - const char *host,
> - unsigned int port,
> + const virStorageNetHostDef *server,
> const char *tls_alias);
> int qemuMonitorJSONNBDServerAdd(qemuMonitorPtr mon,
> const char *deviceID,
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 32b3040473..267a729c6f 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -380,6 +380,10 @@ qemuMigrationDstStartNBDServer(virQEMUDriverPtr driver,
> unsigned short port = 0;
> char *diskAlias = NULL;
> size_t i;
> + virStorageNetHostDef server = {
> + .name = (char *)listenAddr, /* cast away const */
> + .transport = VIR_STORAGE_NET_HOST_TRANS_TCP,
> + };
>
> if (nbdPort < 0 || nbdPort > USHRT_MAX) {
> virReportError(VIR_ERR_INVALID_ARG, "%s",
> @@ -415,7 +419,8 @@ qemuMigrationDstStartNBDServer(virQEMUDriverPtr driver,
> else if (virPortAllocatorAcquire(driver->migrationPorts, &port) < 0)
> goto exit_monitor;
>
> - if (qemuMonitorNBDServerStart(priv->mon, listenAddr, port, tls_alias) < 0)
> + server.port = port;
> + if (qemuMonitorNBDServerStart(priv->mon, &server, tls_alias) < 0)
> goto exit_monitor;
> }
>
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 6b731cd91a..187513a986 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -3925,15 +3925,20 @@ qemuMonitorGetSEVCapabilities(qemuMonitorPtr mon,
>
> int
> qemuMonitorNBDServerStart(qemuMonitorPtr mon,
> - const char *host,
> - unsigned int port,
> + const virStorageNetHostDef *server,
> const char *tls_alias)
> {
> - VIR_DEBUG("host=%s port=%u tls_alias=%s", host, port, NULLSTR(tls_alias));
> + /* Peek inside the struct for nicer logging */
> + if (server->transport == VIR_STORAGE_NET_HOST_TRANS_TCP)
> + VIR_DEBUG("server={tcp host=%s port=%u} tls_alias=%s",
> + NULLSTR(server->name), server->port, NULLSTR(tls_alias));
> + else
> + VIR_DEBUG("server={unix socket=%s} tls_alias=%s",
> + NULLSTR(server->socket), NULLSTR(tls_alias));
>
> QEMU_CHECK_MONITOR(mon);
>
> - return qemuMonitorJSONNBDServerStart(mon, host, port, tls_alias);
> + return qemuMonitorJSONNBDServerStart(mon, server, tls_alias);
> }
>
>
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 53a7de8b77..93113d4e8a 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -6684,8 +6684,7 @@ qemuMonitorJSONBuildUnixSocketAddress(const char *path)
>
> int
> qemuMonitorJSONNBDServerStart(qemuMonitorPtr mon,
> - const char *host,
> - unsigned int port,
> + const virStorageNetHostDef *server,
> const char *tls_alias)
> {
> int ret = -1;
> @@ -6694,10 +6693,22 @@ qemuMonitorJSONNBDServerStart(qemuMonitorPtr mon,
> virJSONValuePtr addr = NULL;
> char *port_str = NULL;
>
> - if (virAsprintf(&port_str, "%u", port) < 0)
> - return ret;
> -
> - if (!(addr = qemuMonitorJSONBuildInetSocketAddress(host, port_str)))
> + switch ((virStorageNetHostTransport)server->transport) {
> + case VIR_STORAGE_NET_HOST_TRANS_TCP:
> + if (virAsprintf(&port_str, "%u", server->port) < 0)
> + return ret;
> + addr = qemuMonitorJSONBuildInetSocketAddress(server->name, port_str);
> + break;
> + case VIR_STORAGE_NET_HOST_TRANS_UNIX:
> + addr = qemuMonitorJSONBuildUnixSocketAddress(server->socket);
> + break;
> + case VIR_STORAGE_NET_HOST_TRANS_RDMA:
> + case VIR_STORAGE_NET_HOST_TRANS_LAST:
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("invalid server address"));
> + goto cleanup;
> + }
> + if (!addr)
> goto cleanup;
>
> if (!(cmd = qemuMonitorJSONMakeCommand("nbd-server-start",
> diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
> index 0894e748ae..9d707fcc7c 100644
> --- a/tests/qemumonitorjsontest.c
> +++ b/tests/qemumonitorjsontest.c
> @@ -1348,7 +1348,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")
> GEN_TEST_FUNC(qemuMonitorJSONNBDServerAdd, "vda", true)
> GEN_TEST_FUNC(qemuMonitorJSONDetachCharDev, "serial1")
> GEN_TEST_FUNC(qemuMonitorJSONBlockdevTrayOpen, "foodev", true)
> @@ -1356,6 +1355,61 @@ GEN_TEST_FUNC(qemuMonitorJSONBlockdevTrayClose, "foodev")
> GEN_TEST_FUNC(qemuMonitorJSONBlockdevMediumRemove, "foodev")
> GEN_TEST_FUNC(qemuMonitorJSONBlockdevMediumInsert, "foodev", "newnode")
>
> +static int
> +testQemuMonitorJSONqemuMonitorJSONNBDServerStart(const void *opaque)
> +{
> + virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr) opaque;
> + qemuMonitorTestPtr test = qemuMonitorTestNewSimple(true, xmlopt);
I'd suggest you use qemuMonitorTestNewSchema here so that we preserve
the schema checks.
> + int ret = -1;
> + virStorageNetHostDef server_tcp = {
> + .name = (char *)"localhost",
> + .port = 12345,
> + .transport = VIR_STORAGE_NET_HOST_TRANS_TCP,
> + };
> + virStorageNetHostDef server_unix = {
> + .socket = (char *)"/tmp/sock",
> + .transport = VIR_STORAGE_NET_HOST_TRANS_UNIX,
> + };
> +
> + if (!test)
> + return -1;
> +
> + if (qemuMonitorTestAddItemVerbatim(test,
> + "{\"execute\":\"nbd-server-start\","
> + " \"arguments\":{\"addr\":{"
> + " \"type\":\"inet\",\"data\":{"
> + " \"host\":\"localhost\","
> + " \"port\":\"12345\"}},"
> + " \"tls-creds\":\"test-alias\"},"
> + " \"id\":\"libvirt-1\"}",
> + NULL, "{\"return\":{}}") < 0)
> + goto cleanup;
> +
> + if (qemuMonitorTestAddItemVerbatim(test,
> + "{\"execute\":\"nbd-server-start\","
> + " \"arguments\":{\"addr\":{"
> + " \"type\":\"unix\",\"data\":{"
> + " \"path\":\"/tmp/sock\"}},"
> + " \"tls-creds\":\"test-alias\"},"
> + " \"id\":\"libvirt-2\"}",
> + NULL, "{\"return\":{}}") < 0)
> + goto cleanup;
> +
> + if (qemuMonitorJSONNBDServerStart(qemuMonitorTestGetMonitor(test),
> + &server_tcp, "test-alias") < 0)
> + goto cleanup;
> +
> + if (qemuMonitorJSONNBDServerStart(qemuMonitorTestGetMonitor(test),
> + &server_unix, "test-alias") < 0)
> + goto cleanup;
> +
> + ret = 0;
> +
> + cleanup:
> + qemuMonitorTestFree(test);
> + return ret;
> +}
> +
> static bool
> testQemuMonitorJSONqemuMonitorJSONQueryCPUsEqual(struct qemuMonitorQueryCpusEntry *a,
> struct qemuMonitorQueryCpusEntry *b)
> @@ -3014,7 +3068,6 @@ mymain(void)
> DO_TEST_GEN(qemuMonitorJSONDrivePivot);
> DO_TEST_GEN(qemuMonitorJSONScreendump);
> DO_TEST_GEN(qemuMonitorJSONOpenGraphics);
> - DO_TEST_GEN(qemuMonitorJSONNBDServerStart);
Which are deleted here.
> DO_TEST_GEN(qemuMonitorJSONNBDServerAdd);
> DO_TEST_GEN(qemuMonitorJSONDetachCharDev);
> DO_TEST_GEN(qemuMonitorJSONBlockdevTrayOpen);
> @@ -3036,6 +3089,7 @@ mymain(void)
ACK with schema checks preserved.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 6/6/19 7:53 AM, Peter Krempa wrote:
> On Wed, Jun 05, 2019 at 22:01:07 -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 a nice reusable struct that can track
>> both TCP and Unix sockets.
>>
>> Update qemumonitorjsontest to not only test the response to the
>> command, but to actually verify that the command itself uses the two
>> correct QMP forms. I'm actually a bit surprised that we are not
>> utilizing qemuMonitorTestAddItemVerbatim more frequently.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>> +static int
>> +testQemuMonitorJSONqemuMonitorJSONNBDServerStart(const void *opaque)
>> +{
>> + virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr) opaque;
>> + qemuMonitorTestPtr test = qemuMonitorTestNewSimple(true, xmlopt);
>
> I'd suggest you use qemuMonitorTestNewSchema here so that we preserve
> the schema checks.
Oh, I _completely_ missed what TestNewSchema was providing.
>
>> + int ret = -1;
>> + virStorageNetHostDef server_tcp = {
>> + .name = (char *)"localhost",
>> + .port = 12345,
>> + .transport = VIR_STORAGE_NET_HOST_TRANS_TCP,
>> + };
>> + virStorageNetHostDef server_unix = {
>> + .socket = (char *)"/tmp/sock",
>> + .transport = VIR_STORAGE_NET_HOST_TRANS_UNIX,
>> + };
>> +
>> + if (!test)
>> + return -1;
>> +
>> + if (qemuMonitorTestAddItemVerbatim(test,
>> + "{\"execute\":\"nbd-server-start\","
>> + " \"arguments\":{\"addr\":{"
>> + " \"type\":\"inet\",\"data\":{"
>> + " \"host\":\"localhost\","
>> + " \"port\":\"12345\"}},"
>> + " \"tls-creds\":\"test-alias\"},"
>> + " \"id\":\"libvirt-1\"}",
>> + NULL, "{\"return\":{}}") < 0)
Testing for a verbatim response proves that we generated at least one
form of acceptable JSON, but if TestNewSchema is able to validate that
the entire command complies with the introspected schema advertised by
qemu, then that also serves to validate things, and with a lot less
fragility. Yes, I'll fix that, now that I _finally_ understand what you
were asking for (you made a similar comment in your v8 review, and I
misunderstood it).
>> @@ -3036,6 +3089,7 @@ mymain(void)
>
> ACK with schema checks preserved.
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Jun 06, 2019 at 08:40:14 -0500, Eric Blake wrote:
> On 6/6/19 7:53 AM, Peter Krempa wrote:
> > On Wed, Jun 05, 2019 at 22:01:07 -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 a nice reusable struct that can track
> >> both TCP and Unix sockets.
> >>
> >> Update qemumonitorjsontest to not only test the response to the
> >> command, but to actually verify that the command itself uses the two
> >> correct QMP forms. I'm actually a bit surprised that we are not
> >> utilizing qemuMonitorTestAddItemVerbatim more frequently.
> >>
> >> Signed-off-by: Eric Blake <eblake@redhat.com>
> >> ---
>
> >> +static int
> >> +testQemuMonitorJSONqemuMonitorJSONNBDServerStart(const void *opaque)
> >> +{
> >> + virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr) opaque;
> >> + qemuMonitorTestPtr test = qemuMonitorTestNewSimple(true, xmlopt);
> >
> > I'd suggest you use qemuMonitorTestNewSchema here so that we preserve
> > the schema checks.
>
> Oh, I _completely_ missed what TestNewSchema was providing.
It has one catch though that I've figured out now.
qemuMonitorTestAddItemVerbatim actually does not support schema checking
yet. I've implemented only a limited subset of the internals to support
it.
Probably the best course of actions for this test will be to swithch it
to qemuMonitorTestAddItem which does schema checking.
In this case I feel it's more useful to do the check against the schema
rather than to see that the resposne is the same.
Alternatively I can see whether it's reasonably feasible to do schema
checking also in qemuMonitorTestAddItemVerbatim.
> >> + int ret = -1;
> >> + virStorageNetHostDef server_tcp = {
> >> + .name = (char *)"localhost",
> >> + .port = 12345,
> >> + .transport = VIR_STORAGE_NET_HOST_TRANS_TCP,
> >> + };
> >> + virStorageNetHostDef server_unix = {
> >> + .socket = (char *)"/tmp/sock",
> >> + .transport = VIR_STORAGE_NET_HOST_TRANS_UNIX,
> >> + };
> >> +
> >> + if (!test)
> >> + return -1;
> >> +
> >> + if (qemuMonitorTestAddItemVerbatim(test,
> >> + "{\"execute\":\"nbd-server-start\","
> >> + " \"arguments\":{\"addr\":{"
> >> + " \"type\":\"inet\",\"data\":{"
> >> + " \"host\":\"localhost\","
> >> + " \"port\":\"12345\"}},"
> >> + " \"tls-creds\":\"test-alias\"},"
> >> + " \"id\":\"libvirt-1\"}",
> >> + NULL, "{\"return\":{}}") < 0)
>
> Testing for a verbatim response proves that we generated at least one
> form of acceptable JSON, but if TestNewSchema is able to validate that
> the entire command complies with the introspected schema advertised by
Well, it still validates only the one given syntax at this point,
because there's no sane way to compare everything what libvirt is able
to generate.
> qemu, then that also serves to validate things, and with a lot less
> fragility. Yes, I'll fix that, now that I _finally_ understand what you
> were asking for (you made a similar comment in your v8 review, and I
> misunderstood it).
The huge advantage of this is that I don't actually have to check for
typos and other things manually against qemu's schema :)
>
> >> @@ -3036,6 +3089,7 @@ mymain(void)
> >
> > ACK with schema checks preserved.
> >
>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc. +1-919-301-3226
> Virtualization: qemu.org | libvirt.org
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, Jun 07, 2019 at 10:13:04 +0200, Peter Krempa wrote: > On Thu, Jun 06, 2019 at 08:40:14 -0500, Eric Blake wrote: > > On 6/6/19 7:53 AM, Peter Krempa wrote: [...] > In this case I feel it's more useful to do the check against the schema > rather than to see that the resposne is the same. > > Alternatively I can see whether it's reasonably feasible to do schema > checking also in qemuMonitorTestAddItemVerbatim. https://www.redhat.com/archives/libvir-list/2019-June/msg00210.html So we can keep using qemuMonitorTestAddItemVerbatim here. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 6/7/19 7:06 AM, Peter Krempa wrote: > On Fri, Jun 07, 2019 at 10:13:04 +0200, Peter Krempa wrote: >> On Thu, Jun 06, 2019 at 08:40:14 -0500, Eric Blake wrote: >>> On 6/6/19 7:53 AM, Peter Krempa wrote: > > [...] > >> In this case I feel it's more useful to do the check against the schema >> rather than to see that the resposne is the same. >> >> Alternatively I can see whether it's reasonably feasible to do schema >> checking also in qemuMonitorTestAddItemVerbatim. > > https://www.redhat.com/archives/libvir-list/2019-June/msg00210.html > > So we can keep using qemuMonitorTestAddItemVerbatim here. AddItemVerbatim is a pain to maintain; I'd rather stick with AddItem + schema checks. But in doing that, I found that a lot of existing code in the test did not do schema tests; hence I'm planning on pushing these four patches (amended per your review) only after a prerequisite fix of the existing tests: https://www.redhat.com/archives/libvir-list/2019-June/msg00283.html -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Jun 10, 2019 at 12:11:34 -0500, Eric Blake wrote: > On 6/7/19 7:06 AM, Peter Krempa wrote: > > On Fri, Jun 07, 2019 at 10:13:04 +0200, Peter Krempa wrote: > >> On Thu, Jun 06, 2019 at 08:40:14 -0500, Eric Blake wrote: > >>> On 6/6/19 7:53 AM, Peter Krempa wrote: > > > > [...] > > > >> In this case I feel it's more useful to do the check against the schema > >> rather than to see that the resposne is the same. > >> > >> Alternatively I can see whether it's reasonably feasible to do schema > >> checking also in qemuMonitorTestAddItemVerbatim. > > > > https://www.redhat.com/archives/libvir-list/2019-June/msg00210.html > > > > So we can keep using qemuMonitorTestAddItemVerbatim here. > > AddItemVerbatim is a pain to maintain; I'd rather stick with AddItem + > schema checks. But in doing that, I found that a lot of existing code in > the test did not do schema tests; hence I'm planning on pushing these > four patches (amended per your review) only after a prerequisite fix of I agree. If there isn't a particular reason to check the data on the monitor as well, using the AddItem is sufficient when we do a schema check. The AddItemVerbatim function makes sense when we couple it with functional testing of other code as well where we need to validate that libvirt's commands are correct as well e.g. as we do for the cpu hotplug tests. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.