Re: [libvirt] mdevctl: A shoestring mediated device management and persistence utility
On Tue, 11 Jun 2019 21:45:08 +0200 Cornelia Huck wrote: > On Fri, 7 Jun 2019 18:06:30 +0200 > Halil Pasic wrote: > > > On Fri, 24 May 2019 12:11:06 +0200 > > Cornelia Huck wrote: > > > > > On Thu, 23 May 2019 17:20:01 -0600 > > > Alex Williamson wrote: > > > > > > > Hi, > > > > > > > > [..] > > > > > > > > > > It would be really useful if s390 folks could help me understand > > > > whether it's possible to glean all the information necessary to > > > > recreate a ccw or ap mdev device from sysfs. I expect the file where > > > > we currently only store the mdev_type to evolve into something that > > > > includes more information to facilitate more complicated devices. For > > > > now I make no claims to maintaining compatibility of recorded mdev > > > > devices, it will absolutely change, but I didn't want to get bogged > > > > down in making sure I don't accidentally source a root kit hidden in an > > > > mdev config file. > > > > > > I played a bit with it on my LPAR, and it is at least not obviously > > > broken with vfio-ccw :) I don't have any ap devices to play with, > > > though. > > > > > > > Sorry for being late... > > > > I guess for vfio-ccw one needs to make sure that the ccw device is bound > > to the vfio-ccw driver first, and only after that can one use > > create-mdev to create the mdev on top of the subchannel. > > > > So to make this work persistently (survive a reboot) one would need to > > take care of the subchannel getting bound to the right vfio_ccw driver > > before mdevctl is called. Right? > > > > BTW how does this concurrence situation between the drivers io_subchannel > > and vfio_ccw work? Especially if both are build in? > > If you have two drivers that match to the same device type, you'll > always have the issue that the driver that is first matched with the > device will bind to it and you have to do the unbind/rebind dance to > get it bound to the correct device driver. (I guess that this was the > basic motivation behind the ap bus default driver infrastructure, > right?) I think that in our case the io_subchannel driver will be > called first (alphabetical order and the fact that vfio-ccw will often > be a module). I'm not sure if it is within the scope of mdevctl to > ensure that the device is bound to the correct driver, or if it rather > should work with devices already bound to the correct driver only. > Maybe a separate udev-rules generator? Getting a device bound to a specific driver is exactly the domain of driverctl. Implement the sysfs interfaces driverctl uses and see if it works. Driverctl defaults to PCI and knows some extra things about PCI, but appears to be written to be generally bus agnostic. Thanks, Alex > There's also the question where that automatic configuration should > stop. Should cio_ignore handling be part of it as well? [That's a > non-generic interface, of course. Tooling within s390-tools, maybe?] > > > > > > > > > I'm also curious how or if libvirt or openstack might use this. > > > > If nothing else, it makes libvirt hook scripts easier to write, > > > > especially if we add an option not to autostart mdevs, or if > > > > users don't mind persistent mdevs, maybe there's nothing more to > > > > do. > > > > +1 > > > > @Alex: I'm curious what is the big management picture for non-auto > > looks like. > > > > Regards, > > Halil > > > > [..] > > > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] mdevctl: A shoestring mediated device management and persistence utility
On Fri, 7 Jun 2019 18:06:30 +0200 Halil Pasic wrote: > On Fri, 24 May 2019 12:11:06 +0200 > Cornelia Huck wrote: > > > On Thu, 23 May 2019 17:20:01 -0600 > > Alex Williamson wrote: > > > > > Hi, > > > > > [..] > > > > > > > It would be really useful if s390 folks could help me understand > > > whether it's possible to glean all the information necessary to > > > recreate a ccw or ap mdev device from sysfs. I expect the file where > > > we currently only store the mdev_type to evolve into something that > > > includes more information to facilitate more complicated devices. For > > > now I make no claims to maintaining compatibility of recorded mdev > > > devices, it will absolutely change, but I didn't want to get bogged > > > down in making sure I don't accidentally source a root kit hidden in an > > > mdev config file. > > > > I played a bit with it on my LPAR, and it is at least not obviously > > broken with vfio-ccw :) I don't have any ap devices to play with, > > though. > > > > Sorry for being late... > > I guess for vfio-ccw one needs to make sure that the ccw device is bound > to the vfio-ccw driver first, and only after that can one use > create-mdev to create the mdev on top of the subchannel. > > So to make this work persistently (survive a reboot) one would need to > take care of the subchannel getting bound to the right vfio_ccw driver > before mdevctl is called. Right? > > BTW how does this concurrence situation between the drivers io_subchannel > and vfio_ccw work? Especially if both are build in? If you have two drivers that match to the same device type, you'll always have the issue that the driver that is first matched with the device will bind to it and you have to do the unbind/rebind dance to get it bound to the correct device driver. (I guess that this was the basic motivation behind the ap bus default driver infrastructure, right?) I think that in our case the io_subchannel driver will be called first (alphabetical order and the fact that vfio-ccw will often be a module). I'm not sure if it is within the scope of mdevctl to ensure that the device is bound to the correct driver, or if it rather should work with devices already bound to the correct driver only. Maybe a separate udev-rules generator? There's also the question where that automatic configuration should stop. Should cio_ignore handling be part of it as well? [That's a non-generic interface, of course. Tooling within s390-tools, maybe?] > > > > > > I'm also curious how or if libvirt or openstack might use this. > > > If nothing else, it makes libvirt hook scripts easier to write, > > > especially if we add an option not to autostart mdevs, or if > > > users don't mind persistent mdevs, maybe there's nothing more to > > > do. > > +1 > > @Alex: I'm curious what is the big management picture for non-auto > looks like. > > Regards, > Halil > > [..] > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] qemu: Use proper block job name when reconnecting to VM
On Tue, Jun 11, 2019 at 04:56:25PM +0200, Peter Krempa wrote: The hash table returned by qemuMonitorGetAllBlockJobInfo is organized by the frontend name (which skipps the 'drive-' prefix). While our code properly matches the jobs to the disk, qemu needs the full job name including the 'drive-' prefix to be able to identify jobs. Fix this by adding an argument to qemuMonitorGetAllBlockJobInfo which does not modify the job name before filling the hash. This fixes a regression where users would not be able to cancel/pivot block jobs after restarting libvirtd while a blockjob is running. Signed-off-by: Peter Krempa --- src/qemu/qemu_migration.c| 2 +- src/qemu/qemu_monitor.c | 7 --- src/qemu/qemu_monitor.h | 3 ++- src/qemu/qemu_monitor_json.c | 12 src/qemu/qemu_monitor_json.h | 3 ++- src/qemu/qemu_process.c | 2 +- 6 files changed, 18 insertions(+), 11 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/3] tests: uri: Improve uri testing and add test for urlencoded uris
On Tue, Jun 11, 2019 at 05:43:13PM +0200, Peter Krempa wrote: Peter Krempa (3): tests: uri: Use VIR_TEST_DEBUG instead of VIR_DEBUG tests: uri: Run all test cases on a single URI tests: uri: Add test for urlencoded URIs tests/viruritest.c | 80 +++--- 1 file changed, 47 insertions(+), 33 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] qemu: domain: Allow overriding images to read-write in qemuDomainStorageSourceAccessAllow
On Tue, Jun 11, 2019 at 04:56:24PM +0200, Peter Krempa wrote:
In commit 76b9aba2ba6 I refactored how the function treats the readonly
flag which introduced a bug when we'd not allow to force read-write
state for an image.
This created problems with blockjobs where we need to temporarily
override images to read-write.
Rename QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_READ_ONLY to
QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_OVERRIDE_READ_ONLY and also introduce
a complement QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_OVERRIDE_READ_WRITE which
Rather than OVERRIDE_ I'd use FORCE_. overriding read-only access might
imply that you want to force it to read-write.
will allow to force wire access.
s/wire/write/
https://bugzilla.redhat.com/show_bug.cgi?id=1717768
Signed-off-by: Peter Krempa
---
src/qemu/qemu_domain.c | 19 +--
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index e521bd3982..595150708b 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -9305,12 +9305,14 @@ typedef enum {
QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_REVOKE = 1 << 0,
/* operate on full backing chain rather than single image */
QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_CHAIN = 1 << 1,
-/* force permissions to read-only when allowing */
-QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_READ_ONLY = 1 << 2,
+/* force permissions to read-only/read-write when allowing */
+/* currently does not properly wrok with
QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_CHAIN */
s/wrok/work/
+QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_OVERRIDE_READ_ONLY = 1 << 2,
+QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_OVERRIDE_READ_WRITE = 1 << 3,
/* don't revoke permissions when modification has failed */
-QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_SKIP_REVOKE = 1 << 3,
+QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_SKIP_REVOKE = 1 << 4,
/* VM already has access to the source and we are just modifying it */
-QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_MODIFY_ACCESS = 1 << 4,
+QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_MODIFY_ACCESS = 1 << 5,
} qemuDomainStorageSourceAccessFlags;
@@ -9344,9 +9346,12 @@ qemuDomainStorageSourceAccessModify(virQEMUDriverPtr
driver,
Reviewed-by: Ján Tomko
Jano
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/3] tests: uri: Run all test cases on a single URI
Determine whether the test has failed after running all the cases so
that we don't need to rerun it multiple times to see all problems.
Signed-off-by: Peter Krempa
---
tests/viruritest.c | 32 +++-
1 file changed, 19 insertions(+), 13 deletions(-)
diff --git a/tests/viruritest.c b/tests/viruritest.c
index 336fffbf0f..04c237832d 100644
--- a/tests/viruritest.c
+++ b/tests/viruritest.c
@@ -51,6 +51,7 @@ static int testURIParse(const void *args)
const struct URIParseData *data = args;
char *uristr = NULL;
size_t i;
+bool fail = false;
if (!(uri = virURIParse(data->uri)))
goto cleanup;
@@ -58,60 +59,62 @@ static int testURIParse(const void *args)
if (STRNEQ(uri->scheme, data->scheme)) {
VIR_TEST_DEBUG("Expected scheme '%s', actual '%s'",
data->scheme, uri->scheme);
-goto cleanup;
+fail = true;
}
if (STRNEQ(uri->server, data->server)) {
VIR_TEST_DEBUG("Expected server '%s', actual '%s'",
data->server, uri->server);
-goto cleanup;
+fail = true;
}
if (uri->port != data->port) {
VIR_TEST_DEBUG("Expected port '%d', actual '%d'",
data->port, uri->port);
-goto cleanup;
+fail = true;
}
if (STRNEQ_NULLABLE(uri->path, data->path)) {
VIR_TEST_DEBUG("Expected path '%s', actual '%s'",
data->path, uri->path);
-goto cleanup;
+fail = true;
}
if (STRNEQ_NULLABLE(uri->query, data->query)) {
VIR_TEST_DEBUG("Expected query '%s', actual '%s'",
data->query, uri->query);
-goto cleanup;
+fail = true;
}
if (STRNEQ_NULLABLE(uri->fragment, data->fragment)) {
VIR_TEST_DEBUG("Expected fragment '%s', actual '%s'",
data->fragment, uri->fragment);
-goto cleanup;
+fail = true;
}
for (i = 0; data->params && data->params[i].name && i < uri->paramsCount;
i++) {
if (STRNEQ_NULLABLE(data->params[i].name, uri->params[i].name)) {
VIR_TEST_DEBUG("Expected param name %zu '%s', actual '%s'",
i, data->params[i].name, uri->params[i].name);
-goto cleanup;
+fail = true;
}
if (STRNEQ_NULLABLE(data->params[i].value, uri->params[i].value)) {
VIR_TEST_DEBUG("Expected param value %zu '%s', actual '%s'",
i, data->params[i].value, uri->params[i].value);
-goto cleanup;
+fail = true;
}
}
if (data->params && data->params[i].name) {
VIR_TEST_DEBUG("Missing parameter %zu %s=%s",
i, data->params[i].name, data->params[i].value);
-goto cleanup;
+fail = true;
}
if (i != uri->paramsCount) {
-VIR_TEST_DEBUG("Unexpected parameter %zu %s=%s",
- i, uri->params[i].name, uri->params[i].value);
-goto cleanup;
+for (; i < uri->paramsCount; i++) {
+VIR_TEST_DEBUG("Unexpected parameter %zu %s=%s",
+ i, uri->params[i].name, uri->params[i].value);
+}
+fail = true;
}
VIR_FREE(uri->query);
@@ -123,9 +126,12 @@ static int testURIParse(const void *args)
if (STRNEQ(uristr, data->uri_out)) {
VIR_TEST_DEBUG("URI did not roundtrip, expect '%s', actual '%s'",
data->uri_out, uristr);
-goto cleanup;
+fail = true;
}
+if (fail)
+goto cleanup;
+
ret = 0;
cleanup:
VIR_FREE(uristr);
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] qemu: domain: Fix logic bug in qemuDomainStorageSourceAccessAllow
On Tue, Jun 11, 2019 at 04:56:23PM +0200, Peter Krempa wrote: In commit 76b9aba2ba6 I tried to refactor qemuDomainStorageSourceAccessAllow but used wrong operators for adding bitwise flags. How did that get past review? O:-) This way the flags would result in 0 if any of them would be applied. Signed-off-by: Peter Krempa --- src/qemu/qemu_domain.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/3] tests: uri: Add test for urlencoded URIs
When specifying extra params for spcie TLS verification, it's necessary
to pass a weird URI to it. Let's add a test for this case where the TLS
string contains a space.
Signed-off-by: Peter Krempa
---
tests/viruritest.c | 8
1 file changed, 8 insertions(+)
diff --git a/tests/viruritest.c b/tests/viruritest.c
index 04c237832d..c09e5323bc 100644
--- a/tests/viruritest.c
+++ b/tests/viruritest.c
@@ -178,6 +178,14 @@ mymain(void)
TEST_PARSE("test://[2001:41c8:1:4fd4::2]:123/system", "test",
"2001:41c8:1:4fd4::2", 123, "/system", NULL, NULL, NULL, NULL);
TEST_PARSE("gluster+rdma://example.com:1234/gv0/vol.img", "gluster+rdma",
"example.com", 1234, "/gv0/vol.img", NULL, NULL, NULL, NULL);
+virURIParam spiceparams[] = {
+{ (char *) "tlsSubject", (char *) "C=XX,L=Testtown,O=Test
Company,CN=tester.test", false },
+{ NULL, NULL, false },
+};
+
TEST_FULL("spice://[3ffe::104]:5900/?tlsSubject=C=XX,L=Testtown,O=Test%20Company,CN=tester.test",
+
"spice://[3ffe::104]:5900/?tlsSubject=C%3dXX%2cL%3dTesttown%2cO%3dTest%20Company%2cCN%3dtester%2etest",
+ "spice", "3ffe::104", 5900, "/",
"tlsSubject=C=XX,L=Testtown,O=Test%20Company,CN=tester.test", NULL, NULL,
spiceparams);
+
virURIParam params1[] = {
{ (char*)"foo", (char*)"one", false },
{ (char*)"bar", (char*)"two", false },
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/3] tests: uri: Use VIR_TEST_DEBUG instead of VIR_DEBUG
VIR_TEST_DEBUG can be easily made verbose in the tests.
Signed-off-by: Peter Krempa
---
tests/viruritest.c | 44 ++--
1 file changed, 22 insertions(+), 22 deletions(-)
diff --git a/tests/viruritest.c b/tests/viruritest.c
index ba5db15011..336fffbf0f 100644
--- a/tests/viruritest.c
+++ b/tests/viruritest.c
@@ -56,61 +56,61 @@ static int testURIParse(const void *args)
goto cleanup;
if (STRNEQ(uri->scheme, data->scheme)) {
-VIR_DEBUG("Expected scheme '%s', actual '%s'",
- data->scheme, uri->scheme);
+VIR_TEST_DEBUG("Expected scheme '%s', actual '%s'",
+ data->scheme, uri->scheme);
goto cleanup;
}
if (STRNEQ(uri->server, data->server)) {
-VIR_DEBUG("Expected server '%s', actual '%s'",
- data->server, uri->server);
+VIR_TEST_DEBUG("Expected server '%s', actual '%s'",
+ data->server, uri->server);
goto cleanup;
}
if (uri->port != data->port) {
-VIR_DEBUG("Expected port '%d', actual '%d'",
- data->port, uri->port);
+VIR_TEST_DEBUG("Expected port '%d', actual '%d'",
+ data->port, uri->port);
goto cleanup;
}
if (STRNEQ_NULLABLE(uri->path, data->path)) {
-VIR_DEBUG("Expected path '%s', actual '%s'",
- data->path, uri->path);
+VIR_TEST_DEBUG("Expected path '%s', actual '%s'",
+ data->path, uri->path);
goto cleanup;
}
if (STRNEQ_NULLABLE(uri->query, data->query)) {
-VIR_DEBUG("Expected query '%s', actual '%s'",
- data->query, uri->query);
+VIR_TEST_DEBUG("Expected query '%s', actual '%s'",
+ data->query, uri->query);
goto cleanup;
}
if (STRNEQ_NULLABLE(uri->fragment, data->fragment)) {
-VIR_DEBUG("Expected fragment '%s', actual '%s'",
- data->fragment, uri->fragment);
+VIR_TEST_DEBUG("Expected fragment '%s', actual '%s'",
+ data->fragment, uri->fragment);
goto cleanup;
}
for (i = 0; data->params && data->params[i].name && i < uri->paramsCount;
i++) {
if (STRNEQ_NULLABLE(data->params[i].name, uri->params[i].name)) {
-VIR_DEBUG("Expected param name %zu '%s', actual '%s'",
- i, data->params[i].name, uri->params[i].name);
+VIR_TEST_DEBUG("Expected param name %zu '%s', actual '%s'",
+ i, data->params[i].name, uri->params[i].name);
goto cleanup;
}
if (STRNEQ_NULLABLE(data->params[i].value, uri->params[i].value)) {
-VIR_DEBUG("Expected param value %zu '%s', actual '%s'",
- i, data->params[i].value, uri->params[i].value);
+VIR_TEST_DEBUG("Expected param value %zu '%s', actual '%s'",
+ i, data->params[i].value, uri->params[i].value);
goto cleanup;
}
}
if (data->params && data->params[i].name) {
-VIR_DEBUG("Missing parameter %zu %s=%s",
- i, data->params[i].name, data->params[i].value);
+VIR_TEST_DEBUG("Missing parameter %zu %s=%s",
+ i, data->params[i].name, data->params[i].value);
goto cleanup;
}
if (i != uri->paramsCount) {
-VIR_DEBUG("Unexpected parameter %zu %s=%s",
- i, uri->params[i].name, uri->params[i].value);
+VIR_TEST_DEBUG("Unexpected parameter %zu %s=%s",
+ i, uri->params[i].name, uri->params[i].value);
goto cleanup;
}
@@ -121,8 +121,8 @@ static int testURIParse(const void *args)
goto cleanup;
if (STRNEQ(uristr, data->uri_out)) {
-VIR_DEBUG("URI did not roundtrip, expect '%s', actual '%s'",
- data->uri_out, uristr);
+VIR_TEST_DEBUG("URI did not roundtrip, expect '%s', actual '%s'",
+ data->uri_out, uristr);
goto cleanup;
}
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/3] tests: uri: Improve uri testing and add test for urlencoded uris
Peter Krempa (3): tests: uri: Use VIR_TEST_DEBUG instead of VIR_DEBUG tests: uri: Run all test cases on a single URI tests: uri: Add test for urlencoded URIs tests/viruritest.c | 80 +++--- 1 file changed, 47 insertions(+), 33 deletions(-) -- 2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 0/4] test_driver: implement virDomainSaveImageGetXMLDesc and virDomainSaveImageDefineXML
On Mon, Jun 10, 2019 at 11:04:57AM +0200, Ilias Stamatis wrote: > Changed in v2: > > * in testDomainSaveImageDefineXML check first that a saved image already > exists before parsing the new file > * reordered the arguments of testDomainSaveImageWrite in order for its > signature to be consistent with testDomainSaveImageOpen > > While implementing virDomainSaveImageGetXMLDesc and > virDomainSaveImageDefineXML for the test driver, I realized that there > exists already code for saving and loading test images which can be > reused. However, it needed to be extracted from testDomainSaveFlags and > testDomainRestoreFlags into separate functions. The new functions are > inspired by the corresponding QEMU driver code where e.g. > qemuDomainSaveImageOpen serves as a helper used by other functions. > > This series of patches initially extracts the code mentioned above into > separate functions and then provides the test driver with > implementations for virDomainSaveImageGetXMLDesc and > virDomainSaveImageDefineXML which make use of the newly introduced > functions. > > Ilias Stamatis (4): > test_driver: extract image saving code into a separate function > test_driver: extract image loading code into a separate function > test_driver: implement virDomainSaveImageDefineXML > test_driver: implement virDomainSaveImageGetXMLDesc > > src/test/test_driver.c | 293 - > 1 file changed, 205 insertions(+), 88 deletions(-) Reviewed-by: Erik Skultety -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 3/4] test_driver: implement virDomainSaveImageDefineXML
On Mon, Jun 10, 2019 at 11:05:00AM +0200, Ilias Stamatis wrote:
> Updates the existing image stored in @path, in case @dxml contains valid
> XML supported by the fake host.
>
> Signed-off-by: Ilias Stamatis
> ---
> src/test/test_driver.c | 37 +
> 1 file changed, 37 insertions(+)
>
> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> index 1b92cb43dd..906c9d5365 100644
> --- a/src/test/test_driver.c
> +++ b/src/test/test_driver.c
> @@ -2255,6 +2255,42 @@ testDomainRestore(virConnectPtr conn,
> return testDomainRestoreFlags(conn, path, NULL, 0);
> }
>
> +
> +static int
> +testDomainSaveImageDefineXML(virConnectPtr conn,
> + const char *path,
> + const char *dxml,
> + unsigned int flags)
> +{
> +int ret = -1;
> +int fd = -1;
> +virDomainDefPtr def = NULL;
> +virDomainDefPtr newdef = NULL;
> +testDriverPtr privconn = conn->privateData;
> +
> +virCheckFlags(VIR_DOMAIN_SAVE_RUNNING |
> + VIR_DOMAIN_SAVE_PAUSED, -1);
> +
> +if ((fd = testDomainSaveImageOpen(privconn, path, )) < 0)
> +goto cleanup;
Since we're not going to use @fd anymore, I'd move VIR_FORCE_CLOSE(fd) here to
be more explicit.
I'll make the change before pushing:
Reviewed-by: Erik Skultety
> +
> +if ((newdef = virDomainDefParseString(dxml, privconn->caps,
> privconn->xmlopt, NULL,
> + VIR_DOMAIN_DEF_PARSE_INACTIVE)) ==
> NULL)
> +goto cleanup;
> +
> +if (!testDomainSaveImageWrite(privconn, path, newdef))
> +goto cleanup;
> +
> +ret = 0;
> +
> + cleanup:
> +VIR_FORCE_CLOSE(fd);
> +virDomainDefFree(def);
> +virDomainDefFree(newdef);
> +return ret;
> +}
> +
> +
> static int testDomainCoreDumpWithFormat(virDomainPtr domain,
> const char *to,
> unsigned int dumpformat,
> @@ -7077,6 +7113,7 @@ static virHypervisorDriver testHypervisorDriver = {
> .domainSaveFlags = testDomainSaveFlags, /* 0.9.4 */
> .domainRestore = testDomainRestore, /* 0.3.2 */
> .domainRestoreFlags = testDomainRestoreFlags, /* 0.9.4 */
> +.domainSaveImageDefineXML = testDomainSaveImageDefineXML, /* 5.5.0 */
> .domainCoreDump = testDomainCoreDump, /* 0.3.2 */
> .domainCoreDumpWithFormat = testDomainCoreDumpWithFormat, /* 1.2.3 */
> .domainSetVcpus = testDomainSetVcpus, /* 0.1.4 */
> --
> 2.21.0
>
> --
> 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
[libvirt] [PATCH 2/3] qemu: domain: Allow overriding images to read-write in qemuDomainStorageSourceAccessAllow
In commit 76b9aba2ba6 I refactored how the function treats the readonly
flag which introduced a bug when we'd not allow to force read-write
state for an image.
This created problems with blockjobs where we need to temporarily
override images to read-write.
Rename QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_READ_ONLY to
QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_OVERRIDE_READ_ONLY and also introduce
a complement QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_OVERRIDE_READ_WRITE which
will allow to force wire access.
https://bugzilla.redhat.com/show_bug.cgi?id=1717768
Signed-off-by: Peter Krempa
---
src/qemu/qemu_domain.c | 19 +--
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index e521bd3982..595150708b 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -9305,12 +9305,14 @@ typedef enum {
QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_REVOKE = 1 << 0,
/* operate on full backing chain rather than single image */
QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_CHAIN = 1 << 1,
-/* force permissions to read-only when allowing */
-QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_READ_ONLY = 1 << 2,
+/* force permissions to read-only/read-write when allowing */
+/* currently does not properly wrok with
QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_CHAIN */
+QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_OVERRIDE_READ_ONLY = 1 << 2,
+QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_OVERRIDE_READ_WRITE = 1 << 3,
/* don't revoke permissions when modification has failed */
-QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_SKIP_REVOKE = 1 << 3,
+QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_SKIP_REVOKE = 1 << 4,
/* VM already has access to the source and we are just modifying it */
-QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_MODIFY_ACCESS = 1 << 4,
+QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_MODIFY_ACCESS = 1 << 5,
} qemuDomainStorageSourceAccessFlags;
@@ -9344,9 +9346,12 @@ qemuDomainStorageSourceAccessModify(virQEMUDriverPtr
driver,
bool revoke_namespace = false;
bool revoke_lockspace = false;
-if (flags & QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_READ_ONLY)
+if (flags & QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_OVERRIDE_READ_ONLY)
src->readonly = true;
+if (flags & QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_OVERRIDE_READ_WRITE)
+src->readonly = false;
+
/* just tear down the disk access */
if (flags & QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_REVOKE) {
virErrorPreserveLast(_err);
@@ -9491,7 +9496,9 @@ qemuDomainStorageSourceAccessAllow(virQEMUDriverPtr
driver,
qemuDomainStorageSourceAccessFlags flags =
QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_SKIP_REVOKE;
if (readonly)
-flags |= QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_READ_ONLY;
+flags |= QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_OVERRIDE_READ_ONLY;
+else
+flags |= QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_OVERRIDE_READ_WRITE;
if (!newSource)
flags |= QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_MODIFY_ACCESS;
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/3] qemu: Fix two block-job related regressions (blockdev-add saga)
Peter Krempa (3): qemu: domain: Fix logic bug in qemuDomainStorageSourceAccessAllow qemu: domain: Allow overriding images to read-write in qemuDomainStorageSourceAccessAllow qemu: Use proper block job name when reconnecting to VM src/qemu/qemu_domain.c | 21 ++--- src/qemu/qemu_migration.c| 2 +- src/qemu/qemu_monitor.c | 7 --- src/qemu/qemu_monitor.h | 3 ++- src/qemu/qemu_monitor_json.c | 12 src/qemu/qemu_monitor_json.h | 3 ++- src/qemu/qemu_process.c | 2 +- 7 files changed, 32 insertions(+), 18 deletions(-) -- 2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/3] qemu: domain: Fix logic bug in qemuDomainStorageSourceAccessAllow
In commit 76b9aba2ba6 I tried to refactor qemuDomainStorageSourceAccessAllow but used wrong operators for adding bitwise flags. This way the flags would result in 0 if any of them would be applied. Signed-off-by: Peter Krempa --- src/qemu/qemu_domain.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 4d3a8868b2..e521bd3982 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9491,10 +9491,10 @@ qemuDomainStorageSourceAccessAllow(virQEMUDriverPtr driver, qemuDomainStorageSourceAccessFlags flags = QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_SKIP_REVOKE; if (readonly) -flags &= QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_READ_ONLY; +flags |= QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_READ_ONLY; if (!newSource) -flags &= QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_MODIFY_ACCESS; +flags |= QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_MODIFY_ACCESS; return qemuDomainStorageSourceAccessModify(driver, vm, elem, flags); } -- 2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/3] qemu: Use proper block job name when reconnecting to VM
The hash table returned by qemuMonitorGetAllBlockJobInfo is organized by
the frontend name (which skipps the 'drive-' prefix). While our code
properly matches the jobs to the disk, qemu needs the full job name
including the 'drive-' prefix to be able to identify jobs.
Fix this by adding an argument to qemuMonitorGetAllBlockJobInfo which
does not modify the job name before filling the hash.
This fixes a regression where users would not be able to cancel/pivot
block jobs after restarting libvirtd while a blockjob is running.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_migration.c| 2 +-
src/qemu/qemu_monitor.c | 7 ---
src/qemu/qemu_monitor.h | 3 ++-
src/qemu/qemu_monitor_json.c | 12
src/qemu/qemu_monitor_json.h | 3 ++-
src/qemu/qemu_process.c | 2 +-
6 files changed, 18 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 32b3040473..1ea817004d 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -5545,7 +5545,7 @@ qemuMigrationSrcFetchMirrorStats(virQEMUDriverPtr driver,
if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
return -1;
-blockinfo = qemuMonitorGetAllBlockJobInfo(priv->mon);
+blockinfo = qemuMonitorGetAllBlockJobInfo(priv->mon, false);
if (qemuDomainObjExitMonitor(driver, vm) < 0 || !blockinfo)
return -1;
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 6b731cd91a..9826426b29 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -3442,10 +3442,11 @@ qemuMonitorBlockJobSetSpeed(qemuMonitorPtr mon,
virHashTablePtr
-qemuMonitorGetAllBlockJobInfo(qemuMonitorPtr mon)
+qemuMonitorGetAllBlockJobInfo(qemuMonitorPtr mon,
+ bool rawjobname)
{
QEMU_CHECK_MONITOR_NULL(mon);
-return qemuMonitorJSONGetAllBlockJobInfo(mon);
+return qemuMonitorJSONGetAllBlockJobInfo(mon, rawjobname);
}
@@ -3465,7 +3466,7 @@ qemuMonitorGetBlockJobInfo(qemuMonitorPtr mon,
VIR_DEBUG("alias=%s, info=%p", alias, info);
-if (!(all = qemuMonitorGetAllBlockJobInfo(mon)))
+if (!(all = qemuMonitorGetAllBlockJobInfo(mon, false)))
return -1;
if ((data = virHashLookup(all, alias))) {
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index dee594fa66..cf4d567667 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -956,7 +956,8 @@ struct _qemuMonitorBlockJobInfo {
int ready; /* -1 if unknown, 0 if not ready, 1 if ready */
};
-virHashTablePtr qemuMonitorGetAllBlockJobInfo(qemuMonitorPtr mon);
+virHashTablePtr qemuMonitorGetAllBlockJobInfo(qemuMonitorPtr mon,
+ bool rawjobname);
int qemuMonitorGetBlockJobInfo(qemuMonitorPtr mon,
const char *device,
qemuMonitorBlockJobInfoPtr info)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 53a7de8b77..2873399dc3 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -4662,7 +4662,8 @@ int qemuMonitorJSONScreendump(qemuMonitorPtr mon,
static int
qemuMonitorJSONParseBlockJobInfo(virHashTablePtr blockJobs,
- virJSONValuePtr entry)
+ virJSONValuePtr entry,
+ bool rawjobname)
{
qemuMonitorBlockJobInfoPtr info = NULL;
const char *device;
@@ -4674,7 +4675,9 @@ qemuMonitorJSONParseBlockJobInfo(virHashTablePtr
blockJobs,
_("entry was missing 'device'"));
return -1;
}
-device = qemuAliasDiskDriveSkipPrefix(device);
+
+if (!rawjobname)
+device = qemuAliasDiskDriveSkipPrefix(device);
if (VIR_ALLOC(info) < 0 ||
virHashAddEntry(blockJobs, device, info) < 0) {
@@ -4724,7 +4727,8 @@ qemuMonitorJSONParseBlockJobInfo(virHashTablePtr
blockJobs,
}
virHashTablePtr
-qemuMonitorJSONGetAllBlockJobInfo(qemuMonitorPtr mon)
+qemuMonitorJSONGetAllBlockJobInfo(qemuMonitorPtr mon,
+ bool rawjobname)
{
virJSONValuePtr cmd = NULL;
virJSONValuePtr reply = NULL;
@@ -4756,7 +4760,7 @@ qemuMonitorJSONGetAllBlockJobInfo(qemuMonitorPtr mon)
_("missing array element"));
goto error;
}
-if (qemuMonitorJSONParseBlockJobInfo(blockJobs, entry) < 0)
+if (qemuMonitorJSONParseBlockJobInfo(blockJobs, entry, rawjobname) < 0)
goto error;
}
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index acef1a0a79..c8fde1d1b6 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -317,7 +317,8 @@ int qemuMonitorJSONBlockJobSetSpeed(qemuMonitorPtr mon,
unsigned long long speed)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
-virHashTablePtr qemuMonitorJSONGetAllBlockJobInfo(qemuMonitorPtr
Re: [libvirt] [PATCH RFC 1/1] allow to specify additional config data
On 6/7/19 4:03 PM, Alex Williamson wrote:
On Fri, 7 Jun 2019 14:26:13 -0400
Tony Krowiak wrote:
On 6/6/19 12:15 PM, Alex Williamson wrote:
On Thu, 6 Jun 2019 09:32:24 -0600
Alex Williamson wrote:
On Thu, 6 Jun 2019 16:44:17 +0200
Cornelia Huck wrote:
Add a rough implementation for vfio-ap.
Signed-off-by: Cornelia Huck
---
mdevctl.libexec | 25 ++
mdevctl.sbin| 56 -
2 files changed, 80 insertions(+), 1 deletion(-)
diff --git a/mdevctl.libexec b/mdevctl.libexec
index 804166b5086d..cc0546142924 100755
--- a/mdevctl.libexec
+++ b/mdevctl.libexec
@@ -54,6 +54,19 @@ wait_for_supported_types () {
fi
}
+# configure vfio-ap devices
+configure_ap_devices() {
+list="`echo "${config[$1]}" | sed 's/,/ /'`"
+[ -z "$list" ] && return
+for a in $list; do
+echo "$a" > "$supported_types/${config[mdev_type]}/devices/$uuid/$2"
+if [ $? -ne 0 ]; then
+echo "Error writing '$a' to '$uuid/$2'" >&2
+exit 1
+fi
+done
+}
+
case ${1} in
start-mdev|stop-mdev)
if [ $# -ne 2 ]; then
@@ -148,6 +161,18 @@ case ${cmd} in
echo "Error creating mdev type ${config[mdev_type]} on $parent"
>&2
exit 1
fi
+
+# some types may specify additional config data
+case ${config[mdev_type]} in
+vfio_ap-passthrough)
I think this could have some application beyond ap too, I know NVIDIA
GRID vGPUs do have some controls under the vendor hierarchy of the
device, ex. setting the frame rate limiter. The implementation here is
a bit rigid, we know a specific protocol for a specific mdev type, but
for supporting arbitrary vendor options we'd really just want to try to
apply whatever options are provided. If we didn't care about ordering,
we could just look for keys for every file in the device's immediate
sysfs hierarchy and apply any value we find, independent of the
mdev_type, ex. intel_vgpu/foo=bar Thanks,
For example:
for key in find -P $mdev_base/$uuid/ \( -path
"$mdev_base/$uuid/power/*" -o -path $mdev_base/$uuid/uevent -o -path
$mdev_base/$uuid/remove \) -prune -o -type f -print | sed -e "s|$mdev_base/$uuid/||g"); do
[ -z ${config[$key]} ] && continue
... parse value(s) and iteratively apply to key
done
The find is a little ugly to exclude stuff, maybe we just let people do
screwy stuff like specify remove=1 in their config. Also need to think
about whether we're imposing a delimiter to apply multiple values to a
key that conflicts with the attribute usage. Thanks,
Alex
I like the idea of looking for files in the device's immediate sysfs
hierarchy, but maybe the find could exclude attributes that are
not vendor defined.
How would we know what attributes are vendor defined? The above `find`
strips out the power, uevent, and remove attributes, which for GVT-g
leaves only the vendor defined attributes[1], but I don't know how to
instead do a positive match of the vendor attributes without
unmaintainable lookup tables. This starts to get into the question of
how much do we want to (or need to) protect the user from themselves.
If we let the user specify a key=value of remove=1 and the device
immediately disappears, is that a bug or a feature? Thanks,
Alex
By vendor defined, I meant attributes that are not defined by the mdev
framework, such as the 'remove' attribute. As far as whether allowing
specification of remove-1, I'd have to play with that and see what all
of the ramifications are.
Tony K
[1] GVT-g doesn't actually have an writable attributes, so we'd also
minimally want to add a test to skip read-only attributes.
Probably a good idea.
+configure_ap_devices ap_adapters assign_adapter
+configure_ap_devices ap_domains assign_domain
+configure_ap_devices ap_control_domains assign_control_domain
+# TODO: is assigning idempotent? Should we unwind on error?
+;;
+*)
+;;
+esac
;;
add-mdev)
diff --git a/mdevctl.sbin b/mdevctl.sbin
index 276cf6ddc817..eb5ee0091879 100755
--- a/mdevctl.sbin
+++ b/mdevctl.sbin
@@ -33,6 +33,8 @@ usage() {
echo "set-start : change mdev start policy, if no option specified,"
>&2
echo " system default policy is used" >&2
echo " options: [--auto] [--manual]" >&2
+echo "set-additional-config {fmt...}: supply additional
configuration" >&2
+echo "show-additional-config-format : prints the format expected by the
device" >&2
echo "list-all: list all possible mdev types supported in the system" >&2
echo "list-available: list all mdev types currently available" >&2
echo "list-mdevs: list currently configured mdevs" >&2
@@ -48,7 +50,7 @@ while (($# > 0)); do
--manual)
Re: [libvirt] [jenkins-ci PATCH] lcitool: Don't warn when using package manager directly
On Tue, Jun 11, 2019 at 14:38:31 +0200, Andrea Bolognani wrote: > On Tue, 2019-06-11 at 14:20 +0200, Peter Krempa wrote: > > On Tue, Jun 11, 2019 at 13:52:23 +0200, Andrea Bolognani wrote: > > > On Mon, 2019-05-27 at 15:06 +0200, Andrea Bolognani wrote: > > > *** PING HERE *** > > > > I think you can push it under the "nobody cares" rule. > > That's a weird way to spell "trivial" O:-) > > Which, looking at the patch again, is pretty clear it qualifies for, > so I've followed your suggestion and pushed it. > > Thank you for your not-quite-a-review! ;) Yeah, I'm sorry for the spelling of "trivial" but in cases of these subprojects which don't get as much review it should be okay to use one's own judgement to push the patch even for less trivial changes. Basically treat a certain amount of silence as "no complaints". signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [jenkins-ci PATCH] lcitool: Don't warn when using package manager directly
On Tue, 2019-06-11 at 14:20 +0200, Peter Krempa wrote: > On Tue, Jun 11, 2019 at 13:52:23 +0200, Andrea Bolognani wrote: > > On Mon, 2019-05-27 at 15:06 +0200, Andrea Bolognani wrote: > > *** PING HERE *** > > I think you can push it under the "nobody cares" rule. That's a weird way to spell "trivial" O:-) Which, looking at the patch again, is pretty clear it qualifies for, so I've followed your suggestion and pushed it. Thank you for your not-quite-a-review! ;) -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [jenkins-ci PATCH] lcitool: Don't warn when using package manager directly
On Tue, Jun 11, 2019 at 13:52:23 +0200, Andrea Bolognani wrote: > On Mon, 2019-05-27 at 15:06 +0200, Andrea Bolognani wrote: > > We only do this when performing operations that the > > corresponding Ansible module doesn't support, so we know > > what we're doing and don't want warnings to show up. > > > > Note that while only the dnf and yum modules complain at > > the moment, we might as well use warn=no everywhere so that > > we're already covered in case in the future the pkgng module > > starts detecting this as well. > > > > Signed-off-by: Andrea Bolognani > > --- > > guests/playbooks/update/tasks/base.yml | 6 ++ > > 1 file changed, 6 insertions(+) > > *** PING HERE *** I think you can push it under the "nobody cares" rule. signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] PCI(e): Documentation "io-reserve" and related properties?
On 6/7/19 2:43 PM, Andrea Bolognani wrote: On Thu, 2019-06-06 at 14:20 -0400, Michael S. Tsirkin wrote: On Thu, Jun 06, 2019 at 06:19:43PM +0200, Kashyap Chamarthy wrote: Hi folks, Today I learnt about some obscure PCIe-related properties, in context of the adding PCIe root ports to a guest, namely: io-reserve mem-reserve bus-reserve pref32-reserve pref64-reserve Unfortunately, the commit[*] that added them provided no documentation whatsover. In my scenario, I was specifically wondering about what does "io-reserve" mean, in what context to use it, etc. (But documentation about other properties is also welcome.) Anyone more well-versed in this area care to shed some light? [*] 6755e618d0 (hw/pci: add PCI resource reserve capability to legacy PCI bridge, 2018-08-21) So normally bios would reserve just enough io space to satisfy all devices behind a bridge. What if you intend to hotplug more devices? These properties allow you to ask bios to reserve extra space. Is it fair to say that setting io-reserve=0 for a pcie-root-port would be a way to implement the requirements set forth in https://bugzilla.redhat.com/show_bug.cgi?id=1408810 ? I tested this on aarch64 and it seems to work as expected, but then again without documentation it's hard to tell. More specifically, I created an aarch64/virt guest with several pcie-root-ports and it couldn't boot much further than GRUB when the number of ports exceeded 24, but as soon as I added the io-reserve=0 option I could get the same guest to boot fine with 32 or even 64 pcie-root-ports. I'm attaching the boot log for reference: there are a bunch of messages about the topic but they would appear to be benign. Hotplug seemed to work too: I tried with a single virtio-net-pci and I could access the network. My understanding is that PCIe devices are required to work without IO space, so this behavior matches my expectations. I wonder, though, what would happen if I had something like -device pcie-root-port,io-reserve=0,id=pci.1 -device pcie-pci-bridge,bus=pci.1 Would I be able to hotplug conventional PCI devices into the pcie-pci-bridge, or would the lack of IO space reservation for the pcie-root-port cause issues with that? You would not have any IO space for a PCI device or PCIe device that for some reason will require IO space (even if they shouldn't) and the hotplug operation would fail. On the other hand, if the pcie-pci-bridge device itself will require some IO space, it will work.. it worth trying. Thanks, Marcel -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/2] util: error: Add API for prefixing last set error with a string
In some cases we report a low level error message which does not have
enough information to see what the problem is. To allow improving on
this add an API which will prefix the error message with another error
message string which can be used to describe where the error comes from.
Signed-off-by: Peter Krempa
---
cfg.mk | 1 +
src/libvirt_private.syms | 1 +
src/util/virerror.c | 38 ++
src/util/virerror.h | 3 +++
4 files changed, 43 insertions(+)
diff --git a/cfg.mk b/cfg.mk
index 5074ef611a..f99b0d0b55 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -614,6 +614,7 @@ msg_gen_function += virReportError
msg_gen_function += virReportErrorHelper
msg_gen_function += virReportSystemError
msg_gen_function += xenapiSessionErrorHandler
+msg_gen_function += virLastErrorPrefixMessage
# Uncomment the following and run "make syntax-check" to see diagnostics
# that are not yet marked for translation, but that need to be rewritten
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 8ee76645cd..1ded794931 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1810,6 +1810,7 @@ virErrorPreserveLast;
virErrorRestore;
virErrorSetErrnoFromLastError;
virLastErrorIsSystemErrno;
+virLastErrorPrefixMessage;
virRaiseErrorFull;
virRaiseErrorObject;
virReportErrorHelper;
diff --git a/src/util/virerror.c b/src/util/virerror.c
index 37b5b2f3f9..7c5f4da8e8 100644
--- a/src/util/virerror.c
+++ b/src/util/virerror.c
@@ -1452,3 +1452,41 @@ bool virLastErrorIsSystemErrno(int errnum)
return false;
return true;
}
+
+
+/**
+ * virLastErrorPrefixMessage:
+ * @fmt: printf-style formatting string
+ * @...: Arguments for @fmt
+ *
+ * Prefixes last error reported with message formatted from @fmt. This is
useful
+ * if the low level error message does not convey enough information to
describe
+ * the problem.
+ */
+void
+virLastErrorPrefixMessage(const char *fmt, ...)
+{
+int save_errno = errno;
+virErrorPtr err = virGetLastError();
+VIR_AUTOFREE(char *) fmtmsg = NULL;
+VIR_AUTOFREE(char *) newmsg = NULL;
+va_list args;
+
+if (!err)
+return;
+
+va_start(args, fmt);
+
+if (virVasprintfQuiet(, fmt, args) < 0)
+goto cleanup;
+
+if (virAsprintfQuiet(, "%s: %s", fmtmsg, err->message) < 0)
+goto cleanup;
+
+VIR_FREE(err->message);
+VIR_STEAL_PTR(err->message, newmsg);
+
+ cleanup:
+va_end(args);
+errno = save_errno;
+}
diff --git a/src/util/virerror.h b/src/util/virerror.h
index 8f51510dc2..ca875fb8f4 100644
--- a/src/util/virerror.h
+++ b/src/util/virerror.h
@@ -206,6 +206,9 @@ bool virLastErrorIsSystemErrno(int errnum);
void virErrorPreserveLast(virErrorPtr *saveerr);
void virErrorRestore(virErrorPtr *savederr);
+void virLastErrorPrefixMessage(const char *fmt, ...)
+ATTRIBUTE_FMT_PRINTF(1, 2);
+
VIR_DEFINE_AUTOPTR_FUNC(virError, virFreeError);
#endif /* LIBVIRT_VIRERROR_H */
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/2] qemu: process: Report better error when virtlogd connection fails
When connecting to virtlogd fails e.g. due to wrong libvirtd selinux
process label we'd report an utterly useless error message:
$ virsh start upstream
error: Failed to start domain upstream
error: Cannot recv data: Connection reset by peer
Use virLastErrorPrefixMessage in the correct place to give a better
sense of what's going on:
$ virsh start upstream
error: Failed to start domain upstream
error: can't connect to virtlogd: Cannot recv data: Connection reset by peer
Signed-off-by: Peter Krempa
---
src/qemu/qemu_process.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 50a76aa0ed..0ac3e0727b 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -6604,8 +6604,10 @@ qemuProcessLaunch(virConnectPtr conn,
VIR_DEBUG("Creating domain log file");
if (!(logCtxt = qemuDomainLogContextNew(driver, vm,
-
QEMU_DOMAIN_LOG_CONTEXT_MODE_START)))
+
QEMU_DOMAIN_LOG_CONTEXT_MODE_START))) {
+virLastErrorPrefixMessage("%s", _("can't connect to virtlogd"));
goto cleanup;
+}
logfile = qemuDomainLogContextGetWriteFD(logCtxt);
if (qemuProcessGenID(vm, flags) < 0)
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/2] error: Allow better reporting of errors in some nested cases
Peter Krempa (2): util: error: Add API for prefixing last set error with a string qemu: process: Report better error when virtlogd connection fails cfg.mk | 1 + src/libvirt_private.syms | 1 + src/qemu/qemu_process.c | 4 +++- src/util/virerror.c | 38 ++ src/util/virerror.h | 3 +++ 5 files changed, 46 insertions(+), 1 deletion(-) -- 2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [jenkins-ci PATCH] lcitool: Don't warn when using package manager directly
On Mon, 2019-05-27 at 15:06 +0200, Andrea Bolognani wrote: > We only do this when performing operations that the > corresponding Ansible module doesn't support, so we know > what we're doing and don't want warnings to show up. > > Note that while only the dnf and yum modules complain at > the moment, we might as well use warn=no everywhere so that > we're already covered in case in the future the pkgng module > starts detecting this as well. > > Signed-off-by: Andrea Bolognani > --- > guests/playbooks/update/tasks/base.yml | 6 ++ > 1 file changed, 6 insertions(+) *** PING HERE *** -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] test_driver: implement virDomainGetLaunchSecurityInfo
Since this is the test driver and this is tied to AMD CPUs at the
moment, we can pretend that the domain doesn't have launch security and
always return 0 parameters.
Signed-off-by: Ilias Stamatis
---
src/test/test_driver.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 1aa79ce898..213f17808b 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -2284,6 +2284,18 @@ testDomainGetOSType(virDomainPtr dom ATTRIBUTE_UNUSED)
}
+static int
+testDomainGetLaunchSecurityInfo(virDomainPtr domain ATTRIBUTE_UNUSED,
+virTypedParameterPtr *params ATTRIBUTE_UNUSED,
+int *nparams,
+unsigned int flags)
+{
+virCheckFlags(0, -1);
+*nparams = 0;
+return 0;
+}
+
+
static unsigned long long
testDomainGetMaxMemory(virDomainPtr domain)
{
@@ -7011,6 +7023,7 @@ static virHypervisorDriver testHypervisorDriver = {
.domainDestroy = testDomainDestroy, /* 0.1.1 */
.domainDestroyFlags = testDomainDestroyFlags, /* 4.2.0 */
.domainGetOSType = testDomainGetOSType, /* 0.1.9 */
+.domainGetLaunchSecurityInfo = testDomainGetLaunchSecurityInfo, /* 5.5.0 */
.domainGetMaxMemory = testDomainGetMaxMemory, /* 0.1.4 */
.domainSetMaxMemory = testDomainSetMaxMemory, /* 0.1.1 */
.domainSetMemory = testDomainSetMemory, /* 0.1.4 */
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH v5 1/3] machine: show if CLI option '-numa node, mem' is supported in QAPI schema
Igor Mammedov writes: > Legacy '-numa node,mem' option has a number of issues and mgmt often > defaults to it. Unfortunately it's no possible to replace it with > an alternative '-numa memdev' without breaking migration compatibility. > What's possible though is to deprecate it, keeping option working with > old machine types only. > > In order to help users to find out if being deprecated CLI option > '-numa node,mem' is still supported by particular machine type, add new > "numa-mem-supported" property to output of query-machines. > > "numa-mem-supported" is set to 'true' for machines that currently support > NUMA, but it will be flipped to 'false' later on, once deprecation period > expires and kept 'true' only for old machine types that used to support > the legacy option so it won't break existing configuration that are using > it. > > Signed-off-by: Igor Mammedov Reviewed-by: Markus Armbruster -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH v4 1/3] machine: show if CLI option '-numa node, mem' is supported in QAPI schema
Eduardo Habkost writes:
> On Fri, Jun 07, 2019 at 07:39:17PM +0200, Markus Armbruster wrote:
>> This is correct when the TYPE_VIRT_MACHINE, TYPE_PC_MACHINE and
>> TYPE_SPAPR_MACHINE are exactly the machines supporting NUMA. How could
>> I check that?
>
> parse_numa_node() rejects the -numa option if the machine doesn't
> implement MachineClass::get_default_cpu_node_id().
>
> Grepping for it:
>
> $ git grep -pw get_default_cpu_node_id
> hw/arm/virt.c=static void virt_machine_class_init(ObjectClass *oc, void *data)
> hw/arm/virt.c:mc->get_default_cpu_node_id = virt_get_default_cpu_node_id;
> hw/core/machine.c=static void machine_numa_finish_cpu_init(MachineState
> *machine)
> hw/core/machine.c:props.node_id =
> mc->get_default_cpu_node_id(machine, i);
> hw/i386/pc.c=static void pc_machine_class_init(ObjectClass *oc, void *data)
> hw/i386/pc.c:mc->get_default_cpu_node_id = pc_get_default_cpu_node_id;
> hw/ppc/spapr.c=static void spapr_machine_class_init(ObjectClass *oc, void
> *data)
> hw/ppc/spapr.c:mc->get_default_cpu_node_id =
> spapr_get_default_cpu_node_id;
> include/hw/boards.h=typedef struct {
> include/hw/boards.h: * @get_default_cpu_node_id:
> include/hw/boards.h=struct MachineClass {
> include/hw/boards.h:int64_t (*get_default_cpu_node_id)(const MachineState
> *ms, int idx);
> numa.c=static void parse_numa_node(MachineState *ms, NumaNodeOptions *node,
> numa.c:if (!mc->cpu_index_to_instance_props ||
> !mc->get_default_cpu_node_id) {
>
>
> Related:
> [PATCH v4 01/11] numa: move numa global variable nb_numa_nodes into
> MachineState
> which adds a MachineClass::numa_supported flag to those machines.
Thanks, Eduardo!
Preferably with commit message and doc comment tweaked:
Reviewed-by: Markus Armbruster
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Question about Windows builds
On Mon, Jun 10, 2019 at 09:23:15AM -0500, Christopher Loyd Nugent wrote: > My name is Christopher Nugent. This is my first time posting to the mailing > list, and I am still adjusting to the community norms. If I do anything out > of line, please let me know. Anyway, on to the question: > > I am attempting to use libvirt in a Windows application I am developing, and > came upon the following in the documentation: > > > Libvirt is known to work as a client (not server) on Windows XP (32-bit), > and Windows 7 (64-bit). > > > I am a bit confused as to what this means. Does it mean that I won't be able > to use libvirt in a host configuration, Hyper-V acting as the underlying > hypervisor? Hi, basically what that means is that libvirtd as a daemon cannot run on Windows, and it's libvirtd acting as a server the clients are connecting to. However, since you're planning on using Hyper-V as the underlying hypervisor, you don't need libvirtd, you just need libvirt public API on the client side. That's because Hyper-V is something we call stateless driver, IOW client-side only driver which is shipped with the library. I suggest you look at the following resource [1] to understand how connection to libvirtd works. Essentially, a bunch of proprietary drivers do not need our libvirtd daemon to tunnel the remote connection nor need the daemon to store a state of the VM. [1] https://libvirt.org/api.html#Remote Erik > > I am posting to the development list because, if I am right, I would like to > donate my time to amend the situation. My application will be cross-platform > and will > > support multiple hypervisors, so using libvirt will help me not have to > duplicate functionality. > > > > Thank you for your time. > > --Chris Nugent. > > > -- > 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
Re: [libvirt] [PATCH]daemon: Fix a crash during virNetlinkEventServiceStopAll
Thank you for reminding me of that comment. Please ignore this patch,
for the comments are inaccurate.
I will send it again with steps to reproduce the bug.
On 2019/6/11 09:39, Laine Stump wrote:
On 6/10/19 2:15 AM, Liu Haitao wrote:
The virNetlinkEventServiceStopAll() should be executed behind
virStateCleanup(),
for some important resources like(static virNetlinkEventSrvPrivatePtr
server)
are freed unexpected. However virStateCleanup() need to use this
variable(server).
The call trace of virNetlinkEventServiceStopAll:
virNetlinkEventServiceStopAll()
--> virNetlinkEventServiceStop()
--> server[protocol] = NULL; // set server to null
The call trace of virStateCleanup():
virStateCleanup()
-->qemuStateCleanup()
-->qemuProcessStop()
Where does qemuStateCleanup() call qemuProcessStop()?
(If you're basing this on the comment at the top of qemuStateCleanup
that says "it will stop all active domains and networks", note that
that comment was added to the code in commit b4c282a79 on June 29,
2007. That was before my time, so I don't know if it was true at that
time that shutting down libvirtd would stop all active domains and
networks, but it definitely is not true now (and hasn't been for at
least 10 years).
What were the exact steps to cause this crash?
-->virNetDevMacVLanDeleteWithVPortProfile()
-->virNetlinkEventRemoveClient()
--> srv = server[protocol]
In virNetlinkEventRemoveClient() the variable server is used again,
but now it
is null that is freed by virNetlinkEventServiceStopAll().So it would
case a crash .
The call trace of crash:
(gdb) bt
0 __GI___pthread_mutex_lock (mutex=0x0) at
/usr/src/debug/glibc/2.24-r0/git/nptl/pthread_mutex_lock.c:67
1 0x7fb0d555d0f9 in virNetlinkEventRemoveClient () from
/usr/lib64/libvirt.so.0
2 0x7fb0d1df in virNetDevMacVLanDeleteWithVPortProfile ()
from /usr/lib64/libvirt.so.0
3 0x7fb0c1131251 in qemuProcessStop () from
/usr/lib64/libvirt/connection-driver/libvirt_driver_qemu.so
4 0x7fb0c11995ea in ?? () from
/usr/lib64/libvirt/connection-driver/libvirt_driver_qemu.so
5 0x7fb0d5588c5b in ?? () from /usr/lib64/libvirt.so.0
6 0x7fb0d5587fe8 in ?? () from /usr/lib64/libvirt.so.0
7 0x7fb0d19533f4 in start_thread (arg=0x7fb0be17b700) at
/usr/src/debug/glibc/2.24-r0/git/nptl/pthread_create.c:456
8 0x7fb0d128f10f in clone () at
../sysdeps/unix/sysv/linux/x86_64/clone.S:105
Signed-off-by: Liu Haitao
---
src/remote/remote_daemon.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
index c3782971f1..7da20a6644 100644
--- a/src/remote/remote_daemon.c
+++ b/src/remote/remote_daemon.c
@@ -1464,8 +1464,6 @@ int main(int argc, char **argv) {
/* Keep cleanup order in inverse order of startup */
virNetDaemonClose(dmn);
- virNetlinkEventServiceStopAll();
-
if (driversInitialized) {
/* NB: Possible issue with timing window between
driversInitialized
* setting if virNetlinkEventServerStart fails */
@@ -1473,6 +1471,8 @@ int main(int argc, char **argv) {
virStateCleanup();
}
+ virNetlinkEventServiceStopAll();
+
virObjectUnref(adminProgram);
virObjectUnref(srvAdm);
virObjectUnref(qemuProgram);
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/4] backup: Prepare for Unix sockets in QMP nbd-server-start
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. signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/4] backup: Add new qemu monitor bitmap
On Mon, Jun 10, 2019 at 21:49:51 -0500, Eric Blake wrote:
> On 6/7/19 2:08 AM, Peter Krempa wrote:
> > On Thu, Jun 06, 2019 at 08:41:26 -0500, Eric Blake wrote:
> >> On 6/6/19 7:48 AM, Peter Krempa wrote:
> >>> On Wed, Jun 05, 2019 at 22:01:09 -0500, Eric Blake wrote:
> The upcoming virDomainBackup() API needs to take advantage of various
> qcow2 bitmap manipulations as the basis to virDomainCheckpoints and
> incremental backups. Add four functions to expose
> block-dirty-bitmap-{add,enable,disable,merge} (this is the
> recently-added QEMU_CAPS_BITMAP_MERGE capability).
>
> Signed-off-by: Eric Blake
> ---
> src/qemu/qemu_monitor.h | 19 ++
> src/qemu/qemu_monitor_json.h | 17 +
> src/qemu/qemu_monitor.c | 51 +++
> src/qemu/qemu_monitor_json.c | 119 +++
> 4 files changed, 206 insertions(+)
> >>>
> >>> I'd suggest you add GEN_TEST_FUNC/DO_TEST_GEN tests for those. It's
> >>> simple to add and gives you checking of the arguments against the QAPI
> >>> schema for free.
> >>
> >> Will do. Do you need to see the amended patch with that added, or should
> >> I go ahead and push once I have it working?
> >
> > ACK if you add those with all the fields excercised.
>
> Here's what I'm squashing in. The MergeBitmaps test depends on my
> cleanup to use TestNewSchema everywhere. Also, I noticed that if we
> wanted to use VIR_AUTOPTR(qemuMonitorTest) it might make a lot of the
> file easier to read, but that should be a separate patch (including fixing:
> qemumonitorjsontest.c:1431:5: error: cleanup argument not a function
> VIR_AUTOPTR(qemuMonitorTest) test = NULL;
As I've pointed out in the RFC patch you've sent, adding the automatic
freeing function is a good idea, but refactoring old code does seem to
be a waste of time.
You certainly can use the new format in the code you are adding though.
> ^~~
> )
>
> diff --git i/tests/qemumonitorjsontest.c w/tests/qemumonitorjsontest.c
> index 2e7e976e9b..c07d8bf548 100644
> --- i/tests/qemumonitorjsontest.c
> +++ w/tests/qemumonitorjsontest.c
> @@ -1375,6 +1375,9 @@ GEN_TEST_FUNC(qemuMonitorJSONBlockdevTrayOpen,
> "foodev", true)
> GEN_TEST_FUNC(qemuMonitorJSONBlockdevTrayClose, "foodev")
> GEN_TEST_FUNC(qemuMonitorJSONBlockdevMediumRemove, "foodev")
> GEN_TEST_FUNC(qemuMonitorJSONBlockdevMediumInsert, "foodev", "newnode")
> +GEN_TEST_FUNC(qemuMonitorJSONAddBitmap, "node", "bitmap", true)
> +GEN_TEST_FUNC(qemuMonitorJSONEnableBitmap, "node", "bitmap")
> +GEN_TEST_FUNC(qemuMonitorJSONDeleteBitmap, "node", "bitmap")
>
> static int
> testQemuMonitorJSONqemuMonitorJSONNBDServerStart(const void *opaque)
> @@ -1419,6 +1422,44 @@
> testQemuMonitorJSONqemuMonitorJSONNBDServerStart(const void *opaque)
> return ret;
> }
>
> +static int
> +testQemuMonitorJSONqemuMonitorJSONMergeBitmaps(const void *opaque)
> +{
> +const testGenericData *data = opaque;
> +virDomainXMLOptionPtr xmlopt = data->xmlopt;
> +qemuMonitorTestPtr test = qemuMonitorTestNewSchema(xmlopt,
> data->schema);
> +int ret = -1;
> +VIR_AUTOPTR(virJSONValue) arr = NULL;
> +
> +
> +if (!test)
> +return -1;
> +
> +if (!(arr = virJSONValueNewArray()))
> +goto cleanup;
> +
> +if (virJSONValueArrayAppendString(arr, "b1") < 0 ||
> +virJSONValueArrayAppendString(arr, "b2") < 0)
> +goto cleanup;
> +
> +if (qemuMonitorTestAddItem(test, "block-dirty-bitmap-merge",
> + "{\"return\":{}}") < 0)
> +goto cleanup;
> +
> +if (qemuMonitorJSONMergeBitmaps(qemuMonitorTestGetMonitor(test),
> +"node", "dst", ) < 0)
> +goto cleanup;
> +
> +if (arr)
> +goto cleanup;
> +
> +ret = 0;
> +
> + cleanup:
> +qemuMonitorTestFree(test);
> +return ret;
> +}
> +
> static bool
> testQemuMonitorJSONqemuMonitorJSONQueryCPUsEqual(struct
> qemuMonitorQueryCpusEntry *a,
> struct
> qemuMonitorQueryCpusEntry *b)
> @@ -3109,6 +3150,9 @@ mymain(void)
> DO_TEST_GEN(qemuMonitorJSONBlockdevTrayClose);
> DO_TEST_GEN(qemuMonitorJSONBlockdevMediumRemove);
> DO_TEST_GEN(qemuMonitorJSONBlockdevMediumInsert);
> +DO_TEST_GEN(qemuMonitorJSONAddBitmap);
> +DO_TEST_GEN(qemuMonitorJSONEnableBitmap);
> +DO_TEST_GEN(qemuMonitorJSONDeleteBitmap);
> DO_TEST(qemuMonitorJSONGetBalloonInfo);
> DO_TEST(qemuMonitorJSONGetBlockInfo);
> DO_TEST(qemuMonitorJSONGetAllBlockStatsInfo);
> @@ -3125,6 +3169,7 @@ mymain(void)
> DO_TEST(qemuMonitorJSONSendKeyHoldtime);
> DO_TEST(qemuMonitorSupportsActiveCommit);
> DO_TEST(qemuMonitorJSONNBDServerStart);
> +DO_TEST(qemuMonitorJSONMergeBitmaps);
>
> DO_TEST_CPU_DATA("host");
> DO_TEST_CPU_DATA("full");
ACK to the squash-in regardless of if you decide to use AUTOPTR for the
test monitor in
Re: [libvirt] [RFC PATCH] qemumonitorjsontest: Use VIR_AUTOPTR for test cleanup
On Mon, Jun 10, 2019 at 22:03:05 -0500, Eric Blake wrote: > Everywhere else is switching, we might as well use it here, too. > > Signed-off-by: Eric Blake > > --- > > Partial patch - is it worth me continuing the cleanups on the rest of > the file? ACK to this one if you split it, but I'm unsure whether it's worth investing time into converting all of the tests. Definitely the definition of the AUTOPTR function is worth because new tests can use the new format. > --- > tests/qemumonitortestutils.h | 3 +++ > tests/qemumonitorjsontest.c | 35 +++ > 2 files changed, 18 insertions(+), 20 deletions(-) > > diff --git a/tests/qemumonitortestutils.h b/tests/qemumonitortestutils.h > index 8461c80caa..a2d2d30820 100644 > --- a/tests/qemumonitortestutils.h > +++ b/tests/qemumonitortestutils.h > @@ -24,6 +24,7 @@ > # include "qemu/qemu_conf.h" > # include "qemu/qemu_monitor.h" > # include "qemu/qemu_agent.h" > +# include "virautoclean.h" > > typedef struct _qemuMonitorTest qemuMonitorTest; > typedef qemuMonitorTest *qemuMonitorTestPtr; > @@ -102,4 +103,6 @@ qemuMonitorPtr > qemuMonitorTestGetMonitor(qemuMonitorTestPtr test); > qemuAgentPtr qemuMonitorTestGetAgent(qemuMonitorTestPtr test); > virDomainObjPtr qemuMonitorTestGetDomainObj(qemuMonitorTestPtr test); > > +VIR_DEFINE_AUTOPTR_FUNC(qemuMonitorTest, qemuMonitorTestFree); Possibly add this in a separate patch? signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemumonitorjsontest: Validate more commands against schema
On Mon, Jun 10, 2019 at 10:30:44 -0500, Eric Blake wrote:
> The DO_TEST() macro in qemumonitorjsontest.c was not passing the
> schema through, which meant that we were not validating any of those
> tests for correct usage according to the schema.
>
> Tested by using this hack, where the test mistakenly passed pre-patch,
> but correctly diagnosed the garbage post-patch:
>
> | diff --git i/src/qemu/qemu_monitor_json.c w/src/qemu/qemu_monitor_json.c
> | index 53a7de8b77..86d8450814 100644
> | --- i/src/qemu/qemu_monitor_json.c
> | +++ w/src/qemu/qemu_monitor_json.c
> | @@ -1532,7 +1532,8 @@ qemuMonitorJSONGetStatus(qemuMonitorPtr mon,
> | if (reason)
> | *reason = VIR_DOMAIN_PAUSED_UNKNOWN;
> |
> | -if (!(cmd = qemuMonitorJSONMakeCommand("query-status", NULL)))
> | +if (!(cmd = qemuMonitorJSONMakeCommand("query-status",
> | + "s:garbage", "foo", NULL)))
I use the same trick when testing these :)
> | return -1;
> |
> | if (qemuMonitorJSONCommand(mon, cmd, ) < 0)
>
> Suggested-by: Peter Krempa
> Signed-off-by: Eric Blake
> ---
> tests/qemumonitorjsontest.c | 251 +---
> 1 file changed, 149 insertions(+), 102 deletions(-)
ACK, it's good to see that actually everything was compliant :).
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
