Re: [libvirt] [PATCH] lxcStateInitialize: Don't leak driver's caps
On Tue, 2017-08-29 at 18:16 +0200, Michal Privoznik wrote:
> Funny thing. So when initializing LXC driver's capabilities,
> firstly the virLXCDriverGetCapabilities() is called. This creates
> new capabilities, stores them under driver->caps, ref() them and
> return them. However, the return value is ignored. Secondly, the
> function is called yet again and since we have driver->caps set,
> they are ref()-ed again an returned. So in the end, driver's
> capabilities have refcount of three when in fact they should have
> refcount of one.
>
> Signed-off-by: Michal Privoznik
> ---
> src/lxc/lxc_driver.c | 6 ++
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
> index 6eb88b0ba..784edad39 100644
> --- a/src/lxc/lxc_driver.c
> +++ b/src/lxc/lxc_driver.c
> @@ -1660,7 +1660,7 @@ static int lxcStateInitialize(bool privileged,
> if (!(lxc_driver->hostdevMgr = virHostdevManagerGetDefault()))
> goto cleanup;
>
> -if ((virLXCDriverGetCapabilities(lxc_driver, true)) == NULL)
> +if (!(caps = virLXCDriverGetCapabilities(lxc_driver, true)))
> goto cleanup;
>
> if (!(lxc_driver->xmlopt = lxcDomainXMLConfInit()))
> @@ -1669,9 +1669,6 @@ static int lxcStateInitialize(bool privileged,
> if (!(lxc_driver->closeCallbacks = virCloseCallbacksNew()))
> goto cleanup;
>
> -if (!(caps = virLXCDriverGetCapabilities(lxc_driver, false)))
> -goto cleanup;
> -
> if (virFileMakePath(cfg->stateDir) < 0) {
> virReportSystemError(errno,
> _("Failed to mkdir %s"),
> @@ -1700,6 +1697,7 @@ static int lxcStateInitialize(bool privileged,
> goto cleanup;
>
> virNWFilterRegisterCallbackDriver(&lxcCallbackDriver);
> +virObjectUnref(caps);
> return 0;
>
> cleanup:
ACK
--
Cedric
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 1/9] Add support for Veritas HyperScale (VxHS) block device protocol
Thanks for all the reviews! I will work on each item and get back. On Tue, Aug 29, 2017 at 4:00 PM, John Ferlan wrote: > > Probably need to beef this up a little bit... I can figure something out. > > On 08/29/2017 02:39 AM, Ashish Mittal wrote: >> Sample XML for a VxHS disk: >> >> >> >> >> >> >> >> >> eb90327c-8302-4725-9e1b-4e85ed4dc251 >> >> >> >> >> Signed-off-by: Ashish Mittal >> --- >> v5 changelog: >> (1) Rebased to latest master. >> (2) Review comments from Peter Krempa on patch v4 1/3 have been addressed >> > > Not all were, but it's also not something simple to figure out. > >> v4 changelog: >> (1) Fixes per review comments from v3. >> (2) Had to remove a test from the previous version that checked for >> error when multiple hosts are specified for VxHS device. >> This started failing virschematest with the error >> "XML document failed to validate against schema" as the >> docs/schemas/domain.rng specifies only a single host. >> >> v3 changelog: >> (1) Implemented the modern syntax for VxHS disk specification. >> (2) Changed qemuxml2argvdata VxHS test case to verify the new syntax. >> (3) Added a negative test case to check failure when multiple hosts >> are specified for a VxHS disk. >> >> v2 changelog: >> (1) Added code for JSON parsing of a VxHS vdisk. >> (2) Added test case to verify JSON parsing. >> (3) Added missing switch-case checks for VIR_STORAGE_NET_PROTOCOL_VXHS. >> (4) Fixed line wrap in qemuxml2argv-disk-drive-network-vxhs.args. >> >> docs/formatdomain.html.in | 15 --- >> docs/schemas/domaincommon.rng | 13 ++ >> src/libxl/libxl_conf.c| 1 + >> src/qemu/qemu_block.c | 60 >> +++ >> src/qemu/qemu_command.c | 9 +++ >> src/qemu/qemu_driver.c| 3 +++ >> src/qemu/qemu_parse_command.c | 15 +++ >> src/util/virstoragefile.c | 40 - >> src/util/virstoragefile.h | 1 + >> src/xenconfig/xen_xl.c| 1 + >> 10 files changed, 154 insertions(+), 4 deletions(-) >> > > Which email address would be "preferred" once this gets finalized - work > or gmail? I don't care either way - just need to know as that becomes > part of the git history. > Work email please. Thanks! > The tree is currently "frozen" for the 3.7.0 release, but I think we can > at least start adding some adjustments for at least the first few > patches once it thaws for 3.8.0. Once the TLS patches start - some more > adjustment will be necessary I think - adjustments that you may need to > make... I can make changes for the first patches as it's mostly simple > things except for the process of capabilities creation... > > FWIW: To reduce the change in this one patch - I took the liberty of > creating a patch that will be inserted before this one that only creates > the new symbol and adjusts all the switches appropriately and making you > the author. > > You are missing a patch to set up the capabilities - from Peter's review > of v4 patch 1/3 > (https://www.redhat.com/archives/libvir-list/2017-June/msg01389.html) > > "Additionally since libvirt supports QAPI introspection, this means we > are now able to detect whether qemu supports vxhs and should report an > error if it doesn't. > > You'll need to add a capability bit in qemu_capabilities.h and the > detection string in qemu_capabilities.c to virQEMUCapsQMPSchemaQueries[]." > > Because libvirt could be run on some host that's not running a QEMU with > the VxHS code we have to have a mechanism that does the appropriate > checks to ensure the underlying QEMU supports what we're trying to a > domain about to be run and generate an appropriate message if it's not > present. > > In any case, all that noted, it seems that this can be accomplished by > adding the following line to virQEMUCapsQMPSchemaQueries[]: > > "blockdev-add/arg-type/+vxhs" > > But there will also need to be a patch to add 2.10 replies and xml data. > Both of these I can do as it's not necessarily intuitively obvious... > > So... What I'll do is make some adjustments to this series, then repost > as a v6 so that you (and others) can look. > Thanks! Appreciate the help! >> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in >> index fba8cfc..64397d4 100644 >> --- a/docs/formatdomain.html.in >> +++ b/docs/formatdomain.html.in >> @@ -2520,9 +2520,9 @@ >> >>The protocol attribute specifies the protocol to >>access to the requested image. Possible values are "nbd", >> - "iscsi", "rbd", "sheepdog" or "gluster". If the >> - protocol attribute is "rbd", "sheepdog" or >> - "gluster", an additional attribute name is >> + "iscsi", "rbd", "sheepdog", "gluster" or "vxhs". If the >> + protocol attribute is "rbd", "sheepdog", >> "gluster" >> + or "vxhs", an additional attribute nam
Re: [libvirt] [PATCH v5 5/9] Add TLS support for Veritas HyperScale (VxHS) block device protocol
On 08/29/2017 02:39 AM, Ashish Mittal wrote:
> The following describes the behavior of TLS for VxHS block device:
>
> (1) Two new options have been added in /etc/libvirt/qemu.conf
> to control TLS behavior with VxHS block devices
> "vxhs_tls" and "vxhs_tls_x509_cert_dir".
> (2) Setting "vxhs_tls=1" in /etc/libvirt/qemu.conf will enable
> TLS for VxHS block devices.
> (3) "vxhs_tls_x509_cert_dir" can be set to the full path where the
> TLS certificates and keys are saved. If this value is missing,
TLS CA certificate and the client certificate and keys are saved.
> the "default_tls_x509_cert_dir" will be used instead.
These three points I think belong in the previous patch commit message...
> (4) If the value of "vxhs_tls" is set to 1, TLS creds will be added
> automatically on the qemu command line for every VxHS
> block device.
> (5) With "vxhs_tls=1", TLS may selectively be disabled on individual
> VxHS disks by specifying tls='no' in the device domain
> specification.
(4) and (5) somewhat contradict each other, but we can work through
this. Additionally, let's use cfg->vxhsTLS or disk->source->haveTLS
What this patch does is use the config->vxhsTLS along with the
introduced disk->src->haveTLS in order to add TLS data to the VxHS
client connection.
> (6) Valid values for domain TLS setting are tls='yes|no'.
s/domain/domain disk source/
> (7) tls='yes' can only be specified if "vxhs_tls" is enabled.
> Specifying tls='yes' when "vxhs_tls=0" results in an error.
I'm still not clear why this is a requirement if in fact you can really
use the default qemu environment.
>
> QEMU changes for VxHS (including TLS support) are already upstream.
>
> Sample TLS args generated by libvirt -
> -object tls-creds-x509,id=objvxhs_tls0,dir=/usr/local/etc/pki/qemu,\
> endpoint=client,verify-peer=yes \
> -drive file.driver=vxhs,file.tls-creds=objvxhs_tls0,\
> file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc251,\
> file.server.host=192.168.0.1,file.server.port=,format=raw,if=none,\
> id=drive-virtio-disk0,cache=none \
> -device virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,\
> id=virtio-disk0
>
> Signed-off-by: Ashish Mittal
> ---
> v5 changelog:
> (1) The v4 3/3 patch has been split into smaller chunks.
> (2) Functionally there are no changes in TLS code yet.
>
> TODO: Changes to TLS functionality are pending.
>
> docs/schemas/domaincommon.rng | 5
> src/conf/domain_conf.c| 19
> src/qemu/qemu_block.c | 42 +++
> src/qemu/qemu_command.c | 67
> +++
> src/util/virstoragefile.c | 13 +
> src/util/virstoragefile.h | 9 ++
> 6 files changed, 143 insertions(+), 12 deletions(-)
>
So this patch is where things are going to get tricky.
On one hand, trying to create a generic means to add/manage a 'tls'
attribute for a disk source needs to be considered; however, this patch
implements it strictly for VxHS, which may not be the best thing.
Thus, I think there needs to be a patch prior to this that adds a
generic/optional "tls" attribute to diskSourceNetworkHost. No one will
use it yet, but it'll be there.
Part of that patch would modify/introduce qemuDomainPrepareDiskSource to
be run after qemuDomainPrepareChardevSource and before
qemuDomainSecretPrepare. It's sole purpose would be to run through the
disk array looking for disks to set up
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 458b8d8..af38c9a 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -1651,6 +1651,11 @@
>
>
>
> +
> +
> +
> +
> +
I think rather than here in the VxHS specific code - perhaps we add tls
to the diskSourceNetworkHost. I need to investigate that. If that's
done, then a patch before this one would be created.
>
>
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 5bad397..f3fb3d0 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -8017,6 +8017,7 @@ virDomainDiskSourceParse(xmlNodePtr node,
> int ret = -1;
> char *protocol = NULL;
> xmlNodePtr saveNode = ctxt->node;
> +char *haveTLS = NULL;
>
> ctxt->node = node;
>
> @@ -8050,6 +8051,19 @@ virDomainDiskSourceParse(xmlNodePtr node,
> goto cleanup;
> }
>
> +/* Check tls=yes|no domain setting for the block device */
> +/* At present only VxHS. Other block devices may be added later */
Adjust the above to be a multi-line comment... e.g.
/* words
* more words */
It's just a consistency thing.
> +if ((haveTLS = virXMLPropString(node, "tls")) &&
> +src->protocol == VIR_STORAGE_NET_PROTOCOL_VXHS) {
flip-flop conditional - no need to check for it for other protocols
(yet)... Besides that's a longer trip -
Re: [libvirt] [PATCH v5 4/9] conf: Introduce TLS options for VxHS block device clients
On 08/29/2017 02:39 AM, Ashish Mittal wrote: > Add a new TLS X.509 certificate type - "vxhs". This will handle the > creation of a TLS certificate capability for properly configured > VxHS network block device clients. > > Signed-off-by: Ashish Mittal > --- > v5 changelog: > (1) Fixed the release version for VxHS changes. > (2) No functional changes. > > v4 changelog: > (1) Add two new options in /etc/libvirt/qemu.conf > to control TLS behavior with VxHS block devices > "vxhs_tls" and "vxhs_tls_x509_cert_dir". > (2) Setting "vxhs_tls=1" in /etc/libvirt/qemu.conf will enable > TLS for VxHS block devices. > (3) "vxhs_tls_x509_cert_dir" can be set to the full path where the > TLS certificates and keys are saved. If this value is missing, > the "default_tls_x509_cert_dir" will be used instead. > > docs/formatdomain.html.in | 16 > src/qemu/libvirtd_qemu.aug | 4 > src/qemu/qemu.conf | 23 +++ > src/qemu/qemu_conf.c | 7 +++ > src/qemu/qemu_conf.h | 3 +++ > src/qemu/test_libvirtd_qemu.aug.in | 2 ++ > 6 files changed, 55 insertions(+) > Starting with this patch - I think there's a bit more work to do... We just need to think logically through things based on the VxHS usage model. > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > index 64397d4..41b4ea8 100644 > --- a/docs/formatdomain.html.in > +++ b/docs/formatdomain.html.in > @@ -2649,6 +2649,22 @@ > transport is "unix", the socket attribute specifies the path to > an > AF_UNIX socket. > > + > +Since 3.8.0, the optional attribute > +tls (QEMU only) can be used to control whether a > vxhs s/(QEMU only) // s/vxhs/VxHS/ > +network block device would utilize a hypervisor configured > +TLS X.509 certificate environment in order to encrypt the data > +channel. For the QEMU hypervisor, usage of a TLS environment can > +be controlled on the host by the vxhs_tls and > +vxhs_tls_x509_cert_dir or > +default_tls_x509_cert_dir settings in the file > +/etc/libvirt/qemu.conf. If vxhs_tls is enabled, > +then unless the domain tls attribute is set to "no", > +libvirt will use the host configured TLS environment. > +If vxhs_tls is disabled, but the tls > +attribute is set to "yes" in the device domain specification, > +then libvirt will throw an error. > + I see this is pretty much a copy of the chardev TLS wording; however, I'm not quite sure the exact same logic can apply in what you've done in this patch. Those last 3 lines may not be necessary - I guess that depends on what happens with how you handle or describe the "default" environment. Is there a reason you've added them and the check in the next patch? BTW: If they stay, it'd be the "domain disk source specification" to be specific. > >snapshot > > diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug > index e1983d1..c19bf3a 100644 > --- a/src/qemu/libvirtd_qemu.aug > +++ b/src/qemu/libvirtd_qemu.aug > @@ -115,6 +115,9 @@ module Libvirtd_qemu = > > let memory_entry = str_entry "memory_backing_dir" > > + let vxhs_entry = bool_entry "vxhs_tls" > + | str_entry "vxhs_tls_x509_cert_dir" > + > (* Each entry in the config is one of the following ... *) > let entry = default_tls_entry > | vnc_entry > @@ -133,6 +136,7 @@ module Libvirtd_qemu = > | nvram_entry > | gluster_debug_level_entry > | memory_entry > + | vxhs_entry > > let comment = [ label "#comment" . del /#[ \t]*/ "# " . store /([^ > \t\n][^\n]*)?/ . del /\n/ "\n" ] > let empty = [ label "#empty" . eol ] > diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf > index f977e3b..0bbcdb2 100644 > --- a/src/qemu/qemu.conf > +++ b/src/qemu/qemu.conf > @@ -258,6 +258,29 @@ > #chardev_tls_x509_secret_uuid = "----" > > > +# Enable use of TLS encryption on the VxHS network block devices. > +# > +# When the VxHS network block device server is set up appropriately, > +# x509 certificates are used for authentication between the clients > +# (qemu processes) and the remote VxHS server. > +# > +# It is necessary to setup CA and issue client and server certificates > +# before enabling this. > +# > +#vxhs_tls = 1 > + > + > +# In order to override the default TLS certificate location for VxHS > +# device TCP certificates, supply a valid path to the certificate directory. > +# This is used to authenticate the VxHS block device clients to the VxHS > +# server. > +# > +# If the provided path does not exist then the default_tls_x509_cert_dir > +# path will be used. > +# > +#vxhs_tls_x509_cert_d
Re: [libvirt] [PATCH v5 2/9] Add a test case to verify generation of qemu command line args for a VxHS disk
On 08/29/2017 02:39 AM, Ashish Mittal wrote:
> Signed-off-by: Ashish Mittal
> ---
> .../qemuxml2argv-disk-drive-network-vxhs.args | 27 +
> .../qemuxml2argv-disk-drive-network-vxhs.xml | 34
> ++
> tests/qemuxml2argvtest.c | 1 +
> 3 files changed, 62 insertions(+)
> create mode 100644
> tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args
> create mode 100644
> tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.xml
>
Could have been merged with previous, but as long as there's tests
that's good.
I'll add a qemuxml2xmltest.c option and a
qemuxml2xmlout-disk-drive-network-vxhs.xml file
There would be an adjustment to the .args output due the "file.server.0"
now instead of just "file.server" and of course the "file.server.0.type
= tcp,". I assume that works with the qemu changes anyway.
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args
> b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args
> new file mode 100644
> index 000..1747dc8
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args
> @@ -0,0 +1,27 @@
> +LC_ALL=C \
> +PATH=/bin \
> +HOME=/home/test \
> +USER=test \
> +LOGNAME=test \
> +QEMU_AUDIO_DRV=none \
> +/usr/bin/qemu-system-x86_64 \
> +-name QEMUGuest1 \
> +-S \
> +-M pc \
> +-cpu qemu32 \
> +-m 214 \
> +-smp 1,sockets=1,cores=1,threads=1 \
> +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
> +-nographic \
> +-nodefaults \
> +-chardev
> socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\
> +server,nowait \
> +-mon chardev=charmonitor,id=monitor,mode=readline \
> +-no-acpi \
> +-boot c \
> +-usb \
> +-drive file.driver=vxhs,file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc251,\
> +file.server.host=192.168.0.1,file.server.port=,format=raw,if=none,\
> +id=drive-virtio-disk0,cache=none \
> +-device virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,\
> +id=virtio-disk0
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.xml
> b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.xml
> new file mode 100644
> index 000..a488770
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.xml
> @@ -0,0 +1,34 @@
> +
> + QEMUGuest1
> + c7a5fdbd-edaf-9455-926a-d65c16db1809
> + 219136
> + 219136
> + 1
> +
> +hvm
> +
> +
> +
> + destroy
> + restart
> + destroy
> +
> +/usr/bin/qemu-system-x86_64
> +
> +
> +
> +
> +
> +
This is unnecessary, I'll remove (it'll cause xml2xml errors too)
> +
> + eb90327c-8302-4725-9e1b-4e85ed4dc251
> +
Likewise, so I'll remove
John
> + function='0x0'/>
> +
> +
> +
> +
> +
> +
> +
> +
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index 5cdbc78..f7dbf39 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -931,6 +931,7 @@ mymain(void)
> # endif
> DO_TEST("disk-drive-network-rbd-ipv6", NONE);
> DO_TEST_FAILURE("disk-drive-network-rbd-no-colon", NONE);
> +DO_TEST("disk-drive-network-vxhs", NONE);
> DO_TEST("disk-drive-no-boot",
> QEMU_CAPS_BOOTINDEX);
> DO_TEST_PARSE_ERROR("disk-device-lun-type-invalid",
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 3/9] Add a test case to verify parsing of VxHS backing storage.
On 08/29/2017 02:39 AM, Ashish Mittal wrote:
> Signed-off-by: Ashish Mittal
> ---
> tests/virstoragetest.c | 10 ++
> 1 file changed, 10 insertions(+)
>
Similarly this probably should be merged with the code that alters
qemu_{command|block}...
> diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
> index d83db78..f8444e1 100644
> --- a/tests/virstoragetest.c
> +++ b/tests/virstoragetest.c
> @@ -1585,6 +1585,16 @@ mymain(void)
> "\n"
> " \n"
> "\n");
> +TEST_BACKING_PARSE("json:{\"file\":{\"driver\":\"vxhs\","
> +
> "\"vdisk-id\":\"c6718f6b-0401-441d-a8c3-1f0064d75ee0\","
> + "\"server\": {
> \"host\":\"example.com\","
> + "\"port\":\"1234\""
> + "}"
> + "}"
> +"}",
> + " name='c6718f6b-0401-441d-a8c3-1f0064d75ee0'>\n"
> + " \n"
> + "\n");
For consistency, I'll modify port to be and I'll add the
"type:tcp," string as well to match previous change
John
> #endif /* WITH_YAJL */
>
> cleanup:
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 1/9] Add support for Veritas HyperScale (VxHS) block device protocol
Probably need to beef this up a little bit... I can figure something out. On 08/29/2017 02:39 AM, Ashish Mittal wrote: > Sample XML for a VxHS disk: > > > > > > > > > eb90327c-8302-4725-9e1b-4e85ed4dc251 > > > > > Signed-off-by: Ashish Mittal > --- > v5 changelog: > (1) Rebased to latest master. > (2) Review comments from Peter Krempa on patch v4 1/3 have been addressed > Not all were, but it's also not something simple to figure out. > v4 changelog: > (1) Fixes per review comments from v3. > (2) Had to remove a test from the previous version that checked for > error when multiple hosts are specified for VxHS device. > This started failing virschematest with the error > "XML document failed to validate against schema" as the > docs/schemas/domain.rng specifies only a single host. > > v3 changelog: > (1) Implemented the modern syntax for VxHS disk specification. > (2) Changed qemuxml2argvdata VxHS test case to verify the new syntax. > (3) Added a negative test case to check failure when multiple hosts > are specified for a VxHS disk. > > v2 changelog: > (1) Added code for JSON parsing of a VxHS vdisk. > (2) Added test case to verify JSON parsing. > (3) Added missing switch-case checks for VIR_STORAGE_NET_PROTOCOL_VXHS. > (4) Fixed line wrap in qemuxml2argv-disk-drive-network-vxhs.args. > > docs/formatdomain.html.in | 15 --- > docs/schemas/domaincommon.rng | 13 ++ > src/libxl/libxl_conf.c| 1 + > src/qemu/qemu_block.c | 60 > +++ > src/qemu/qemu_command.c | 9 +++ > src/qemu/qemu_driver.c| 3 +++ > src/qemu/qemu_parse_command.c | 15 +++ > src/util/virstoragefile.c | 40 - > src/util/virstoragefile.h | 1 + > src/xenconfig/xen_xl.c| 1 + > 10 files changed, 154 insertions(+), 4 deletions(-) > Which email address would be "preferred" once this gets finalized - work or gmail? I don't care either way - just need to know as that becomes part of the git history. The tree is currently "frozen" for the 3.7.0 release, but I think we can at least start adding some adjustments for at least the first few patches once it thaws for 3.8.0. Once the TLS patches start - some more adjustment will be necessary I think - adjustments that you may need to make... I can make changes for the first patches as it's mostly simple things except for the process of capabilities creation... FWIW: To reduce the change in this one patch - I took the liberty of creating a patch that will be inserted before this one that only creates the new symbol and adjusts all the switches appropriately and making you the author. You are missing a patch to set up the capabilities - from Peter's review of v4 patch 1/3 (https://www.redhat.com/archives/libvir-list/2017-June/msg01389.html) "Additionally since libvirt supports QAPI introspection, this means we are now able to detect whether qemu supports vxhs and should report an error if it doesn't. You'll need to add a capability bit in qemu_capabilities.h and the detection string in qemu_capabilities.c to virQEMUCapsQMPSchemaQueries[]." Because libvirt could be run on some host that's not running a QEMU with the VxHS code we have to have a mechanism that does the appropriate checks to ensure the underlying QEMU supports what we're trying to a domain about to be run and generate an appropriate message if it's not present. In any case, all that noted, it seems that this can be accomplished by adding the following line to virQEMUCapsQMPSchemaQueries[]: "blockdev-add/arg-type/+vxhs" But there will also need to be a patch to add 2.10 replies and xml data. Both of these I can do as it's not necessarily intuitively obvious... So... What I'll do is make some adjustments to this series, then repost as a v6 so that you (and others) can look. > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > index fba8cfc..64397d4 100644 > --- a/docs/formatdomain.html.in > +++ b/docs/formatdomain.html.in > @@ -2520,9 +2520,9 @@ > >The protocol attribute specifies the protocol to >access to the requested image. Possible values are "nbd", > - "iscsi", "rbd", "sheepdog" or "gluster". If the > - protocol attribute is "rbd", "sheepdog" or > - "gluster", an additional attribute name is > + "iscsi", "rbd", "sheepdog", "gluster" or "vxhs". If the > + protocol attribute is "rbd", "sheepdog", "gluster" > + or "vxhs", an additional attribute name is >mandatory to specify which volume/image will be used. For > "nbd", >the name attribute is optional. For "iscsi" >(since 1.0.4), the name > @@ -2530,6 +2530,9 @@ >target's name by a slash (e.g., >iqn.2013-07.com.example:iscsi-pool/1). If not >
[libvirt] [PATCH 1/4] conf: use virXMLFormatElement for
Use the new helper to simplify the code.
This also fixes the bug of not formatting 'eim' in the useless
case if it's the only enabled attribute.
---
src/conf/domain_conf.c | 55 --
1 file changed, 31 insertions(+), 24 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index d97aab483..8e555ad9d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -25056,37 +25056,36 @@ virDomainDefIothreadShouldFormat(virDomainDefPtr def)
}
-static void
+static int
virDomainIOMMUDefFormat(virBufferPtr buf,
const virDomainIOMMUDef *iommu)
{
virBuffer childBuf = VIR_BUFFER_INITIALIZER;
+virBuffer driverAttrBuf = VIR_BUFFER_INITIALIZER;
+int ret = -1;
virBufferSetChildIndent(&childBuf, buf);
-if (iommu->intremap != VIR_TRISTATE_SWITCH_ABSENT ||
-iommu->caching_mode != VIR_TRISTATE_SWITCH_ABSENT ||
-iommu->iotlb != VIR_TRISTATE_SWITCH_ABSENT) {
-virBufferAddLit(&childBuf, "intremap != VIR_TRISTATE_SWITCH_ABSENT) {
-virBufferAsprintf(&childBuf, " intremap='%s'",
- virTristateSwitchTypeToString(iommu->intremap));
-}
-if (iommu->caching_mode != VIR_TRISTATE_SWITCH_ABSENT) {
-virBufferAsprintf(&childBuf, " caching_mode='%s'",
-
virTristateSwitchTypeToString(iommu->caching_mode));
-}
-if (iommu->eim != VIR_TRISTATE_SWITCH_ABSENT) {
-virBufferAsprintf(&childBuf, " eim='%s'",
- virTristateSwitchTypeToString(iommu->eim));
-}
-if (iommu->iotlb != VIR_TRISTATE_SWITCH_ABSENT) {
-virBufferAsprintf(&childBuf, " iotlb='%s'",
- virTristateSwitchTypeToString(iommu->iotlb));
-}
-virBufferAddLit(&childBuf, "/>\n");
+if (iommu->intremap != VIR_TRISTATE_SWITCH_ABSENT) {
+virBufferAsprintf(&driverAttrBuf, " intremap='%s'",
+ virTristateSwitchTypeToString(iommu->intremap));
+}
+if (iommu->caching_mode != VIR_TRISTATE_SWITCH_ABSENT) {
+virBufferAsprintf(&driverAttrBuf, " caching_mode='%s'",
+ virTristateSwitchTypeToString(iommu->caching_mode));
+}
+if (iommu->eim != VIR_TRISTATE_SWITCH_ABSENT) {
+virBufferAsprintf(&driverAttrBuf, " eim='%s'",
+ virTristateSwitchTypeToString(iommu->eim));
+}
+if (iommu->iotlb != VIR_TRISTATE_SWITCH_ABSENT) {
+virBufferAsprintf(&driverAttrBuf, " iotlb='%s'",
+ virTristateSwitchTypeToString(iommu->iotlb));
}
+if (virXMLFormatElement(&childBuf, "driver", &driverAttrBuf, NULL) < 0)
+goto cleanup;
+
virBufferAsprintf(buf, "model));
@@ -25097,6 +25096,13 @@ virDomainIOMMUDefFormat(virBufferPtr buf,
} else {
virBufferAddLit(buf, "/>\n");
}
+
+ret = 0;
+
+ cleanup:
+virBufferFreeAndReset(&childBuf);
+virBufferFreeAndReset(&driverAttrBuf);
+return ret;
}
@@ -25866,8 +25872,9 @@ virDomainDefFormatInternal(virDomainDefPtr def,
goto error;
}
-if (def->iommu)
-virDomainIOMMUDefFormat(buf, def->iommu);
+if (def->iommu &&
+virDomainIOMMUDefFormat(buf, def->iommu) < 0)
+goto error;
virBufferAdjustIndent(buf, -2);
virBufferAddLit(buf, "\n");
--
2.13.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/4] conf: validate IOMMU interrupt remapping setting
*** BLURB HERE, THERE AND EVERYWHERE *** Ján Tomko (4): conf: use virXMLFormatElement for conf: use virXMLFormatElement for tests: merge iommu tests conf: validate IOMMU interrupt remapping setting src/conf/domain_conf.c | 85 ++ .../qemuxml2argv-intel-iommu-caching-mode.args | 2 +- .../qemuxml2argv-intel-iommu-caching-mode.xml | 5 +- .../qemuxml2argv-intel-iommu-ioapic.args | 21 -- .../qemuxml2argv-intel-iommu-ioapic.xml| 31 tests/qemuxml2argvtest.c | 6 +- .../qemuxml2xmlout-intel-iommu-ioapic.xml | 1 - tests/qemuxml2xmltest.c| 1 - 8 files changed, 59 insertions(+), 93 deletions(-) delete mode 100644 tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-ioapic.args delete mode 100644 tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-ioapic.xml delete mode 12 tests/qemuxml2xmloutdata/qemuxml2xmlout-intel-iommu-ioapic.xml -- 2.13.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/4] conf: use virXMLFormatElement for
Simplify the formatting function even further.
---
src/conf/domain_conf.c | 12
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 8e555ad9d..a26731e75 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -25061,6 +25061,7 @@ virDomainIOMMUDefFormat(virBufferPtr buf,
const virDomainIOMMUDef *iommu)
{
virBuffer childBuf = VIR_BUFFER_INITIALIZER;
+virBuffer attrBuf = VIR_BUFFER_INITIALIZER;
virBuffer driverAttrBuf = VIR_BUFFER_INITIALIZER;
int ret = -1;
@@ -25086,16 +25087,11 @@ virDomainIOMMUDefFormat(virBufferPtr buf,
if (virXMLFormatElement(&childBuf, "driver", &driverAttrBuf, NULL) < 0)
goto cleanup;
-virBufferAsprintf(buf, "model));
-if (virBufferError(&childBuf) != 0 || virBufferUse(&childBuf)) {
-virBufferAddLit(buf, ">\n");
-virBufferAddBuffer(buf, &childBuf);
-virBufferAddLit(buf, "\n");
-} else {
-virBufferAddLit(buf, "/>\n");
-}
+if (virXMLFormatElement(buf, "iommu", &attrBuf, &childBuf) < 0)
+goto cleanup;
ret = 0;
--
2.13.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 4/4] conf: validate IOMMU interrupt remapping setting
This option requires:
Report an error in case someone tries to combine
it with different ioapic setting.
Setting 'eim' on without enabling 'intremap' does not make sense.
https://bugzilla.redhat.com/show_bug.cgi?id=1457610
---
src/conf/domain_conf.c | 18 ++
1 file changed, 18 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index a26731e75..fd236f5d2 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5551,6 +5551,24 @@ virDomainDefValidateInternal(const virDomainDef *def)
if (virDomainDefGetVcpusTopology(def, NULL) < 0)
return -1;
+if (def->iommu &&
+def->iommu->intremap == VIR_TRISTATE_SWITCH_ON &&
+(def->features[VIR_DOMAIN_FEATURE_IOAPIC] != VIR_TRISTATE_SWITCH_ON ||
+ def->ioapic != VIR_DOMAIN_IOAPIC_QEMU)) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("IOMMU interrupt remapping requires split I/O APIC "
+ "(ioapic driver='qemu')"));
+return -1;
+}
+
+if (def->iommu &&
+def->iommu->eim == VIR_TRISTATE_SWITCH_ON &&
+def->iommu->intremap != VIR_TRISTATE_SWITCH_ON) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("IOMMU eim requires interrupt remapping to be
enabled"));
+return -1;
+}
+
return 0;
}
--
2.13.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/4] tests: merge iommu tests
Using intremap without does not work.
Merge the tests to avoid a duplicit test once we start validating it.
---
.../qemuxml2argv-intel-iommu-caching-mode.args | 2 +-
.../qemuxml2argv-intel-iommu-caching-mode.xml | 5 +++-
.../qemuxml2argv-intel-iommu-ioapic.args | 21 ---
.../qemuxml2argv-intel-iommu-ioapic.xml| 31 --
tests/qemuxml2argvtest.c | 6 +
.../qemuxml2xmlout-intel-iommu-ioapic.xml | 1 -
tests/qemuxml2xmltest.c| 1 -
7 files changed, 6 insertions(+), 61 deletions(-)
delete mode 100644 tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-ioapic.args
delete mode 100644 tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-ioapic.xml
delete mode 12
tests/qemuxml2xmloutdata/qemuxml2xmlout-intel-iommu-ioapic.xml
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-caching-mode.args
b/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-caching-mode.args
index 5d12aabf4..81feecfcf 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-caching-mode.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-caching-mode.args
@@ -7,7 +7,7 @@ QEMU_AUDIO_DRV=none \
/usr/bin/qemu-system-x86_64 \
-name QEMUGuest1 \
-S \
--machine q35,accel=tcg \
+-machine q35,accel=kvm,kernel_irqchip=split \
-m 214 \
-smp 1,sockets=1,cores=1,threads=1 \
-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-caching-mode.xml
b/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-caching-mode.xml
index 5f3384da7..36a392403 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-caching-mode.xml
+++ b/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-caching-mode.xml
@@ -1,4 +1,4 @@
-
+
QEMUGuest1
c7a5fdbd-edaf-9455-926a-d65c16db1809
219100
@@ -8,6 +8,9 @@
hvm
+
+
+
destroy
restart
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-ioapic.args
b/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-ioapic.args
deleted file mode 100644
index 8e70bf910..0
--- a/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-ioapic.args
+++ /dev/null
@@ -1,21 +0,0 @@
-LC_ALL=C \
-PATH=/bin \
-HOME=/home/test \
-USER=test \
-LOGNAME=test \
-QEMU_AUDIO_DRV=none \
-/usr/bin/qemu-system-x86_64 \
--name QEMUGuest1 \
--S \
--machine q35,accel=kvm,kernel_irqchip=split \
--m 214 \
--smp 1,sockets=1,cores=1,threads=1 \
--uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
--nographic \
--nodefaults \
--chardev
socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\
-server,nowait \
--mon chardev=charmonitor,id=monitor,mode=readline \
--no-acpi \
--boot c \
--device intel-iommu,intremap=on
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-ioapic.xml
b/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-ioapic.xml
deleted file mode 100644
index bfe714ad8..0
--- a/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-ioapic.xml
+++ /dev/null
@@ -1,31 +0,0 @@
-
- QEMUGuest1
- c7a5fdbd-edaf-9455-926a-d65c16db1809
- 219100
- 219100
- 1
-
-hvm
-
-
-
-
-
-
- destroy
- restart
- destroy
-
-/usr/bin/qemu-system-x86_64
-
-
-
-
-
-
-
-
-
-
-
-
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 0c0bd16f9..3a0080297 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -2699,14 +2699,10 @@ mymain(void)
DO_TEST("intel-iommu-machine",
QEMU_CAPS_MACHINE_OPT,
QEMU_CAPS_MACHINE_IOMMU);
-DO_TEST("intel-iommu-ioapic",
+DO_TEST("intel-iommu-caching-mode",
QEMU_CAPS_MACHINE_OPT,
QEMU_CAPS_MACHINE_KERNEL_IRQCHIP,
QEMU_CAPS_MACHINE_KERNEL_IRQCHIP_SPLIT,
-QEMU_CAPS_INTEL_IOMMU_INTREMAP,
-QEMU_CAPS_DEVICE_INTEL_IOMMU);
-DO_TEST("intel-iommu-caching-mode",
-QEMU_CAPS_MACHINE_OPT,
QEMU_CAPS_DEVICE_PCI_BRIDGE,
QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
QEMU_CAPS_DEVICE_IOH3420,
diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-intel-iommu-ioapic.xml
b/tests/qemuxml2xmloutdata/qemuxml2xmlout-intel-iommu-ioapic.xml
deleted file mode 12
index 42d17b2c0..0
--- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-intel-iommu-ioapic.xml
+++ /dev/null
@@ -1 +0,0 @@
-../qemuxml2argvdata/qemuxml2argv-intel-iommu-ioapic.xml
\ No newline at end of file
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 311b71356..de8a781c1 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -1187,7 +1187,6 @@ mymain(void)
DO_TEST("intel-iommu-machine",
QEMU_CAPS_MACHINE_OPT,
QEMU_CAPS_MACHINE_IOMMU);
-DO_TEST("intel-iommu-ioapic", NONE);
DO_TEST("intel-iommu-caching-mode", NONE);
DO_TEST("intel-iommu-eim", NONE);
DO_TEST("intel-iommu-device-iotlb", NONE);
--
2.13.0
--
libvir-list maili
[libvirt] [PATCH] lxcStateInitialize: Don't leak driver's caps
Funny thing. So when initializing LXC driver's capabilities,
firstly the virLXCDriverGetCapabilities() is called. This creates
new capabilities, stores them under driver->caps, ref() them and
return them. However, the return value is ignored. Secondly, the
function is called yet again and since we have driver->caps set,
they are ref()-ed again an returned. So in the end, driver's
capabilities have refcount of three when in fact they should have
refcount of one.
Signed-off-by: Michal Privoznik
---
src/lxc/lxc_driver.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 6eb88b0ba..784edad39 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -1660,7 +1660,7 @@ static int lxcStateInitialize(bool privileged,
if (!(lxc_driver->hostdevMgr = virHostdevManagerGetDefault()))
goto cleanup;
-if ((virLXCDriverGetCapabilities(lxc_driver, true)) == NULL)
+if (!(caps = virLXCDriverGetCapabilities(lxc_driver, true)))
goto cleanup;
if (!(lxc_driver->xmlopt = lxcDomainXMLConfInit()))
@@ -1669,9 +1669,6 @@ static int lxcStateInitialize(bool privileged,
if (!(lxc_driver->closeCallbacks = virCloseCallbacksNew()))
goto cleanup;
-if (!(caps = virLXCDriverGetCapabilities(lxc_driver, false)))
-goto cleanup;
-
if (virFileMakePath(cfg->stateDir) < 0) {
virReportSystemError(errno,
_("Failed to mkdir %s"),
@@ -1700,6 +1697,7 @@ static int lxcStateInitialize(bool privileged,
goto cleanup;
virNWFilterRegisterCallbackDriver(&lxcCallbackDriver);
+virObjectUnref(caps);
return 0;
cleanup:
--
2.13.5
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] Fix TLS test suites with gnutls 3.6.0
With gnutls 3.6.0, SHA1 is no longer accepted for certificate
signatures. We must usw SHA256 instead.
Signed-off-by: Daniel P. Berrange
---
tests/virnettlshelpers.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/virnettlshelpers.c b/tests/virnettlshelpers.c
index 531d0b907..b735c4e2f 100644
--- a/tests/virnettlshelpers.c
+++ b/tests/virnettlshelpers.c
@@ -384,7 +384,7 @@ testTLSGenerateCert(struct testTLSCertReq *req,
* If no 'ca' is set then we are self signing
* the cert. This is done for the root CA certs
*/
-if ((err = gnutls_x509_crt_sign(crt, ca ? ca : crt, privkey)) < 0) {
+if ((err = gnutls_x509_crt_sign2(crt, ca ? ca : crt, privkey,
GNUTLS_DIG_SHA256, 0)) < 0) {
VIR_WARN("Failed to sign certificate %s", gnutls_strerror(err));
abort();
}
--
2.13.5
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 REBASE 12/16] qemu: migrate: show disks stats on job info requests
On Thu, Aug 24, 2017 at 09:56:49 +0300, Nikolay Shirokovskiy wrote:
> This patch shows incorrect info when client request comes
> when migration routine is stopping mirror jobs or mirror
> jobs already stopped. This issue will be addressed in next
> patch.
...
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 2a8a721..c7af1ac 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -5922,3 +5922,52 @@ qemuMigrationReset(virQEMUDriverPtr driver,
> virFreeError(err);
> }
> }
> +
> +
> +int
> +qemuMigrationFetchMirrorStats(virQEMUDriverPtr driver,
> + virDomainObjPtr vm,
> + qemuDomainAsyncJob asyncJob,
> + qemuMonitorMigrationStatsPtr stats)
> +{
> +size_t i;
> +qemuDomainObjPrivatePtr priv = vm->privateData;
> +bool nbd = false;
> +virHashTablePtr blockinfo = NULL;
> +
> +for (i = 0; i < vm->def->ndisks; i++) {
> +virDomainDiskDefPtr disk = vm->def->disks[i];
> +if (QEMU_DOMAIN_DISK_PRIVATE(disk)->migrating) {
> +nbd = true;
> +break;
> +}
> +}
> +
> +if (!nbd)
> +return 0;
> +
> +if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
> +return -1;
> +
> +blockinfo = qemuMonitorGetAllBlockJobInfo(priv->mon);
> +
> +if (qemuDomainObjExitMonitor(driver, vm) < 0 || !blockinfo)
> +return -1;
> +
> +for (i = 0; i < vm->def->ndisks; i++) {
> +virDomainDiskDefPtr disk = vm->def->disks[i];
> +qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
> +qemuMonitorBlockJobInfoPtr data;
> +
> +if (!diskPriv->migrating ||
> +!(data = virHashLookup(blockinfo, disk->info.alias)))
> +continue;
> +
> +stats->disk_transferred += data->cur;
> +stats->disk_total += data->end;
> +stats->disk_remaining += data->end - data->cur;
You should not touch qemuMonitorMigrationStatsPtr here. The scructure is
used to store raw data from query_migrate QMP command. Just add another
structure to jobInfo for storing NBD migration statistics. If you do
this, you don't need the hack to conditionally clear
qemuMonitorMigrationStats. Both structs can be combined in
qemuDomainJobInfoTo*.
Jirka
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] rpc: avoid ssh interpreting malicious hostname as arguments
On 08/29/2017 04:14 PM, Daniel P. Berrange wrote:
> Inspired by the recent GIT / Mercurial security flaws
> (http://blog.recurity-labs.com/2017-08-10/scm-vulns),
> consider someone/something manages to feed libvirt a bogus
> URI such as:
Yeah, I was wondering this myself when reading the news on my PTO but
you beat me to it :-)
>
> virsh -c qemu+ssh://-oProxyCommand=gnome-calculator/system
>
> In this case, the hosname "-oProxyCommand=gnome-calculator"
> will get interpreted as an argument to ssh, not a hostname.
> Fortunately, due to the set of args we have following the
> hostname, SSH will then interpret our bit of shell script
> that runs 'nc' on the remote host as a cipher name, which is
> clearly invalid. This makes ssh exit during argv parsing and
> so it never tries to run gnome-calculator.
>
> We are lucky this time, but lets be more paranoid, by using
> '--' to explicitly tell SSH when it has finished seeing
> command line options. This forces it to interpret
> "-oProxyCommand=gnome-calculator" as a hostname, and thus
> see a fail from hostname lookup.
>
> Signed-off-by: Daniel P. Berrange
> ---
> src/rpc/virnetsocket.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
> index d228c8a8c..23089afef 100644
> --- a/src/rpc/virnetsocket.c
> +++ b/src/rpc/virnetsocket.c
> @@ -868,7 +868,7 @@ int virNetSocketNewConnectSSH(const char *nodename,
> if (!netcat)
> netcat = "nc";
>
> -virCommandAddArgList(cmd, nodename, "sh", "-c", NULL);
> +virCommandAddArgList(cmd, "--", nodename, "sh", "-c", NULL);
>
> virBufferEscapeShell(&buf, netcat);
> if (virBufferCheckError(&buf) < 0) {
>
ACK and safe for the freeze.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH for 3.7.0] docs: Document yet another limitation of tx_queue_size
https://bugzilla.redhat.com/show_bug.cgi?id=1484234 Turns out, only vhostuser type of interfaces are supported currently. Signed-off-by: Michal Privoznik --- I'd like to get this one in since the whole feature is being introduced in this release. docs/formatdomain.html.in | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 19e869f12..8ca7637a4 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -5213,7 +5213,10 @@ qemu-kvm -net nic,model=? /dev/null The default value is hypervisor dependent and may change across its releases. Moreover, some hypervisors may pose some restrictions on actual value. For instance, QEMU -v2.9 requires value to be a power of two from [256, 1024] range. +v2.9 requires value to be a power of two from [256, 1024] +range. In addition to that, this may work only for a subset of +interface types, e.g. aforementioned QEMU enables this option +only for vhostuser type. Since 3.7.0 (QEMU and KVM only) In general you should leave this option alone, unless you -- 2.13.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 REBASE 11/16] qemu: always get job condition on getting job stats
On Thu, Aug 24, 2017 at 09:56:48 +0300, Nikolay Shirokovskiy wrote:
> Looks like it is more simple to drop this optimization as we are
> going to add getting disks stats during migration via quering qemu
> process and checking if we have to acquire job condition becomes
> more complicate.
...
> @@ -12947,7 +12942,8 @@ qemuDomainGetJobStatsInternal(virQEMUDriverPtr driver,
> if (jobInfo->status == QEMU_DOMAIN_JOB_STATUS_ACTIVE ||
> jobInfo->status == QEMU_DOMAIN_JOB_STATUS_MIGRATING ||
> jobInfo->status == QEMU_DOMAIN_JOB_STATUS_POSTCOPY) {
> -if (fetch &&
> +
Remove the empty line here
> +if (events && jobInfo->status != QEMU_DOMAIN_JOB_STATUS_ACTIVE &&
and break this line after the first "&&".
> qemuMigrationFetchMigrationStats(driver, vm, QEMU_ASYNC_JOB_NONE,
> &jobInfo->stats, false) < 0)
> goto cleanup;
ACK with the trivial change.
Jirka
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 REBASE 10/16] qemu: introduce migrating job status
On Thu, Aug 24, 2017 at 09:56:47 +0300, Nikolay Shirokovskiy wrote: > Instead of checking stat.status let's set status to migrating > as soon as migrate command is send (waiting for completion > is a good place too). > --- > src/qemu/qemu_domain.c| 1 + > src/qemu/qemu_domain.h| 1 + > src/qemu/qemu_driver.c| 4 +++- > src/qemu/qemu_migration.c | 9 +++-- > 4 files changed, 12 insertions(+), 3 deletions(-) ACK Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 REBASE 09/16] qemu: start all async job with job status active
On Thu, Aug 24, 2017 at 09:56:46 +0300, Nikolay Shirokovskiy wrote: > Setting status to none has little value - getting job status > will not return even elapsed time. > > After this patch getting job stats stays correct in a sence > it will not fetch migration stats because it consults > stats.status before doing the fetch. > --- > src/qemu/qemu_domain.c| 1 + > src/qemu/qemu_driver.c| 2 -- > src/qemu/qemu_migration.c | 4 > src/qemu/qemu_process.c | 4 > 4 files changed, 1 insertion(+), 10 deletions(-) Makes sense. ACK Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] rpc: avoid ssh interpreting malicious hostname as arguments
Inspired by the recent GIT / Mercurial security flaws
(http://blog.recurity-labs.com/2017-08-10/scm-vulns),
consider someone/something manages to feed libvirt a bogus
URI such as:
virsh -c qemu+ssh://-oProxyCommand=gnome-calculator/system
In this case, the hosname "-oProxyCommand=gnome-calculator"
will get interpreted as an argument to ssh, not a hostname.
Fortunately, due to the set of args we have following the
hostname, SSH will then interpret our bit of shell script
that runs 'nc' on the remote host as a cipher name, which is
clearly invalid. This makes ssh exit during argv parsing and
so it never tries to run gnome-calculator.
We are lucky this time, but lets be more paranoid, by using
'--' to explicitly tell SSH when it has finished seeing
command line options. This forces it to interpret
"-oProxyCommand=gnome-calculator" as a hostname, and thus
see a fail from hostname lookup.
Signed-off-by: Daniel P. Berrange
---
src/rpc/virnetsocket.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
index d228c8a8c..23089afef 100644
--- a/src/rpc/virnetsocket.c
+++ b/src/rpc/virnetsocket.c
@@ -868,7 +868,7 @@ int virNetSocketNewConnectSSH(const char *nodename,
if (!netcat)
netcat = "nc";
-virCommandAddArgList(cmd, nodename, "sh", "-c", NULL);
+virCommandAddArgList(cmd, "--", nodename, "sh", "-c", NULL);
virBufferEscapeShell(&buf, netcat);
if (virBufferCheckError(&buf) < 0) {
--
2.13.5
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 REBASE 08/16] qemu: fail querying destination migration statistics always
On Thu, Aug 24, 2017 at 09:56:45 +0300, Nikolay Shirokovskiy wrote: > Querying destination migration statistics may result in getting > a failure or getting a elapsed time value depending on stats.status > value which is odd. Instead let's always fail. Clients should > be ready to handle this as currently getting failure period > can be considerable. Alternatively, we could always return elapsed time only. ACK Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Entering freeze for libvirt-3.7.0
On Tue, Aug 29, 2017 at 02:12:56PM +0200, Daniel Veillard wrote: > As suggested yesterday (sorry for short notice) I have just tagged the > rc1 in git and pushed signed tarball and rpms to the usual place: > > ftp://libvirt.org/libvirt/ > > Things looks fine for me, jenkins on https://ci.centos.org/view/libvirt/ > seems mostly happy (though there is that master rpm weirdness) so it It seems our test suite for TLS is broken by gnutls 3.6.0 that just hit fedora rawhide. IIUC, we are generating certs using the "NORMAL" gnutls priority (which defaults to SHA1 signature alg), but we're trying to verify with "SYSTEM" priority (which forbids SHA1 signature alg). Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 REBASE 07/16] qemu: simplify getting completed job stats
On Thu, Aug 24, 2017 at 09:56:44 +0300, Nikolay Shirokovskiy wrote:
> ---
> src/qemu/qemu_driver.c | 26 --
> 1 file changed, 12 insertions(+), 14 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index c62d416..b8a4df7 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -12905,12 +12905,18 @@ qemuDomainGetJobStatsInternal(virQEMUDriverPtr
> driver,
>qemuDomainJobInfoPtr jobInfo)
> {
> qemuDomainObjPrivatePtr priv = vm->privateData;
> -qemuDomainJobInfoPtr info;
> bool fetch = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT);
> int ret = -1;
>
> -if (completed)
> -fetch = false;
> +if (completed) {
> +if (priv->job.current || !priv->job.completed) {
> +jobInfo->status = QEMU_DOMAIN_JOB_STATUS_NONE;
> +return 0;
> +}
> +
> +*jobInfo = *priv->job.completed;
> +return 0;
I think keeping just one return would make the code a bit easier to
read:
if (priv->job.current || !priv->job.completed)
jobInfo->status = QEMU_DOMAIN_JOB_STATUS_NONE;
else
*jobInfo = *priv->job.completed;
return 0;
Or even
if (priv->job.completed && !priv->job.current)
*jobInfo = *priv->job.completed;
else
jobInfo->status = QEMU_DOMAIN_JOB_STATUS_NONE;
return 0;
> +}
>
> /* Do not ask QEMU if migration is not even running yet */
> if (!priv->job.current || !priv->job.current->stats.status)
...
Jirka
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 REBASE 06/16] qemu: refactor fetching migration stats
On Thu, Aug 24, 2017 at 09:56:43 +0300, Nikolay Shirokovskiy wrote:
> qemuMigrationFetchJobStatus is rather inconvinient. Some of its
> callers don't need status to be updated, some don't need to update
> elapsed time right away. So let's update status or elapsed time
> in callers instead.
>
> In qemuMigrationConfirmPhase we should fetch stats with copy
> flag set as stats variable refers to domain object not the stack.
>
> This patch drops updating job status on getting job stats by
> client. This way we will not provide status 'completed' while
> it is not yet updated by migration routine.
...
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index cc42f7a..dec0a08 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -1376,24 +1376,28 @@ qemuMigrationUpdateJobType(qemuDomainJobInfoPtr
> jobInfo)
>
>
> int
> -qemuMigrationFetchJobStatus(virQEMUDriverPtr driver,
> -virDomainObjPtr vm,
> -qemuDomainAsyncJob asyncJob,
> -qemuDomainJobInfoPtr jobInfo)
> +qemuMigrationFetchMigrationStats(virQEMUDriverPtr driver,
The name contains "Migration" twice. How about qemuMigrationFetchStats
or qemuMigrationFetchJobStats?
> + virDomainObjPtr vm,
> + qemuDomainAsyncJob asyncJob,
> + qemuMonitorMigrationStatsPtr stats,
It looks like all callers are always passing something like
&jobInfo->stats so keeping qemuDomainJobInfoPtr jobInfo argument could
make the callers a bit simpler.
> + bool copy)
I'd just drop the "copy" parameter completely and let the function
always fetch stats in a local variable and then copy its content into
the "stats" argument. I.e., make it always work as if copy == true.
> {
> qemuDomainObjPrivatePtr priv = vm->privateData;
> +qemuMonitorMigrationStats statsCopy;
> int rv;
>
> if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
> return -1;
>
> -rv = qemuMonitorGetMigrationStats(priv->mon, &jobInfo->stats);
> +rv = qemuMonitorGetMigrationStats(priv->mon, copy ? &statsCopy : stats);
>
> if (qemuDomainObjExitMonitor(driver, vm) < 0 || rv < 0)
> return -1;
>
> -qemuMigrationUpdateJobType(jobInfo);
> -return qemuDomainJobInfoUpdateTime(jobInfo);
> +if (copy)
> +*stats = statsCopy;
> +
> +return 0;
> }
>
>
...
> @@ -1442,11 +1429,12 @@ qemuMigrationCheckJobStatus(virQEMUDriverPtr driver,
>
> bool events = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT);
>
> -if (events)
> -qemuMigrationUpdateJobType(jobInfo);
> -else if (qemuMigrationUpdateJobStatus(driver, vm, asyncJob) < 0)
> +if (!events && qemuMigrationFetchMigrationStats(driver, vm, asyncJob,
Break the line after &&, please.
> +&jobInfo->stats, true) <
> 0)
> return -1;
>
> +qemuMigrationUpdateJobType(jobInfo);
> +
> switch (jobInfo->status) {
> case QEMU_DOMAIN_JOB_STATUS_NONE:
> virReportError(VIR_ERR_OPERATION_FAILED, _("%s: %s"),
...
Jirka
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 6/6] nodedev: Work around the uevent race by hooking up virFileWaitForAccess
On Mon, Aug 28, 2017 at 12:40:44PM -0400, John Ferlan wrote:
>
>
> On 08/24/2017 07:23 AM, Erik Skultety wrote:
> > If we find ourselves in the situation that the 'add' uevent has been
> > fired earlier than the sysfs tree for a device was created, we should
> > use the best-effort approach and give kernel some predetermined amount
> > of time, thus waiting for the attributes to be ready rather than
> > discarding the device from our device list forever. If those don't appear
> > in the given time frame, we need to move on, since libvirt can't wait
> > indefinitely.
> >
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1463285
> >
> > Signed-off-by: Erik Skultety
> > ---
> > src/node_device/node_device_udev.c | 16 +++-
> > 1 file changed, 15 insertions(+), 1 deletion(-)
> >
>
> IIRC - I pointed out to you that this is eerily familiar to something
> that happens in the vHBA code w/r/t to wwnn/wwpn files. Except that the
> files exist, but have a -1 in them which is totally bogus. Then some
> magic thing happens and the real wwnn/wwpn is placed into the file, but
> libvirt already looked and failed. When I tried to work around this the
> decision was to let it be and call it a kernel / udev bug.
>
> https://www.redhat.com/archives/libvir-list/2016-June/msg02213.html
>
> and Daniel's answer
>
> https://www.redhat.com/archives/libvir-list/2016-July/msg00912.html
>
> Although yes, with the other changes in place one would think having a
> wait is no big deal.
>
> Still are you guaranteed that once the file exists that the data within
> the file is valid? In the vHBA case it wasn't and that led to issues.
Yes, I recall you pointing me to this issue before and you're right that if the
data is bogus, we can't do much about that, except that in this case, I'm only
relying on the existence of the file/dir, because I need its name to determine
the mediated device's type, not its content, which arguably makes it a different
problem.
Erik
>
> I'd "use this" processing instead of the hack I proposed as well seeing
> as it doesn't seem kernel/udev fixing issues such as these is on any
> priority list /-{
Exactly ^this :(
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 4/6] nodedev: Disable/re-enable polling on the udev fd
On Mon, Aug 28, 2017 at 12:38:08PM -0400, John Ferlan wrote: > > > On 08/24/2017 07:23 AM, Erik Skultety wrote: > > The event loop may get scheduled earlier than the udev event handler > > thread which means that it would keep invoking the handler callback with > > "new" events, while in fact it's most likely still the same event which > > the handler thread hasn't managed to remove from the socket queue yet. > > This is due to unwanted increments of the number of events queuing on the > > socket and causes the handler thread to spam the logs with an error about > > libudev returning NULL (errno == EAGAIN). > > > > Signed-off-by: Erik Skultety > > --- > > src/node_device/node_device_udev.c | 17 + > > 1 file changed, 17 insertions(+) > > > > And by disabling the polling couldn't we miss an event? That would be > really bad if the event after the one we miss relies on the event we > missed creating something that the subsequent one would need (if that > makes sense). No, we just don't poll on the @fd for a moment, but the data has kept being delivered to the udev monitor and waits for us to be extracted. Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 3/6] udev: Convert udevEventHandleThread to an actual thread routine
On Mon, Aug 28, 2017 at 12:26:08PM -0400, John Ferlan wrote: > > > On 08/24/2017 07:23 AM, Erik Skultety wrote: > > Adjust udevEventHandleThread to be a proper thread routine running in an > > infinite loop handling devices. Also introduce udevEventThreadData > > private structure. > > Every time there's and incoming event from udev, udevEventHandleCallback > > only increments the number of events queuing on the monitor and signals > > the worker thread to handle them. > > > > Signed-off-by: Erik Skultety > > --- > > src/node_device/node_device_udev.c | 132 > > ++--- > > 1 file changed, 108 insertions(+), 24 deletions(-) > > > > Once we get here I'm not sure I understand the udev interaction. I guess > it's not "crystal clear" that the socket relationship is a queue of > device events. I also haven't studied the udev code nor have I been > working through this as much as you have lately - so perhaps something > you've uncovered will help educate me in this manner. Still here's my > thoughts and a small sliver of investigative data... > > So far there's been a 1-to-1 interaction between libvirt and udev event. > With this change there could be an n-to-1 interaction - as in receive @n > devices. > > Up to this point the interaction has been: > > 1. udev event > 2. validate fd/udev_monitor > 3. call udev_monitor_receive_device to get @device > 4. process @device > 5. unref @device > 6. return to udev > > After this point > > 1. udev event > 2. validate fd/udev_monitor > 3. increase event count > 4. signal condition to thread to wakeup > 5. return to udev > > asynchronously in a loop > > 1. wakeup on condition and for each 'event' refcnt > 2. decrease event refcnt > 3. validate fd/udev_monitor > 4. call udev_monitor_receive_device to get @device > 5. process @device > 6. unref @device > > If there's more than one event, does udev_monitor_receive_device > guarantee the order? Are we sure udev buffers/queues in this manner? This should be fairly deterministic as it's not that much of a udev problem as it is a kernel problem (since we're talking about a netlink socket which is internally represented by a 'struct socket' structure in the kernel which in fact implements socket buffers, more specifically - very important - queues). So, if kernel didn't guarantee the order of packets/events on the socket, you'd experience the same problem with calling udev_monitor_receive_device, which basically only does recvmsg(), directly from udevEventHandleCallback. IOW if that was the case, udev itself could get easily confused by the kernel events about a device being deleted that had not been previously added etc. So, yes, the order has to be guaranteed and if it is not, then it's a kernel bug. > That is since we're not grabbing @device before we return to udev when > the event is signaled, is there any guarantee from udev that *the same* > device will still exist when we do get around to making our call? My > concern being how "long" is the data guaranteed to be there. As I said, unless the socket buffer size limit has been reached, in which case ENOSPC was returned by libudev - I had a BZ on this: https://bugzilla.redhat.com/show_bug.cgi?id=1450960 > > As I read the subsequent patch - I'm thinking perhaps we really want to > keep that 1-to-1 relationship. Could the reason that you're getting > those messages be because we're now calling udev out of the order it > expected? Do we know why we're getting a NULL return? I found: Yeah, I was getting EAGAIN and it wasn't until that point that I realized that the scheduler came into play, IOW the main event loop thread polls the file descriptor list, invoking the corresponding handlers, which means that udevEventHandleCallback just increased the overall number of events waiting to be processed by the handler thread, except that since no one actually extracted the data out of the socket and the thread hadn't been scheduled in the meantime it's still the same data that the main event loop thread finds waiting on the socket the next time it polls the @fd, therefore incrementing the event counter multiple times for the same event. Naturally, when the handler thread finally removes the event from the monitor, not knowing the counter had incremented for this event multiple times, it will fail to remove any more events from the monitor, as none had arrived in the meantime, therefore the EAGAIN. > > https://sourcecodebrowser.com/udev/141/libudev-monitor_8c.html > > which seems to indicate a number of reasons to return NULL... Maybe udev > is processing another device and while doing so it blocks anyone calling > udev_monitor_receive_device. Conversely while processing an event that > same block wouldn't be present because the expectation is that the > called function will call udev_monitor_receive_device. If this was the case, sooner or later we'd hit the problem anyway. > > I think perhaps the processing thread is fine to have; however, instead
Re: [libvirt] New QEMU daemon for persistent reservations
On Tue, Aug 22, 2017 at 06:27:40PM +0200, Paolo Bonzini wrote: > Hi all, > > I am adding a new daemon to QEMU, that QEMU can connect to in order to > issue persistent reservation commands. Can you elaborate on what this daemon does ? IIUC, by 'persistent reservation' you are referring to SCSI LUN reservations ? If so, the security model needs to involve more than just policy on the socket that QEMU uses to talk to the PR daemon. We need to be able to control what device nodes this daemon is permitted to access. For iSCSI backed disks, the daemon might need to use the libiscsi driver, instead of assuming there is a device node on the local host too. Libvirt has a generic lock manager facility and I've previously though might be able to be extended to acquire SCSI reservations for disks which are backed by SCSI/iSCSI, but never tried to work on this. > The daemon can only issue the commands on file descriptor that QEMU > already has. In addition normal users shouldn't have access to the > daemon's Unix socket in /run, so the daemon is protected against misuse. > > My question is what is the best way to handle the connection to the > daemon socket. Currently, the path to the socket is passed to QEMU on > the command line: > > -object pr-manager-helper,id=mgr,path=/run/qemu-pr-helper.sock \ > -drive if=none,id=hd,driver=raw,filename=/dev/sdb,file.pr-manager=mgr \ > -device scsi-block,drive=hd > > (the new parts are "-object pr-manager-helper" and "file.pr-manager"). I'm not sure if want to have QEMU spawning this daemon itself or not. On the one hand if QEMU spawns the daemon, then it means the daemon inherits the SELinux policy of QEMU by default, so is restricted to only access the devices that QEMU has been granted. On the other hand, this means we need to give QEMU permission to execve(), which is something we've been striving to remove by default. > I could just make it root:root and pass a file descriptor from libvirt > to QEMU, but this would make it impossible for QEMU to reconnect to the > daemon in case someone does a "systemctl restart" or even just kills it > inadvertently. The daemon is stateless, so transparent reconnection > would be a nice feature to have. > > The alternative is to somehow label the daemon socket so that it can be > accessed by QEMU, but I'm not very well versed in SELinux. > > Any ideas? Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] Entering freeze for libvirt-3.7.0
As suggested yesterday (sorry for short notice) I have just tagged the rc1 in git and pushed signed tarball and rpms to the usual place: ftp://libvirt.org/libvirt/ Things looks fine for me, jenkins on https://ci.centos.org/view/libvirt/ seems mostly happy (though there is that master rpm weirdness) so it looks good from my side but my own testing is very limited so please give it a try ! If everything goes well I will push RC2 on Thursday with hopefully a final release over the week end. please give it some testing, thanks, Daniel -- Daniel Veillard | Red Hat Developers Tools http://developer.redhat.com/ veill...@redhat.com | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | virtualization library http://libvirt.org/ -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 2/2] qemu: Also treat directories properly when using namespaces
On 08/29/2017 10:34 AM, Martin Kletzander wrote:
> When recreating folders with namespaces, the directory type was not
> being handled at all. It's not special, we probably just didn't know
> that that can be used as a volume path as well. The code failed
> gracefully, but we want to allow that so that we can use type='dir'> in domains again.
>
> Partially-resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1443434
>
> Signed-off-by: Martin Kletzander
> ---
> src/qemu/qemu_domain.c | 40 +++-
> 1 file changed, 35 insertions(+), 5 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 2c77a6442467..2549f9bf3290 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -7855,6 +7855,7 @@ qemuDomainCreateDeviceRecursive(const char *device,
> bool isLink = false;
> bool isDev = false;
> bool isReg = false;
> +bool isDir = false;
> bool create = false;
> #ifdef WITH_SELINUX
> char *tcon = NULL;
> @@ -7879,6 +7880,7 @@ qemuDomainCreateDeviceRecursive(const char *device,
> isLink = S_ISLNK(sb.st_mode);
> isDev = S_ISCHR(sb.st_mode) || S_ISBLK(sb.st_mode);
> isReg = S_ISREG(sb.st_mode) || S_ISFIFO(sb.st_mode) ||
> S_ISSOCK(sb.st_mode);
> +isDir = S_ISDIR(sb.st_mode);
>
> /* Here, @device might be whatever path in the system. We
> * should create the path in the namespace iff it's "/dev"
> @@ -7996,6 +7998,10 @@ qemuDomainCreateDeviceRecursive(const char *device,
> goto cleanup;
> /* Just create the file here so that code below sets
> * proper owner and mode. Bind mount only after that. */
> +} else if (isDir) {
> +if (create &&
> +virFileMakePathWithMode(devicePath, sb.st_mode) < 0)
> +goto cleanup;
> } else {
> virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> _("unsupported device type %s 0%o"),
> @@ -8057,7 +8063,7 @@ qemuDomainCreateDeviceRecursive(const char *device,
> #endif
>
> /* Finish mount process started earlier. */
> -if (isReg &&
> +if ((isReg || isDir) &&
> virFileBindMountDevice(device, devicePath) < 0)
> goto cleanup;
>
> @@ -8686,6 +8692,7 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid
> ATTRIBUTE_UNUSED,
> bool isLink = S_ISLNK(data->sb.st_mode);
> bool isDev = S_ISCHR(data->sb.st_mode) || S_ISBLK(data->sb.st_mode);
> bool isReg = S_ISREG(data->sb.st_mode) || S_ISFIFO(data->sb.st_mode) ||
> S_ISSOCK(data->sb.st_mode);
> +bool isDir = S_ISDIR(data->sb.st_mode);
>
> qemuSecurityPostFork(data->driver->securityManager);
>
> @@ -8741,6 +8748,23 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid
> ATTRIBUTE_UNUSED,
> delDevice = true;
> /* Just create the file here so that code below sets
> * proper owner and mode. Move the mount only after that. */
> +} else if (isDir) {
> +/* We are not cleaning up disks on virDomainDetachDevice
> + * because disk might be still in use by different disk
> + * as its backing chain. This might however clash here.
> + * Therefore do the cleanup here. */
> +if (umount(data->file) < 0 &&
> +errno != ENOENT && errno != EINVAL) {
> +virReportSystemError(errno,
> + _("Unable to umount %s"),
> + data->file);
> +goto cleanup;
> +}
> +if (virFileMakePathWithMode(data->file, data->sb.st_mode) < 0)
> +goto cleanup;
> +delDevice = true;
> +/* Just create the folder here so that code below sets
> + * proper owner and mode. Move the mount only after that. */
Or, you can merge this with the previous else branch since it's
practically identical. Just like you're doing in the next hunk.
> } else {
> virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> _("unsupported device type %s 0%o"),
> @@ -8788,14 +8812,18 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid
> ATTRIBUTE_UNUSED,
> # endif
>
> /* Finish mount process started earlier. */
> -if (isReg &&
> +if ((isReg || isDir) &&
> virFileMoveMount(data->target, data->file) < 0)
> goto cleanup;
>
> ret = 0;
> cleanup:
> -if (ret < 0 && delDevice)
> -unlink(data->file);
> +if (ret < 0 && delDevice) {
> +if (isDir)
> +virFileDeleteTree(data->file);
> +else
> +unlink(data->file);
> +}
> # ifdef WITH_SELINUX
> freecon(data->tcon);
> # endif
ACK series.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v5 7/7] virsh: Implement managedsave-edit command
From: Kothapally Madhu Pavan
Add a simple virsh command handler which makes use of the new API.
Signed-off-by: Kothapally Madhu Pavan
---
tools/virsh-domain.c | 72
tools/virsh.pod | 21 +++
2 files changed, 93 insertions(+)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 14960c4a2..f235c66b0 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -4704,6 +4704,72 @@ cmdManagedSaveRemove(vshControl *ctl, const vshCmd *cmd)
return ret;
}
+/*
+ * "managedsave-edit" command
+ */
+static const vshCmdInfo info_managed_save_edit[] = {
+ {.name = "help",
+.data = N_("edit XML for a domain's managed save state file")
+ },
+ {.name = "desc",
+.data = N_("Edit the domain XML associated with the managed save state
file")
+ },
+ {.name = NULL}
+};
+
+static const vshCmdOptDef opts_managed_save_edit[] = {
+VIRSH_COMMON_OPT_DOMAIN_FULL,
+{.name = "running",
+ .type = VSH_OT_BOOL,
+ .help = N_("set domain to be running on start")
+},
+{.name = "paused",
+ .type = VSH_OT_BOOL,
+ .help = N_("set domain to be paused on start")
+},
+{.name = NULL}
+};
+
+static bool
+cmdManagedSaveEdit(vshControl *ctl, const vshCmd *cmd)
+{
+bool ret = false;
+virDomainPtr dom = NULL;
+unsigned int getxml_flags = VIR_DOMAIN_XML_SECURE;
+unsigned int define_flags = 0;
+
+if (vshCommandOptBool(cmd, "running"))
+define_flags |= VIR_DOMAIN_SAVE_RUNNING;
+if (vshCommandOptBool(cmd, "paused"))
+define_flags |= VIR_DOMAIN_SAVE_PAUSED;
+
+VSH_EXCLUSIVE_OPTIONS("running", "paused");
+
+dom = virshCommandOptDomain(ctl, cmd, NULL);
+if (dom == NULL)
+goto cleanup;
+
+#define EDIT_GET_XML virDomainManagedSaveGetXMLDesc(dom, getxml_flags)
+#define EDIT_NOT_CHANGED
\
+do {
\
+vshPrintExtra(ctl, _("Managed save image of domain %s XML
configuration " \
+ "not changed.\n"), virDomainGetName(dom));
\
+ret = true;
\
+goto edit_cleanup;
\
+} while (0)
+#define EDIT_DEFINE \
+(virDomainManagedSaveDefineXML(dom, doc_edited, define_flags) == 0)
+#include "virsh-edit.c"
+
+vshPrintExtra(ctl, _("Managed save image of Domain %s XML configuration
edited.\n"),
+ virDomainGetName(dom));
+ret = true;
+
+ cleanup:
+virshDomainFree(dom);
+return ret;
+}
+
/*
* "managedsave-dumpxml" command
*/
@@ -14006,6 +14072,12 @@ const vshCmdDef domManagementCmds[] = {
.info = info_managedsaveremove,
.flags = 0
},
+{.name = "managedsave-edit",
+ .handler = cmdManagedSaveEdit,
+ .opts = opts_managed_save_edit,
+ .info = info_managed_save_edit,
+ .flags = 0
+},
{.name = "managedsave-dumpxml",
.handler = cmdManagedSaveDumpxml,
.opts = opts_managed_save_dumpxml,
diff --git a/tools/virsh.pod b/tools/virsh.pod
index f7b05f803..c13f96f22 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -1648,6 +1648,27 @@ Extract the domain XML that was in effect at the time
the saved state
file I was created with the B command. Using
I<--security-info> will also include security sensitive information.
+=item B I [{I<--running> | I<--paused>}]
+
+Edit the XML configuration associated with a saved state file of a
+I was created by the B command.
+
+The managed save image records whether the domain should be started to a
+running or paused state. Normally, this command does not alter the
+recorded state; passing either the I<--running> or I<--paused> flag
+will allow overriding which state the B should use.
+
+This is equivalent to:
+
+ virsh managedsave-dumpxml domain-name > state-file.xml
+ vi state-file.xml (or make changes with your other text editor)
+ virsh managedsave-define domain-name state-file-xml
+
+except that it does some error checking.
+
+The editor used can be supplied by the C<$VISUAL> or C<$EDITOR> environment
+variables, and defaults to C.
+
=item B [I]
Provide the maximum number of virtual CPUs supported for a guest VM on
--
2.14.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v5 2/7] lib: Add API to edit domain's managed save state xml configuration
From: Kothapally Madhu Pavan
Similar to domainSaveImageDefineXML this commit adds domainManagedSaveDefineXML
API which allows to edit domain's managed save state xml configuration.
Signed-off-by: Kothapally Madhu Pavan
---
include/libvirt/libvirt-domain.h | 4 +++
src/driver-hypervisor.h | 6 +
src/libvirt-domain.c | 57
src/libvirt_public.syms | 1 +
src/remote/remote_driver.c | 1 +
src/remote/remote_protocol.x | 15 ++-
src/remote_protocol-structs | 6 +
7 files changed, 89 insertions(+), 1 deletion(-)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index 4c5a7f7b5..030a62c43 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -1215,6 +1215,10 @@ int
virDomainManagedSaveRemove(virDomainPtr dom,
unsigned int flags);
char * virDomainManagedSaveGetXMLDesc(virDomainPtr domain,
unsigned int flags);
+intvirDomainManagedSaveDefineXML(virDomainPtr domain,
+ const char *dxml,
+ unsigned int flags);
+
/*
* Domain core dump
diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h
index a3f9cbc31..6c3f7d795 100644
--- a/src/driver-hypervisor.h
+++ b/src/driver-hypervisor.h
@@ -754,6 +754,11 @@ typedef char *
(*virDrvDomainManagedSaveGetXMLDesc)(virDomainPtr domain,
unsigned int flags);
+typedef int
+(*virDrvDomainManagedSaveDefineXML)(virDomainPtr domain,
+const char *dxml,
+unsigned int flags);
+
typedef virDomainSnapshotPtr
(*virDrvDomainSnapshotCreateXML)(virDomainPtr domain,
const char *xmlDesc,
@@ -1433,6 +1438,7 @@ struct _virHypervisorDriver {
virDrvDomainHasManagedSaveImage domainHasManagedSaveImage;
virDrvDomainManagedSaveRemove domainManagedSaveRemove;
virDrvDomainManagedSaveGetXMLDesc domainManagedSaveGetXMLDesc;
+virDrvDomainManagedSaveDefineXML domainManagedSaveDefineXML;
virDrvDomainSnapshotCreateXML domainSnapshotCreateXML;
virDrvDomainSnapshotGetXMLDesc domainSnapshotGetXMLDesc;
virDrvDomainSnapshotNum domainSnapshotNum;
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index f43ab2478..ca5a1536a 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -9389,6 +9389,63 @@ virDomainManagedSaveGetXMLDesc(virDomainPtr domain,
unsigned int flags)
}
+/**
+ * virDomainManagedSaveDefineXML:
+ * @domain: a domain object
+ * @dxml: XML config for adjusting guest xml used on restore
+ * @flags: bitwise-OR of virDomainSaveRestoreFlags
+ *
+ * This updates the definition of a domain stored in a saved state
+ * file.
+ *
+ * @dxml can be used to alter host-specific portions of the domain XML
+ * that will be used on the next start of the domain. For example, it is
+ * possible to alter the backing filename that is associated with a
+ * disk device.
+ *
+ * Normally, the saved state file will remember whether the domain was
+ * running or paused, and restore defaults to the same state.
+ * Specifying VIR_DOMAIN_SAVE_RUNNING or VIR_DOMAIN_SAVE_PAUSED in
+ * @flags will override the default saved into the file; omitting both
+ * leaves the file's default unchanged. These two flags are mutually
+ * exclusive.
+ *
+ * Returns 0 in case of success and -1 in case of failure.
+ */
+int
+virDomainManagedSaveDefineXML(virDomainPtr domain, const char *dxml,
+ unsigned int flags)
+{
+virConnectPtr conn;
+
+VIR_DOMAIN_DEBUG(domain, "flags=%x", flags);
+
+virResetLastError();
+
+VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_SAVE_RUNNING,
+ VIR_DOMAIN_SAVE_PAUSED,
+ error);
+
+virCheckDomainReturn(domain, -1);
+conn = domain->conn;
+
+if (conn->driver->domainManagedSaveDefineXML) {
+int ret;
+ret = conn->driver->domainManagedSaveDefineXML(domain, dxml, flags);
+
+if (ret < 0)
+goto error;
+return ret;
+}
+
+virReportUnsupportedError();
+
+ error:
+virDispatchError(domain->conn);
+return -1;
+}
+
+
/**
* virDomainOpenConsole:
* @dom: a domain object
diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
index 58b9b4910..498601943 100644
--- a/src/libvirt_public.syms
+++ b/src/libvirt_public.syms
@@ -772,5 +772,6 @@ LIBVIRT_3.7.0 {
global:
virDomainMigrateGetMaxDowntime;
virDomainManagedSaveGetXMLDesc;
+virDomainManagedSaveDefineXML;
} LIBVIRT_3.4.0;
# define new API here using predicted next version number
diff --git a/src/remote/remote_driver.c b/src/remote/remote_
[libvirt] [PATCH v5 5/7] virsh: Implement managedsave-define command
From: Kothapally Madhu Pavan
Add a simple virsh command handler which makes use of the new API.
Signed-off-by: Kothapally Madhu Pavan
---
tools/virsh-domain.c | 78
tools/virsh.pod | 12
2 files changed, 90 insertions(+)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index a3410e791..8f048f678 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -4704,6 +4704,78 @@ cmdManagedSaveRemove(vshControl *ctl, const vshCmd *cmd)
return ret;
}
+/*
+ * "managedsave-define" command
+ */
+static const vshCmdInfo info_managed_save_define[] = {
+{.name = "help",
+ .data = N_("redefine the XML for a domain's managed save state file")
+},
+{.name = "desc",
+ .data = N_("Replace the domain XML associated with a managed save state
file")
+},
+{.name = NULL}
+};
+
+static const vshCmdOptDef opts_managed_save_define[] = {
+VIRSH_COMMON_OPT_DOMAIN_FULL,
+{.name = "xml",
+ .type = VSH_OT_DATA,
+ .flags = VSH_OFLAG_REQ,
+ .help = N_("filename containing updated XML for the target")
+},
+{.name = "running",
+ .type = VSH_OT_BOOL,
+ .help = N_("set domain to be running on start")
+},
+{.name = "paused",
+ .type = VSH_OT_BOOL,
+ .help = N_("set domain to be paused on start")
+},
+{.name = NULL}
+};
+
+static bool
+cmdManagedSaveDefine(vshControl *ctl, const vshCmd *cmd)
+{
+bool ret = false;
+virDomainPtr dom = NULL;
+const char *xmlfile = NULL;
+char *xml = NULL;
+unsigned int flags = 0;
+
+if (vshCommandOptBool(cmd, "running"))
+flags |= VIR_DOMAIN_SAVE_RUNNING;
+if (vshCommandOptBool(cmd, "paused"))
+flags |= VIR_DOMAIN_SAVE_PAUSED;
+
+VSH_EXCLUSIVE_OPTIONS("running", "paused");
+
+if (vshCommandOptStringReq(ctl, cmd, "xml", &xmlfile) < 0)
+return false;
+
+if (virFileReadAll(xmlfile, VSH_MAX_XML_FILE, &xml) < 0)
+return false;
+
+if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
+goto cleanup;
+
+if (virDomainManagedSaveDefineXML(dom, xml, flags) < 0) {
+vshError(ctl, _("Failed to update %s XML configuration"),
+virDomainGetName(dom));
+goto cleanup;
+}
+
+vshPrintExtra(ctl, _("Managed save state file of domain %s updated.\n"),
+ virDomainGetName(dom));
+ret = true;
+
+ cleanup:
+virshDomainFree(dom);
+VIR_FREE(xml);
+return ret;
+}
+
/*
* "schedinfo" command
*/
@@ -13886,6 +13958,12 @@ const vshCmdDef domManagementCmds[] = {
.info = info_managedsaveremove,
.flags = 0
},
+{.name = "managedsave-define",
+ .handler = cmdManagedSaveDefine,
+ .opts = opts_managed_save_define,
+ .info = info_managed_save_define,
+ .flags = 0
+},
{.name = "memtune",
.handler = cmdMemtune,
.opts = opts_memtune,
diff --git a/tools/virsh.pod b/tools/virsh.pod
index 15a414399..5c87af302 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -1630,6 +1630,18 @@ has any managed save image.
Remove the B state file for a domain, if it exists. This
ensures the domain will do a full boot the next time it is started.
+=item B I I [{I<--running> | I<--paused>}]
+
+Update the domain XML that will be used when I is later
+started. The I argument must be a file name containing
+the alternative XML, with changes only in the host-specific portions of
+the domain XML. For example, it can be used to change disk file paths.
+
+The managed save image records whether the domain should be started to a
+running or paused state. Normally, this command does not alter the
+recorded state; passing either the I<--running> or I<--paused> flag
+will allow overriding which state the B should use.
+
=item B [I]
Provide the maximum number of virtual CPUs supported for a guest VM on
--
2.14.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v5 6/7] virsh: Implement managedsave-dumpxml command
From: Kothapally Madhu Pavan
Add a simple virsh command handler which makes use of the new API.
Signed-off-by: Kothapally Madhu Pavan
---
tools/virsh-domain.c | 54
tools/virsh.pod | 6 ++
2 files changed, 60 insertions(+)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 8f048f678..14960c4a2 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -4704,6 +4704,54 @@ cmdManagedSaveRemove(vshControl *ctl, const vshCmd *cmd)
return ret;
}
+/*
+ * "managedsave-dumpxml" command
+ */
+static const vshCmdInfo info_managed_save_dumpxml[] = {
+ {.name = "help",
+.data = N_("Domain information of managed save state file in XML")
+ },
+ {.name = "desc",
+.data = N_("Dump XML of domain information for a managed save state file
to stdout.")
+ },
+ {.name = NULL}
+};
+
+static const vshCmdOptDef opts_managed_save_dumpxml[] = {
+VIRSH_COMMON_OPT_DOMAIN_FULL,
+{.name = "security-info",
+ .type = VSH_OT_BOOL,
+ .help = N_("include security sensitive information in XML dump")
+},
+{.name = NULL}
+};
+
+static bool
+cmdManagedSaveDumpxml(vshControl *ctl, const vshCmd *cmd)
+{
+bool ret = false;
+virDomainPtr dom = NULL;
+unsigned int flags = 0;
+char *xml = NULL;
+
+if (vshCommandOptBool(cmd, "security-info"))
+flags |= VIR_DOMAIN_XML_SECURE;
+
+if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
+goto cleanup;
+
+if (!(xml = virDomainManagedSaveGetXMLDesc(dom, flags)))
+goto cleanup;
+
+vshPrint(ctl, "%s", xml);
+ret = true;
+
+ cleanup:
+virshDomainFree(dom);
+VIR_FREE(xml);
+return ret;
+}
+
/*
* "managedsave-define" command
*/
@@ -13958,6 +14006,12 @@ const vshCmdDef domManagementCmds[] = {
.info = info_managedsaveremove,
.flags = 0
},
+{.name = "managedsave-dumpxml",
+ .handler = cmdManagedSaveDumpxml,
+ .opts = opts_managed_save_dumpxml,
+ .info = info_managed_save_dumpxml,
+ .flags = 0
+},
{.name = "managedsave-define",
.handler = cmdManagedSaveDefine,
.opts = opts_managed_save_define,
diff --git a/tools/virsh.pod b/tools/virsh.pod
index 5c87af302..f7b05f803 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -1642,6 +1642,12 @@ running or paused state. Normally, this command does
not alter the
recorded state; passing either the I<--running> or I<--paused> flag
will allow overriding which state the B should use.
+=item B I [I<--security-info>]
+
+Extract the domain XML that was in effect at the time the saved state
+file I was created with the B command. Using
+I<--security-info> will also include security sensitive information.
+
=item B [I]
Provide the maximum number of virtual CPUs supported for a guest VM on
--
2.14.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v5 4/7] qemu: Implement qemuDomainManagedSaveDefineXML
From: Kothapally Madhu Pavan
This commit adds qemu driver implementation to edit xml
configuration of managed save state file of a domain.
Signed-off-by: Kothapally Madhu Pavan
---
src/qemu/qemu_driver.c | 34 ++
1 file changed, 34 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index b9e58e562..b7824512c 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6841,6 +6841,39 @@ qemuDomainManagedSaveGetXMLDesc(virDomainPtr dom,
unsigned int flags)
return ret;
}
+static int
+qemuDomainManagedSaveDefineXML(virDomainPtr dom, const char *dxml,
+ unsigned int flags)
+{
+virQEMUDriverPtr driver = dom->conn->privateData;
+virConnectPtr conn = dom->conn;
+virDomainObjPtr vm;
+char *path = NULL;
+int ret = -1;
+
+if (!(vm = qemuDomObjFromDomain(dom)))
+return -1;
+
+if (virDomainManagedSaveDefineXMLEnsureACL(conn, vm->def) < 0)
+goto cleanup;
+
+if (!(path = qemuDomainManagedSavePath(driver, vm)))
+goto cleanup;
+
+if (!virFileExists(path)) {
+virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("domain does not have managed save image"));
+goto cleanup;
+}
+
+ret = qemuDomainSaveImageDefineXML(conn, path, dxml, flags);
+
+ cleanup:
+virDomainObjEndAPI(&vm);
+VIR_FREE(path);
+return ret;
+}
+
/* Return 0 on success, 1 if incomplete saved image was silently unlinked,
* and -1 on failure with error raised. */
static int
@@ -20945,6 +20978,7 @@ static virHypervisorDriver qemuHypervisorDriver = {
.domainHasManagedSaveImage = qemuDomainHasManagedSaveImage, /* 0.8.0 */
.domainManagedSaveRemove = qemuDomainManagedSaveRemove, /* 0.8.0 */
.domainManagedSaveGetXMLDesc = qemuDomainManagedSaveGetXMLDesc, /* 3.7.0 */
+.domainManagedSaveDefineXML = qemuDomainManagedSaveDefineXML, /* 3.7.0 */
.domainSnapshotCreateXML = qemuDomainSnapshotCreateXML, /* 0.8.0 */
.domainSnapshotGetXMLDesc = qemuDomainSnapshotGetXMLDesc, /* 0.8.0 */
.domainSnapshotNum = qemuDomainSnapshotNum, /* 0.8.0 */
--
2.14.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v5 3/7] qemu: Implement qemuDomainManagedSaveGetXMLDesc
From: Kothapally Madhu Pavan
This commit adds qemu driver implementation to get xml description
for managed save state domain.
Signed-off-by: Kothapally Madhu Pavan
---
src/qemu/qemu_driver.c | 45 +
1 file changed, 45 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 672d3f8b3..b9e58e562 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6797,6 +6797,50 @@ qemuDomainSaveImageDefineXML(virConnectPtr conn, const
char *path,
return ret;
}
+static char *
+qemuDomainManagedSaveGetXMLDesc(virDomainPtr dom, unsigned int flags)
+{
+virQEMUDriverPtr driver = dom->conn->privateData;
+virDomainObjPtr vm;
+char *path = NULL;
+char *ret = NULL;
+virDomainDefPtr def = NULL;
+int fd = -1;
+virQEMUSaveDataPtr data = NULL;
+
+/* We only take subset of virDomainDefFormat flags. */
+virCheckFlags(VIR_DOMAIN_XML_SECURE, NULL);
+
+if (!(vm = qemuDomObjFromDomain(dom)))
+return ret;
+
+if (virDomainManagedSaveGetXMLDescEnsureACL(dom->conn, vm->def, flags) < 0)
+goto cleanup;
+
+if (!(path = qemuDomainManagedSavePath(driver, vm)))
+goto cleanup;
+
+if (!virFileExists(path)) {
+virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("domain does not have managed save image"));
+goto cleanup;
+}
+
+if ((fd = qemuDomainSaveImageOpen(driver, path, &def, &data,
+ false, NULL, false, false)) < 0)
+goto cleanup;
+
+ret = qemuDomainDefFormatXML(driver, def, flags);
+
+ cleanup:
+virQEMUSaveDataFree(data);
+virDomainDefFree(def);
+VIR_FORCE_CLOSE(fd);
+virDomainObjEndAPI(&vm);
+VIR_FREE(path);
+return ret;
+}
+
/* Return 0 on success, 1 if incomplete saved image was silently unlinked,
* and -1 on failure with error raised. */
static int
@@ -20900,6 +20944,7 @@ static virHypervisorDriver qemuHypervisorDriver = {
.domainManagedSave = qemuDomainManagedSave, /* 0.8.0 */
.domainHasManagedSaveImage = qemuDomainHasManagedSaveImage, /* 0.8.0 */
.domainManagedSaveRemove = qemuDomainManagedSaveRemove, /* 0.8.0 */
+.domainManagedSaveGetXMLDesc = qemuDomainManagedSaveGetXMLDesc, /* 3.7.0 */
.domainSnapshotCreateXML = qemuDomainSnapshotCreateXML, /* 0.8.0 */
.domainSnapshotGetXMLDesc = qemuDomainSnapshotGetXMLDesc, /* 0.8.0 */
.domainSnapshotNum = qemuDomainSnapshotNum, /* 0.8.0 */
--
2.14.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v5 1/7] lib: Add API to dump xml configuration of managed save state domain
From: Kothapally Madhu Pavan
Similar to domainSaveImageGetXMLDesc this commit adds
domainManagedSaveGetXMLDesc
API which allows to get the xml of managed save state domain.
Signed-off-by: Kothapally Madhu Pavan
---
include/libvirt/libvirt-domain.h | 2 ++
src/driver-hypervisor.h | 5
src/libvirt-domain.c | 49
src/libvirt_public.syms | 1 +
src/remote/remote_driver.c | 1 +
src/remote/remote_protocol.x | 17 +-
src/remote_protocol-structs | 8 +++
7 files changed, 82 insertions(+), 1 deletion(-)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index fae24ac93..4c5a7f7b5 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -1213,6 +1213,8 @@ int
virDomainHasManagedSaveImage(virDomainPtr dom,
unsigned int flags);
intvirDomainManagedSaveRemove(virDomainPtr dom,
unsigned int flags);
+char * virDomainManagedSaveGetXMLDesc(virDomainPtr domain,
+ unsigned int flags);
/*
* Domain core dump
diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h
index 7b35e9e68..a3f9cbc31 100644
--- a/src/driver-hypervisor.h
+++ b/src/driver-hypervisor.h
@@ -750,6 +750,10 @@ typedef int
(*virDrvDomainManagedSaveRemove)(virDomainPtr domain,
unsigned int flags);
+typedef char *
+(*virDrvDomainManagedSaveGetXMLDesc)(virDomainPtr domain,
+ unsigned int flags);
+
typedef virDomainSnapshotPtr
(*virDrvDomainSnapshotCreateXML)(virDomainPtr domain,
const char *xmlDesc,
@@ -1428,6 +1432,7 @@ struct _virHypervisorDriver {
virDrvDomainManagedSave domainManagedSave;
virDrvDomainHasManagedSaveImage domainHasManagedSaveImage;
virDrvDomainManagedSaveRemove domainManagedSaveRemove;
+virDrvDomainManagedSaveGetXMLDesc domainManagedSaveGetXMLDesc;
virDrvDomainSnapshotCreateXML domainSnapshotCreateXML;
virDrvDomainSnapshotGetXMLDesc domainSnapshotGetXMLDesc;
virDrvDomainSnapshotNum domainSnapshotNum;
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 4d0ac30d5..f43ab2478 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -9339,6 +9339,55 @@ virDomainManagedSaveRemove(virDomainPtr dom, unsigned
int flags)
}
+/**
+ * virDomainManagedSaveGetXMLDesc:
+ * @domain: a domain object
+ * @flags: bitwise-OR of subset of virDomainXMLFlags
+ *
+ * This method will extract the XML description of the managed save
+ * state file of a domain.
+ *
+ * No security-sensitive data will be included unless @flags contains
+ * VIR_DOMAIN_XML_SECURE; this flag is rejected on read-only
+ * connections. For this API, @flags should not contain either
+ * VIR_DOMAIN_XML_INACTIVE or VIR_DOMAIN_XML_UPDATE_CPU.
+ *
+ * Returns a 0 terminated UTF-8 encoded XML instance, or NULL in case of
+ * error. The caller must free() the returned value.
+ */
+char *
+virDomainManagedSaveGetXMLDesc(virDomainPtr domain, unsigned int flags)
+{
+virConnectPtr conn;
+
+VIR_DOMAIN_DEBUG(domain, "flags=%x", flags);
+
+virResetLastError();
+
+virCheckDomainReturn(domain, NULL);
+conn = domain->conn;
+
+if ((conn->flags & VIR_CONNECT_RO) && (flags & VIR_DOMAIN_XML_SECURE)) {
+virReportError(VIR_ERR_OPERATION_DENIED, "%s",
+ _("virDomainManagedSaveGetXMLDesc with secure flag"));
+goto error;
+}
+
+if (conn->driver->domainManagedSaveGetXMLDesc) {
+char *ret;
+ret = conn->driver->domainManagedSaveGetXMLDesc(domain, flags);
+if (!ret)
+goto error;
+return ret;
+}
+
+virReportUnsupportedError();
+
+ error:
+virDispatchError(domain->conn);
+return NULL;
+}
+
/**
* virDomainOpenConsole:
diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
index b55ca4b55..58b9b4910 100644
--- a/src/libvirt_public.syms
+++ b/src/libvirt_public.syms
@@ -771,5 +771,6 @@ LIBVIRT_3.4.0 {
LIBVIRT_3.7.0 {
global:
virDomainMigrateGetMaxDowntime;
+virDomainManagedSaveGetXMLDesc;
} LIBVIRT_3.4.0;
# define new API here using predicted next version number
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index 027b073ec..c64f5b337 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -8411,6 +8411,7 @@ static virHypervisorDriver hypervisor_driver = {
.domainManagedSave = remoteDomainManagedSave, /* 0.8.0 */
.domainHasManagedSaveImage = remoteDomainHasManagedSaveImage, /* 0.8.0 */
.domainManagedSaveRemove = remoteDomainManagedSaveRemove, /* 0.8.0 */
+.domainManagedSaveGetXMLDesc = remoteDomainManagedSaveGetXMLDesc, /* 3.7.0
*/
[libvirt] [PATCH v5 0/7] Add managed save XML modification APIs (pushed)
This is the version I've pushed. I'm sending it in for a reference of the modifications I've made as part of the review. Kothapally Madhu Pavan (7): lib: Add API to dump xml configuration of managed save state domain lib: Add API to edit domain's managed save state xml configuration qemu: Implement qemuDomainManagedSaveGetXMLDesc qemu: Implement qemuDomainManagedSaveDefineXML virsh: Implement managedsave-define command virsh: Implement managedsave-dumpxml command virsh: Implement managedsave-edit command include/libvirt/libvirt-domain.h | 6 ++ src/driver-hypervisor.h | 11 +++ src/libvirt-domain.c | 106 src/libvirt_public.syms | 2 + src/qemu/qemu_driver.c | 79 +++ src/remote/remote_driver.c | 2 + src/remote/remote_protocol.x | 30 +- src/remote_protocol-structs | 14 +++ tools/virsh-domain.c | 204 +++ tools/virsh.pod | 39 10 files changed, 492 insertions(+), 1 deletion(-) -- 2.14.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 2/6] udev: Split udevEventHandleCallback in two functions
On Mon, Aug 28, 2017 at 11:04:35AM -0400, John Ferlan wrote:
>
>
> On 08/24/2017 07:23 AM, Erik Skultety wrote:
> > This patch splits udevEventHandleCallback in two (introduces
> > udevEventHandleThread) in order to be later able to refactor the latter
> > to actually become a detached thread which will wait some time for the
> > kernel to create the whole sysfs tree for a device as we cannot do that
> > in the event loop directly.
> >
> > Signed-off-by: Erik Skultety
> > ---
> > src/node_device/node_device_udev.c | 54
> > ++
> > 1 file changed, 37 insertions(+), 17 deletions(-)
> >
> > diff --git a/src/node_device/node_device_udev.c
> > b/src/node_device/node_device_udev.c
> > index 465d272ba..fe943c78b 100644
> > --- a/src/node_device/node_device_udev.c
> > +++ b/src/node_device/node_device_udev.c
> > @@ -1647,36 +1647,56 @@ udevCheckMonitorFD(struct udev_monitor
> > *udev_monitor, int fd)
> >
> >
> > static void
> > +udevEventHandleThread(void *opaque ATTRIBUTE_UNUSED)
>
> It's not ATTRIBUTE_UNUSED
>
> > +{
> > +struct udev_device *device = NULL;
> > +struct udev_monitor *udev_monitor = NULL;
> > +int fd = (intptr_t) opaque;
>
> ^^^ opaque!
Right, good catch, thanks.
>
> > +
> > +nodeDeviceLock();
> > +udev_monitor = DRV_STATE_UDEV_MONITOR(driver);
> > +
> > +if (!udevCheckMonitorFD(udev_monitor, fd))
> > +goto unlock;
> > +
> > +device = udev_monitor_receive_device(udev_monitor);
> > +if (device == NULL) {
> > +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > + _("udev_monitor_receive_device returned NULL"));
> > +goto unlock;
> > +}
> > +
> > +nodeDeviceUnlock();
> > +udevHandleOneDevice(device);
> > +
> > + cleanup:
> > +udev_device_unref(device);
> > +return;
> > +
> > + unlock:
> > +nodeDeviceUnlock();
> > +goto cleanup;
> > +}
> > +
> > +
> > +static void
> > udevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
> > int fd,
> > int events ATTRIBUTE_UNUSED,
> > void *data ATTRIBUTE_UNUSED)
> > {
> > -struct udev_device *device = NULL;
> > struct udev_monitor *udev_monitor = NULL;
> >
> > nodeDeviceLock();
> > udev_monitor = DRV_STATE_UDEV_MONITOR(driver);
> >
> > -if (!udevCheckMonitorFD(udev_monitor, fd))
> > -goto unlock;
> > -
> > -if (!(device = udev_monitor_receive_device(udev_monitor))) {
> > -virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > - _("udev_monitor_receive_device returned NULL"));
> > -goto unlock;
> > +if (!udevCheckMonitorFD(udev_monitor, fd)) {
> > +nodeDeviceUnlock();
> > +return;
> > }
>
> The above sequence will be done twice - once here and repeated again in
> HandleThread. I understand why, but as I get to patch 3 - I'm not sure
> things are going to work as expected.
Okay, let's continue with the discussion in patch 3 comments :).
Erik
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 1/6] nodedev: Introduce udevCheckMonitorFD helper function
[...]
> > +
> > +if (!udevCheckMonitorFD(udev_monitor, fd))
> > +goto unlock;> +
> > +if (!(device = udev_monitor_receive_device(udev_monitor))) {
> > virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > _("udev_monitor_receive_device returned NULL"));
>
> I almost wonder if it would be better to have this be a
> ReportSystemError w/ errno... Not that udev docs give any guidance
> there, but maybe it'd help.
Not really, that would imply that we can rely on libudev setting the errno, but
the call can fail in multiple ways the most of which are unrelated to any
system call and given the poor interface libudev has :/ we don't have much of a
choice (but yeah, the error is rather uninformative, but since you have no
control over it, it's the best we have IMO).
>
> > -goto cleanup;
> > +goto unlock;
>
> I know this is "existing"; however, if !device, then what's the purpose
> of calling udev_device_unref(NULL)? In fact there's one path in
> udevGetDMIData which actually checks != NULL before calling - although
> it has no reason to since I see no way for it to be NULL at cleanup
> (again a different issue).
>
> Also, wouldn't nodeDeviceLock/Unlock wrapping only be necessary within
> udevCheckMonitorFD? Why would the udev call need to be wrapped as well?
So, IMHO, again, I'm still not convinced about the whole file descriptor
changing under our hands idea (unrelated - was it actually the correct
wording?). But since the general consensus was to keep the sanity checks, I
moved the @fd extraction within the locks. Now, if we presume such thing can
happen, you don't want anyone touching the driver structure in the meantime,
otherwise you're left with a dangling pointer @udev_monitor and bad things
would happen. This way, you don't have to worry about the driver's structure
getting changed, possibly invalidating the file descriptor (not that we were,
but if we really would/should...).
Erik
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] qemu: Honour
On 08/29/2017 12:57 PM, Martin Kletzander wrote:
> On Tue, Aug 29, 2017 at 12:46:43PM +0200, Michal Privoznik wrote:
>> On 08/29/2017 11:08 AM, Martin Kletzander wrote:
>>> On Wed, Aug 16, 2017 at 04:38:09PM +0200, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1476866
For some reason, we completely ignore setting for
domains. The implementation is simply not there. It never was.
Signed-off-by: Michal Privoznik
---
diff to v1:
- dropped the spoofed logic
- Switch from qemuProcessShutdownOrReboot() to qemuPrcoessStop()
because that's
what impl does too.
src/qemu/qemu_process.c | 27 ---
1 file changed, 24 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index fed2bc588..3df6c320e 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -484,6 +484,7 @@ qemuProcessHandleReset(qemuMonitorPtr mon
ATTRIBUTE_UNUSED,
virObjectEventPtr event;
qemuDomainObjPrivatePtr priv;
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+int ret = -1;
virObjectLock(vm);
@@ -495,12 +496,32 @@ qemuProcessHandleReset(qemuMonitorPtr mon
ATTRIBUTE_UNUSED,
if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm,
driver->caps) < 0)
VIR_WARN("Failed to save status on vm %s", vm->def->name);
+if (vm->def->onReboot == VIR_DOMAIN_LIFECYCLE_DESTROY ||
+vm->def->onReboot == VIR_DOMAIN_LIFECYCLE_PRESERVE) {
+
+if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
+goto cleanup;
+
+if (!virDomainObjIsActive(vm)) {
+VIR_DEBUG("Ignoring RESET event from inactive domain %s",
+ vm->def->name);
+goto endjob;
+}
+
+qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_DESTROYED,
+QEMU_ASYNC_JOB_NONE, 0);
+virDomainAuditStop(vm, "destroyed");
>>>
>>> Queuing another event here that the domain is being destroyed seems both
>>> appropriate and weird to me. So I'll leave it up to you. It's not like
>>> anyone ever used this functionality... ever. ACK either way.
>>
>> I think we want to queue the event here. Imagine that there's a mgmt app
>> that acts like HA daemon. Whenever a domain is destroyed it has to start
>> it up again. Well, with the current code it has to listen to RESET event
>> and depending on libvirt version it has to either ignore the event or
>> start it up again. However, if we queue the event all that the app has
>> to do is to listen to DESTROY event. Therefore I'm inclined to leave it
>> here.
>>
>
> Leave? I was talking about adding it. I don't see it here.
Oh, I though that qemuProcessStop does queue an event. But it doesn't. I
shouldn't reply to e-mails right after having lunch. I'm going to post a
patch that does enqueue the event. Stay tuned.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] qemu: Honour
On Tue, Aug 29, 2017 at 12:46:43PM +0200, Michal Privoznik wrote:
On 08/29/2017 11:08 AM, Martin Kletzander wrote:
On Wed, Aug 16, 2017 at 04:38:09PM +0200, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1476866
For some reason, we completely ignore setting for
domains. The implementation is simply not there. It never was.
Signed-off-by: Michal Privoznik
---
diff to v1:
- dropped the spoofed logic
- Switch from qemuProcessShutdownOrReboot() to qemuPrcoessStop()
because that's
what impl does too.
src/qemu/qemu_process.c | 27 ---
1 file changed, 24 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index fed2bc588..3df6c320e 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -484,6 +484,7 @@ qemuProcessHandleReset(qemuMonitorPtr mon
ATTRIBUTE_UNUSED,
virObjectEventPtr event;
qemuDomainObjPrivatePtr priv;
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+int ret = -1;
virObjectLock(vm);
@@ -495,12 +496,32 @@ qemuProcessHandleReset(qemuMonitorPtr mon
ATTRIBUTE_UNUSED,
if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm,
driver->caps) < 0)
VIR_WARN("Failed to save status on vm %s", vm->def->name);
+if (vm->def->onReboot == VIR_DOMAIN_LIFECYCLE_DESTROY ||
+vm->def->onReboot == VIR_DOMAIN_LIFECYCLE_PRESERVE) {
+
+if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
+goto cleanup;
+
+if (!virDomainObjIsActive(vm)) {
+VIR_DEBUG("Ignoring RESET event from inactive domain %s",
+ vm->def->name);
+goto endjob;
+}
+
+qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_DESTROYED,
+QEMU_ASYNC_JOB_NONE, 0);
+virDomainAuditStop(vm, "destroyed");
Queuing another event here that the domain is being destroyed seems both
appropriate and weird to me. So I'll leave it up to you. It's not like
anyone ever used this functionality... ever. ACK either way.
I think we want to queue the event here. Imagine that there's a mgmt app
that acts like HA daemon. Whenever a domain is destroyed it has to start
it up again. Well, with the current code it has to listen to RESET event
and depending on libvirt version it has to either ignore the event or
start it up again. However, if we queue the event all that the app has
to do is to listen to DESTROY event. Therefore I'm inclined to leave it
here.
Leave? I was talking about adding it. I don't see it here.
Pushed, thanks.
Michal
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/5] tests: add qemu chardev srouce reconnect tests
On Tue, Aug 29, 2017 at 11:17:13AM +0200, Michal Privoznik wrote: > On 08/28/2017 02:56 PM, Pavel Hrdina wrote: > > Signed-off-by: Pavel Hrdina > > --- > > .../qemuxml2argv-channel-reconnect.args| 31 +++ > > ...uxml2argv-chardev-reconnect-invalid-timeout.xml | 23 +++ > > .../qemuxml2argv-chardev-reconnect.args| 40 +++ > > .../qemuxml2argv-chardev-reconnect.xml | 46 > > ++ > > tests/qemuxml2argvtest.c | 11 ++ > > 5 files changed, 151 insertions(+) > > create mode 100644 > > tests/qemuxml2argvdata/qemuxml2argv-channel-reconnect.args > > create mode 100644 > > tests/qemuxml2argvdata/qemuxml2argv-chardev-reconnect-invalid-timeout.xml > > create mode 100644 > > tests/qemuxml2argvdata/qemuxml2argv-chardev-reconnect.args > > create mode 100644 > > tests/qemuxml2argvdata/qemuxml2argv-chardev-reconnect.xml > > > > diff --git a/tests/qemuxml2argvdata/qemuxml2argv-channel-reconnect.args > > b/tests/qemuxml2argvdata/qemuxml2argv-channel-reconnect.args > > new file mode 100644 > > index 00..43a5d5bb3e > > --- /dev/null > > +++ b/tests/qemuxml2argvdata/qemuxml2argv-channel-reconnect.args > > @@ -0,0 +1,31 @@ > > +LC_ALL=C \ > > +PATH=/bin \ > > +HOME=/home/test \ > > +USER=test \ > > +LOGNAME=test \ > > +QEMU_AUDIO_DRV=none \ > > +/usr/bin/qemu-system-i686 \ > > +-name QEMUGuest1 \ > > +-S \ > > +-M pc \ > > +-m 214 \ > > +-smp 1,sockets=1,cores=1,threads=1 \ > > +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ > > +-nographic \ > > +-nodefconfig \ > > +-nodefaults \ > > +-chardev > > socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ > > +server,nowait \ > > +-mon chardev=charmonitor,id=monitor,mode=readline \ > > +-no-acpi \ > > +-boot c \ > > +-device virtio-serial-pci,id=virtio-serial1,bus=pci.0,addr=0xa \ > > +-device virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x3 \ > > +-usb \ > > +-chardev socket,id=charchannel0,host=localhost,port=1234,reconnect=10 \ > > +-device virtserialport,bus=virtio-serial0.0,nr=1,chardev=charchannel0,\ > > +id=channel0,name=asdf \ > > +-chardev > > socket,id=charchannel1,path=/tmp/channel/domain--1-QEMUGuest1/fdsa,\ > > +server,nowait,reconnect=10 \ > > This doesn't look right. How come a server can have reconnect at the > same time? So there are two issues with our code: 1. We successfully parse this configuration: ... ... While formatting the XML we ignore the source element if there is no path and the code starting a domain generates a new path and set's the mode to 'bind'. With the reconnect patches if we parse the reconnect it's unconditionally put to the command line if it was parsed even though the mode was changed. 2. This test uses a path that matches a pattern of automatically generated paths by libvirt: ... ... and our code removes that path in order to get it automatically generated following new rules and also changes the mode to "bind" and that leads to the same issue as for the first example. I'll send a patches to fix it. Pavel signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] qemu: Honour
On 08/29/2017 11:08 AM, Martin Kletzander wrote:
> On Wed, Aug 16, 2017 at 04:38:09PM +0200, Michal Privoznik wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1476866
>>
>> For some reason, we completely ignore setting for
>> domains. The implementation is simply not there. It never was.
>>
>> Signed-off-by: Michal Privoznik
>> ---
>>
>> diff to v1:
>> - dropped the spoofed logic
>> - Switch from qemuProcessShutdownOrReboot() to qemuPrcoessStop()
>> because that's
>> what impl does too.
>>
>> src/qemu/qemu_process.c | 27 ---
>> 1 file changed, 24 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index fed2bc588..3df6c320e 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -484,6 +484,7 @@ qemuProcessHandleReset(qemuMonitorPtr mon
>> ATTRIBUTE_UNUSED,
>> virObjectEventPtr event;
>> qemuDomainObjPrivatePtr priv;
>> virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>> +int ret = -1;
>>
>> virObjectLock(vm);
>>
>> @@ -495,12 +496,32 @@ qemuProcessHandleReset(qemuMonitorPtr mon
>> ATTRIBUTE_UNUSED,
>> if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm,
>> driver->caps) < 0)
>> VIR_WARN("Failed to save status on vm %s", vm->def->name);
>>
>> +if (vm->def->onReboot == VIR_DOMAIN_LIFECYCLE_DESTROY ||
>> +vm->def->onReboot == VIR_DOMAIN_LIFECYCLE_PRESERVE) {
>> +
>> +if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
>> +goto cleanup;
>> +
>> +if (!virDomainObjIsActive(vm)) {
>> +VIR_DEBUG("Ignoring RESET event from inactive domain %s",
>> + vm->def->name);
>> +goto endjob;
>> +}
>> +
>> +qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_DESTROYED,
>> +QEMU_ASYNC_JOB_NONE, 0);
>> +virDomainAuditStop(vm, "destroyed");
>
> Queuing another event here that the domain is being destroyed seems both
> appropriate and weird to me. So I'll leave it up to you. It's not like
> anyone ever used this functionality... ever. ACK either way.
I think we want to queue the event here. Imagine that there's a mgmt app
that acts like HA daemon. Whenever a domain is destroyed it has to start
it up again. Well, with the current code it has to listen to RESET event
and depending on libvirt version it has to either ignore the event or
start it up again. However, if we queue the event all that the app has
to do is to listen to DESTROY event. Therefore I'm inclined to leave it
here.
Pushed, thanks.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/5] tests: add qemu chardev srouce reconnect tests
On Tue, Aug 29, 2017 at 11:17:13AM +0200, Michal Privoznik wrote: > On 08/28/2017 02:56 PM, Pavel Hrdina wrote: > > Signed-off-by: Pavel Hrdina > > --- > > .../qemuxml2argv-channel-reconnect.args| 31 +++ > > ...uxml2argv-chardev-reconnect-invalid-timeout.xml | 23 +++ > > .../qemuxml2argv-chardev-reconnect.args| 40 +++ > > .../qemuxml2argv-chardev-reconnect.xml | 46 > > ++ > > tests/qemuxml2argvtest.c | 11 ++ > > 5 files changed, 151 insertions(+) > > create mode 100644 > > tests/qemuxml2argvdata/qemuxml2argv-channel-reconnect.args > > create mode 100644 > > tests/qemuxml2argvdata/qemuxml2argv-chardev-reconnect-invalid-timeout.xml > > create mode 100644 > > tests/qemuxml2argvdata/qemuxml2argv-chardev-reconnect.args > > create mode 100644 > > tests/qemuxml2argvdata/qemuxml2argv-chardev-reconnect.xml > > > > diff --git a/tests/qemuxml2argvdata/qemuxml2argv-channel-reconnect.args > > b/tests/qemuxml2argvdata/qemuxml2argv-channel-reconnect.args > > new file mode 100644 > > index 00..43a5d5bb3e > > --- /dev/null > > +++ b/tests/qemuxml2argvdata/qemuxml2argv-channel-reconnect.args > > @@ -0,0 +1,31 @@ > > +LC_ALL=C \ > > +PATH=/bin \ > > +HOME=/home/test \ > > +USER=test \ > > +LOGNAME=test \ > > +QEMU_AUDIO_DRV=none \ > > +/usr/bin/qemu-system-i686 \ > > +-name QEMUGuest1 \ > > +-S \ > > +-M pc \ > > +-m 214 \ > > +-smp 1,sockets=1,cores=1,threads=1 \ > > +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ > > +-nographic \ > > +-nodefconfig \ > > +-nodefaults \ > > +-chardev > > socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ > > +server,nowait \ > > +-mon chardev=charmonitor,id=monitor,mode=readline \ > > +-no-acpi \ > > +-boot c \ > > +-device virtio-serial-pci,id=virtio-serial1,bus=pci.0,addr=0xa \ > > +-device virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x3 \ > > +-usb \ > > +-chardev socket,id=charchannel0,host=localhost,port=1234,reconnect=10 \ > > +-device virtserialport,bus=virtio-serial0.0,nr=1,chardev=charchannel0,\ > > +id=channel0,name=asdf \ > > +-chardev > > socket,id=charchannel1,path=/tmp/channel/domain--1-QEMUGuest1/fdsa,\ > > +server,nowait,reconnect=10 \ > > This doesn't look right. How come a server can have reconnect at the > same time? Yes, that is strange. If you look at the XML the mode is connect so there is something else going on. I'll investigate the issue. Also I've noticed that one file [1] shouldn't be included in that patch. I'll send a followup to clean that file. Pavel [1] signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 2/2] qemuDomainUndefineFlags: Grab QEMU_JOB_MODIFY
On 08/17/2017 01:24 PM, John Ferlan wrote: > > > On 08/15/2017 03:53 AM, Michal Privoznik wrote: >> This API is definitely modifying state of @vm. Therefore it >> should grab a job. >> >> Signed-off-by: Michal Privoznik >> --- >> src/qemu/qemu_driver.c | 25 +++-- >> 1 file changed, 15 insertions(+), 10 deletions(-) >> > > Reviewed-by: John Ferlan > Thanks, pushed. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/2] qemu: Introduce and use qemuDomainRemoveInactiveJob
On 08/17/2017 01:21 PM, John Ferlan wrote:
>
>
> On 08/15/2017 03:53 AM, Michal Privoznik wrote:
>> At some places we either already have synchronous job or we just
>> released it. Also, some APIs might want to use this code without
>> having to release their job. Anyway, the job acquire code is
>> moved out to qemuDomainRemoveInactiveJob so that
>> qemuDomainRemoveInactive does just what it promises.
>
> Feels like this is a partial thought as to what's being adjusted here. I
> think essentially you're trying to state that RemoveInactiveJob is a
> wrapper to RemoveInactive for paths that don't already have a non async
> job. For paths with an async job, that job must first be ended before
> calling/using the new InactiveJob API.
>
>>
>> Signed-off-by: Michal Privoznik
>> ---
>> src/qemu/qemu_domain.c| 36 +++-
>> src/qemu/qemu_domain.h| 3 +++
>> src/qemu/qemu_driver.c| 26 +-
>> src/qemu/qemu_migration.c | 10 +-
>> src/qemu/qemu_process.c | 10 +-
>> 5 files changed, 53 insertions(+), 32 deletions(-)
>>
>
> Should I assume you tested using the scenario from Martin's commit id
> 'b629c64e5'?
>
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index 40608554c..2b19f841c 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -5187,14 +5187,16 @@
>> qemuDomainSnapshotDiscardAllMetadata(virQEMUDriverPtr driver,
>> return rem.err;
>> }
>>
>> -/*
>> +
>> +/**
>> + * qemuDomainRemoveInactive:
>> + *
>> * The caller must hold a lock the vm.
>
> "hold a lock to the vm."
>
> And this should only be called if the caller has taken a non
> asynchronous job (right?)...
Not really. Async jobs are orthogonal to this. But it may be true that
currently only APIs that also set an async job call this function. But
that's just a coincidence. Also, if you look at qemuDomainSaveInternal()
there's no async job held when calling RemoveInactive().
>
>> */
>> void
>> qemuDomainRemoveInactive(virQEMUDriverPtr driver,
>> virDomainObjPtr vm)
>> {
>> -bool haveJob = true;
>> char *snapDir;
>> virQEMUDriverConfigPtr cfg;
>>
>> @@ -5205,9 +5207,6 @@ qemuDomainRemoveInactive(virQEMUDriverPtr driver,
>>
>> cfg = virQEMUDriverGetConfig(driver);
>>
>> -if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
>> -haveJob = false;
>> -
>> /* Remove any snapshot metadata prior to removing the domain */
>> if (qemuDomainSnapshotDiscardAllMetadata(driver, vm) < 0) {
>> VIR_WARN("unable to remove all snapshots for domain %s",
>> @@ -5240,13 +5239,32 @@ qemuDomainRemoveInactive(virQEMUDriverPtr driver,
>> */
>> virObjectLock(vm);
>> virObjectUnref(cfg);
>> -
>> -if (haveJob)
>> -qemuDomainObjEndJob(driver, vm);
>> -
>> virObjectUnref(vm);
>> }
>>
>> +
>> +/**
>> + * qemuDomainRemoveInactiveJob:
>> + *
>> + * Just like qemuDomainRemoveInactive but it tries to grab a
>> + * QEMU_JOB_MODIFY before. If it doesn't succeed in grabbing the
>
> s/before/first/
> s/If it doesn't/Even though it doesn't/
>
>> + * job the control carries with qemuDomainRemoveInactive though.
>
> s/job the control carries with/job, continue on with the/
> s/though/call/
>
>> + */
>> +void
>> +qemuDomainRemoveInactiveJob(virQEMUDriverPtr driver,
>> +virDomainObjPtr vm)
>> +{
>> +bool haveJob;
>> +
>> +haveJob = qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) >= 0;
>
> Is perhaps the reason this was failing because we already had a job in
> some instances?
Yeah, I blindly trusted the code. Maybe we need to re-evaluate if this
is still true. But lets save that for another day.
> Since this is a void path on the path to destruction
> failure probably won't matter, although I suppose it could be logged in
> some VIR_DEBUG/WARN manner. Not a requirement, just a thought.
It already is. Look at qemuDomainObjBeginJobInternal at error label.
>
>> +
>> +qemuDomainRemoveInactive(driver, vm);
>> +
>> +if (haveJob)
>> +qemuDomainObjEndJob(driver, vm);
>> +}
>> +
>> +
>> void
>> qemuDomainSetFakeReboot(virQEMUDriverPtr driver,
>> virDomainObjPtr vm,
>> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
>> index 4c9050aff..f93b09b69 100644
>> --- a/src/qemu/qemu_domain.h
>> +++ b/src/qemu/qemu_domain.h
>> @@ -611,6 +611,9 @@ int
>> qemuDomainSnapshotDiscardAllMetadata(virQEMUDriverPtr driver,
>> void qemuDomainRemoveInactive(virQEMUDriverPtr driver,
>>virDomainObjPtr vm);
>>
>> +void qemuDomainRemoveInactiveJob(virQEMUDriverPtr driver,
>> + virDomainObjPtr vm);
>> +
>> void qemuDomainSetFakeReboot(virQEMUDriverPtr driver,
>> virDomainObjPtr vm,
>> bool value);
>> diff --git a/src/qemu/qemu_drive
Re: [libvirt] [PATCH 5/5] tests: add qemu chardev srouce reconnect tests
On 08/28/2017 02:56 PM, Pavel Hrdina wrote: > Signed-off-by: Pavel Hrdina > --- > .../qemuxml2argv-channel-reconnect.args| 31 +++ > ...uxml2argv-chardev-reconnect-invalid-timeout.xml | 23 +++ > .../qemuxml2argv-chardev-reconnect.args| 40 +++ > .../qemuxml2argv-chardev-reconnect.xml | 46 > ++ > tests/qemuxml2argvtest.c | 11 ++ > 5 files changed, 151 insertions(+) > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-channel-reconnect.args > create mode 100644 > tests/qemuxml2argvdata/qemuxml2argv-chardev-reconnect-invalid-timeout.xml > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-chardev-reconnect.args > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-chardev-reconnect.xml > > diff --git a/tests/qemuxml2argvdata/qemuxml2argv-channel-reconnect.args > b/tests/qemuxml2argvdata/qemuxml2argv-channel-reconnect.args > new file mode 100644 > index 00..43a5d5bb3e > --- /dev/null > +++ b/tests/qemuxml2argvdata/qemuxml2argv-channel-reconnect.args > @@ -0,0 +1,31 @@ > +LC_ALL=C \ > +PATH=/bin \ > +HOME=/home/test \ > +USER=test \ > +LOGNAME=test \ > +QEMU_AUDIO_DRV=none \ > +/usr/bin/qemu-system-i686 \ > +-name QEMUGuest1 \ > +-S \ > +-M pc \ > +-m 214 \ > +-smp 1,sockets=1,cores=1,threads=1 \ > +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ > +-nographic \ > +-nodefconfig \ > +-nodefaults \ > +-chardev > socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ > +server,nowait \ > +-mon chardev=charmonitor,id=monitor,mode=readline \ > +-no-acpi \ > +-boot c \ > +-device virtio-serial-pci,id=virtio-serial1,bus=pci.0,addr=0xa \ > +-device virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x3 \ > +-usb \ > +-chardev socket,id=charchannel0,host=localhost,port=1234,reconnect=10 \ > +-device virtserialport,bus=virtio-serial0.0,nr=1,chardev=charchannel0,\ > +id=channel0,name=asdf \ > +-chardev socket,id=charchannel1,path=/tmp/channel/domain--1-QEMUGuest1/fdsa,\ > +server,nowait,reconnect=10 \ This doesn't look right. How come a server can have reconnect at the same time? Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] qemu: Honour
On Wed, Aug 16, 2017 at 04:38:09PM +0200, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1476866
For some reason, we completely ignore setting for
domains. The implementation is simply not there. It never was.
Signed-off-by: Michal Privoznik
---
diff to v1:
- dropped the spoofed logic
- Switch from qemuProcessShutdownOrReboot() to qemuPrcoessStop() because that's
what impl does too.
src/qemu/qemu_process.c | 27 ---
1 file changed, 24 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index fed2bc588..3df6c320e 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -484,6 +484,7 @@ qemuProcessHandleReset(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
virObjectEventPtr event;
qemuDomainObjPrivatePtr priv;
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+int ret = -1;
virObjectLock(vm);
@@ -495,12 +496,32 @@ qemuProcessHandleReset(qemuMonitorPtr mon
ATTRIBUTE_UNUSED,
if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps) <
0)
VIR_WARN("Failed to save status on vm %s", vm->def->name);
+if (vm->def->onReboot == VIR_DOMAIN_LIFECYCLE_DESTROY ||
+vm->def->onReboot == VIR_DOMAIN_LIFECYCLE_PRESERVE) {
+
+if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
+goto cleanup;
+
+if (!virDomainObjIsActive(vm)) {
+VIR_DEBUG("Ignoring RESET event from inactive domain %s",
+ vm->def->name);
+goto endjob;
+}
+
+qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_DESTROYED,
+QEMU_ASYNC_JOB_NONE, 0);
+virDomainAuditStop(vm, "destroyed");
Queuing another event here that the domain is being destroyed seems both
appropriate and weird to me. So I'll leave it up to you. It's not like
anyone ever used this functionality... ever. ACK either way.
+qemuDomainRemoveInactive(driver, vm);
+ endjob:
+qemuDomainObjEndJob(driver, vm);
+}
+
+ret = 0;
+ cleanup:
virObjectUnlock(vm);
-
qemuDomainEventQueue(driver, event);
-
virObjectUnref(cfg);
-return 0;
+return ret;
}
--
2.13.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 0/2] Yet another namespace fix, kinda
Fixing easy problem (patch 1) lead me to finding out that there is yet another problem that needs fixing (patch 2). For more information read the code. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1443434 Martin Kletzander (2): qemu: Don't mangle the storage format for type='dir' qemu: Also treat directories properly when using namespaces src/qemu/qemu_domain.c | 40 +++- src/storage/storage_source.c | 12 +--- tests/virstoragetest.c | 11 +-- 3 files changed, 53 insertions(+), 10 deletions(-) -- 2.14.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 2/2] qemu: Also treat directories properly when using namespaces
When recreating folders with namespaces, the directory type was not
being handled at all. It's not special, we probably just didn't know
that that can be used as a volume path as well. The code failed
gracefully, but we want to allow that so that we can use in domains again.
Partially-resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1443434
Signed-off-by: Martin Kletzander
---
src/qemu/qemu_domain.c | 40 +++-
1 file changed, 35 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 2c77a6442467..2549f9bf3290 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7855,6 +7855,7 @@ qemuDomainCreateDeviceRecursive(const char *device,
bool isLink = false;
bool isDev = false;
bool isReg = false;
+bool isDir = false;
bool create = false;
#ifdef WITH_SELINUX
char *tcon = NULL;
@@ -7879,6 +7880,7 @@ qemuDomainCreateDeviceRecursive(const char *device,
isLink = S_ISLNK(sb.st_mode);
isDev = S_ISCHR(sb.st_mode) || S_ISBLK(sb.st_mode);
isReg = S_ISREG(sb.st_mode) || S_ISFIFO(sb.st_mode) ||
S_ISSOCK(sb.st_mode);
+isDir = S_ISDIR(sb.st_mode);
/* Here, @device might be whatever path in the system. We
* should create the path in the namespace iff it's "/dev"
@@ -7996,6 +7998,10 @@ qemuDomainCreateDeviceRecursive(const char *device,
goto cleanup;
/* Just create the file here so that code below sets
* proper owner and mode. Bind mount only after that. */
+} else if (isDir) {
+if (create &&
+virFileMakePathWithMode(devicePath, sb.st_mode) < 0)
+goto cleanup;
} else {
virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
_("unsupported device type %s 0%o"),
@@ -8057,7 +8063,7 @@ qemuDomainCreateDeviceRecursive(const char *device,
#endif
/* Finish mount process started earlier. */
-if (isReg &&
+if ((isReg || isDir) &&
virFileBindMountDevice(device, devicePath) < 0)
goto cleanup;
@@ -8686,6 +8692,7 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid
ATTRIBUTE_UNUSED,
bool isLink = S_ISLNK(data->sb.st_mode);
bool isDev = S_ISCHR(data->sb.st_mode) || S_ISBLK(data->sb.st_mode);
bool isReg = S_ISREG(data->sb.st_mode) || S_ISFIFO(data->sb.st_mode) ||
S_ISSOCK(data->sb.st_mode);
+bool isDir = S_ISDIR(data->sb.st_mode);
qemuSecurityPostFork(data->driver->securityManager);
@@ -8741,6 +8748,23 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid
ATTRIBUTE_UNUSED,
delDevice = true;
/* Just create the file here so that code below sets
* proper owner and mode. Move the mount only after that. */
+} else if (isDir) {
+/* We are not cleaning up disks on virDomainDetachDevice
+ * because disk might be still in use by different disk
+ * as its backing chain. This might however clash here.
+ * Therefore do the cleanup here. */
+if (umount(data->file) < 0 &&
+errno != ENOENT && errno != EINVAL) {
+virReportSystemError(errno,
+ _("Unable to umount %s"),
+ data->file);
+goto cleanup;
+}
+if (virFileMakePathWithMode(data->file, data->sb.st_mode) < 0)
+goto cleanup;
+delDevice = true;
+/* Just create the folder here so that code below sets
+ * proper owner and mode. Move the mount only after that. */
} else {
virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
_("unsupported device type %s 0%o"),
@@ -8788,14 +8812,18 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid
ATTRIBUTE_UNUSED,
# endif
/* Finish mount process started earlier. */
-if (isReg &&
+if ((isReg || isDir) &&
virFileMoveMount(data->target, data->file) < 0)
goto cleanup;
ret = 0;
cleanup:
-if (ret < 0 && delDevice)
-unlink(data->file);
+if (ret < 0 && delDevice) {
+if (isDir)
+virFileDeleteTree(data->file);
+else
+unlink(data->file);
+}
# ifdef WITH_SELINUX
freecon(data->tcon);
# endif
@@ -8818,6 +8846,7 @@ qemuDomainAttachDeviceMknodRecursive(virQEMUDriverPtr
driver,
char *target = NULL;
bool isLink;
bool isReg;
+bool isDir;
if (!ttl) {
virReportSystemError(ELOOP,
@@ -8840,8 +8869,9 @@ qemuDomainAttachDeviceMknodRecursive(virQEMUDriverPtr
driver,
isLink = S_ISLNK(data.sb.st_mode);
isReg = S_ISREG(data.sb.st_mode) || S_ISFIFO(data.sb.st_mode) ||
S_ISSOCK(data.sb.st_mode);
+isDir = S_ISDIR(data.sb.st_mode);
-if (isReg && STRPREFIX(file, DEVPREFIX)) {
+if ((isReg || isDir) && STRPREFIX(file, DEVPREFIX)) {
cfg = virQEMUDriverGetConfig(driver);
if (!(target = qemuDomainGetPreservedMountPath(cfg, vm, file)))
goto cleanup;
--
[libvirt] [PATCH v2 1/2] qemu: Don't mangle the storage format for type='dir'
Our backing probing code handles directory file types properly in
virStorageFileGetMetadataRecurse(), by that I mean it leaves them
alone. However its caller, the virStorageFileGetMetadata() resets the
type to raw before probing, without even checking the type. We need
to special-case TYPE_DIR in order to achieve desired results.
Also, in order to properly test this, we need to stop resetting format
of volumes in tests for TYPE_DIR (probably the reason why we didn't
catch that and why the test data didn't need to be modified).
Partially-resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1443434
Signed-off-by: Martin Kletzander
---
src/storage/storage_source.c | 12 +---
tests/virstoragetest.c | 11 +--
2 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/src/storage/storage_source.c b/src/storage/storage_source.c
index b620153f1e5a..bf47622372ab 100644
--- a/src/storage/storage_source.c
+++ b/src/storage/storage_source.c
@@ -527,14 +527,20 @@ virStorageFileGetMetadata(virStorageSourcePtr src,
allow_probe, report_broken);
virHashTablePtr cycle = NULL;
+virStorageType actualType = virStorageSourceGetActualType(src);
int ret = -1;
if (!(cycle = virHashCreate(5, NULL)))
return -1;
-if (src->format <= VIR_STORAGE_FILE_NONE)
-src->format = allow_probe ?
-VIR_STORAGE_FILE_AUTO : VIR_STORAGE_FILE_RAW;
+if (src->format <= VIR_STORAGE_FILE_NONE) {
+if (actualType == VIR_STORAGE_TYPE_DIR)
+src->format = VIR_STORAGE_FILE_DIR;
+else if (allow_probe)
+src->format = VIR_STORAGE_FILE_AUTO;
+else
+src->format = VIR_STORAGE_FILE_RAW;
+}
ret = virStorageFileGetMetadataRecurse(src, src, uid, gid,
allow_probe, report_broken, cycle);
diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
index d83db78f566f..60e3164b0ac8 100644
--- a/tests/virstoragetest.c
+++ b/tests/virstoragetest.c
@@ -111,7 +111,6 @@ testStorageFileGetMetadata(const char *path,
if (stat(path, &st) == 0) {
if (S_ISDIR(st.st_mode)) {
ret->type = VIR_STORAGE_TYPE_DIR;
-ret->format = VIR_STORAGE_FILE_DIR;
} else if (S_ISBLK(st.st_mode)) {
ret->type = VIR_STORAGE_TYPE_BLOCK;
}
@@ -963,7 +962,15 @@ mymain(void)
.type = VIR_STORAGE_TYPE_DIR,
.format = VIR_STORAGE_FILE_DIR,
};
-TEST_CHAIN(absdir, VIR_STORAGE_FILE_AUTO,
+testFileData dir_as_raw = {
+.path = canondir,
+.type = VIR_STORAGE_TYPE_DIR,
+.format = VIR_STORAGE_FILE_RAW,
+};
+TEST_CHAIN(absdir, VIR_STORAGE_FILE_RAW,
+ (&dir_as_raw), EXP_PASS,
+ (&dir_as_raw), ALLOW_PROBE | EXP_PASS);
+TEST_CHAIN(absdir, VIR_STORAGE_FILE_NONE,
(&dir), EXP_PASS,
(&dir), ALLOW_PROBE | EXP_PASS);
TEST_CHAIN(absdir, VIR_STORAGE_FILE_DIR,
--
2.14.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] Add entry for ZStack to apps page
Signed-off-by: Shuang He --- docs/apps.html.in | 11 +++ 1 file changed, 11 insertions(+) diff --git a/docs/apps.html.in b/docs/apps.html.in index 1ced03c..06bf8a2 100644 --- a/docs/apps.html.in +++ b/docs/apps.html.in @@ -286,6 +286,17 @@ perfect for setting up low-end servers in a cloud or a cloud where you want the most bang for the bucks. + + http://en.zstack.io/";>ZStack + +ZStack is open source IaaS software aiming to automate +datacenters, managing resources of compute, storage, +and networking all by APIs. Users can setup ZStack +environments in a download-and-run manner, spending 5 minutes +building a POC environment all on a single Linux machine, +or 30 minutes building a multi-node production environment +that can scale to hundreds of thousands of physical servers. + Libraries -- 1.8.3.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] qemu: Honour
On 08/16/2017 04:38 PM, Michal Privoznik wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1476866 > > For some reason, we completely ignore setting for > domains. The implementation is simply not there. It never was. > > Signed-off-by: Michal Privoznik > --- > > diff to v1: > - dropped the spoofed logic > - Switch from qemuProcessShutdownOrReboot() to qemuPrcoessStop() because > that's > what impl does too. > > src/qemu/qemu_process.c | 27 --- > 1 file changed, 24 insertions(+), 3 deletions(-) Ping. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt-php][PATCH 0/3] Couple of trivial fixes/improvements
On Tue, Aug 29, 2017 at 09:57:55AM +0200, Michal Privoznik wrote: On 08/29/2017 09:55 AM, Martin Kletzander wrote: On Mon, Aug 28, 2017 at 04:45:45PM +0200, Michal Privoznik wrote: *** BLURB HERE *** Michal Privoznik (3): Shut up automake libvirt-php.c: Reorder includes s/OCD/CDO/ ? =D :-) Seems sane, ACK series. Oh these are already pushed. Since there were no reviewers for -php I've started to push patches and then merely send them to the list for others to know. Should have said that in the cover letter, sorry. No need to, you can push it under the rule of well nobody actually reads them. In that case maybe there is no need to send them to the ML? But I'd leave that up to you. I just didn't see anyone in similar position doing that before ;) Michal signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt-php][PATCH 0/3] Couple of trivial fixes/improvements
On 08/29/2017 09:55 AM, Martin Kletzander wrote: > On Mon, Aug 28, 2017 at 04:45:45PM +0200, Michal Privoznik wrote: >> *** BLURB HERE *** >> >> Michal Privoznik (3): >> Shut up automake >> libvirt-php.c: Reorder includes > > s/OCD/CDO/ ? =D :-) > > Seems sane, ACK series. Oh these are already pushed. Since there were no reviewers for -php I've started to push patches and then merely send them to the list for others to know. Should have said that in the cover letter, sorry. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt-php][PATCH 0/3] Couple of trivial fixes/improvements
On Mon, Aug 28, 2017 at 04:45:45PM +0200, Michal Privoznik wrote: *** BLURB HERE *** Michal Privoznik (3): Shut up automake libvirt-php.c: Reorder includes s/OCD/CDO/ ? =D Seems sane, ACK series. examples: Initialize $ret in index.php configure.ac | 4 examples/index.php | 1 + src/libvirt-php.c | 8 3 files changed, 9 insertions(+), 4 deletions(-) -- 2.13.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 1/3] Add support for Veritas HyperScale (VxHS) block device protocol
Hi Peter, Patch series V5 addresses most of the review comments from this email. I'm working on the other set of review comments from you and John, and will send updates soon. On Fri, Jun 30, 2017 at 1:22 AM, Peter Krempa wrote: > On Thu, Jun 29, 2017 at 19:02:39 -0700, Ashish Mittal wrote: >> From: Ashish Mittal >> >> Sample XML for a VxHS disk: >> >> >> >> >> >> >> >> >> eb90327c-8302-4725-9e1b-4e85ed4dc251 >> >> >> >> >> Signed-off-by: Ashish Mittal >> --- >> v2 changelog: >> (1) Added code for JSON parsing of a VxHS vdisk. >> (2) Added test case to verify JSON parsing. >> (3) Added missing switch-case checks for VIR_STORAGE_NET_PROTOCOL_VXHS. >> (4) Fixed line wrap in qemuxml2argv-disk-drive-network-vxhs.args. >> >> v3 changelog: >> (1) Implemented the modern syntax for VxHS disk specification. >> (2) Changed qemuxml2argvdata VxHS test case to verify the new syntax. >> (3) Added a negative test case to check failure when multiple hosts >> are specified for a VxHS disk. >> >> v4 changelog: >> (1) Fixes per review comments from v3. >> (2) Had to remove a test from the previous version that checked for >> error when multiple hosts are specified for VxHS device. >> This started failing virschematest with the error >> "XML document failed to validate against schema" as the >> docs/schemas/domain.rng specifies only a single host. >> >> docs/formatdomain.html.in | 15 - >> docs/schemas/domaincommon.rng | 13 >> src/libxl/libxl_conf.c | 1 + >> src/qemu/qemu_command.c| 70 >> ++ >> src/qemu/qemu_driver.c | 3 + >> src/qemu/qemu_parse_command.c | 25 >> src/util/virstoragefile.c | 64 +++- >> src/util/virstoragefile.h | 1 + >> src/xenconfig/xen_xl.c | 1 + >> .../qemuargv2xml-disk-drive-network-vxhs-fail.args | 24 >> tests/qemuargv2xmltest.c | 17 +- >> .../qemuxml2argv-disk-drive-network-vxhs.args | 25 >> .../qemuxml2argv-disk-drive-network-vxhs.xml | 34 +++ >> tests/qemuxml2argvtest.c | 1 + >> tests/virstoragetest.c | 19 ++ >> 15 files changed, 308 insertions(+), 5 deletions(-) >> create mode 100644 >> tests/qemuargv2xmldata/qemuargv2xml-disk-drive-network-vxhs-fail.args >> create mode 100644 >> tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args >> create mode 100644 >> tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.xml >> >> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in >> index 36bea67..62d67f4 100644 >> --- a/docs/formatdomain.html.in >> +++ b/docs/formatdomain.html.in > > [...] > >> @@ -2511,6 +2511,9 @@ >>target's name by a slash (e.g., >>iqn.2013-07.com.example:iscsi-pool/1). If not >>specified, the default LUN is zero. >> + For "vxhs" (since 3.3.0), thea > > 3.6.0 at best. Changed to 3.8.0 in all places. > >> + name is the UUID of the volume, assigned by the >> + HyperScale sever. >>Since 0.8.7 >> >> volume > > [...] > >> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng >> index bdf7103..7525a2a 100644 >> --- a/docs/schemas/domaincommon.rng >> +++ b/docs/schemas/domaincommon.rng >> @@ -1613,6 +1613,18 @@ >> >> >> >> + >> + >> + >> + >> + vxhs >> + >> + >> + >> + > > This is misaligned. > Fixed. >> + >> + >> + >> >> >>network > > [...] > >> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c >> index c53ab97..8e00782 100644 >> --- a/src/qemu/qemu_command.c >> +++ b/src/qemu/qemu_command.c >> @@ -524,6 +524,7 @@ qemuNetworkDriveGetPort(int protocol, >> return 0; >> >> case VIR_STORAGE_NET_PROTOCOL_RBD: >> +case VIR_STORAGE_NET_PROTOCOL_VXHS: >> case VIR_STORAGE_NET_PROTOCOL_LAST: >> case VIR_STORAGE_NET_PROTOCOL_NONE: >> /* not applicable */ > > Now we are in the section of stuff which should be split into a separate > patch. > > Also here you declare that there,s no default port ... (and you said) > qemuNetworkDriveGetPort() NA in the latest code. Added to virStorageSourceNetworkDefaultPort() >> @@ -931,6 +932,65 @@ qemuBuildGlusterDriveJSON(virStorageSourcePtr src) >> } >> >> >> +#define QEMU_DEFAULT_VXHS_PORT "" > > And here you declare the default port via a ... define. I'd suggest you > stick with the common code paths. > The now-defunct qemuNetworkDriveGetPort() was not used in the JSON code path, hence I had omitted it. Added to virStorageSourceNetwo
Re: [libvirt] [PATCH v2 1/3] conf: Properly truncate wide character names in virDomainObjGetShortName
On Fri, Aug 25, 2017 at 06:03:15PM -0400, John Ferlan wrote:
On 08/25/2017 07:21 AM, Martin Kletzander wrote:
We always truncated the name at 20 bytes instead of characters. In
case 20 bytes were in the middle of a multi-byte character, then the
string became invalid and various parts of the code would error
out (e.g. XML parsing of that string). Let's instead properly
truncate it after 20 characters instead.
We cannot test this in our test suite because we would need to know
what locales are installed on the system where the tests are ran and
if there is supported one (most probably there will be, but we cannot
be 100% sure), we could initialize gettext in qemuxml2argvtest, but
there would still be a chance of getting two different (both valid,
though) results.
Yeah - I tried - it got ugly fast Although I suppose in a test
environment would could just "pick" a charset like en_US.UTF-8 and just
ensure that things work as expected with that. Perhaps even make the
shortened name tests go at the bottom with a FAILURE to compare argv
results before setting the locale and a success after a call on the same
test by name. Not something for this series because I'm not even sure it
would work properly. Maybe something for the byte sized tasks (rather
literally too ;-)!)
The problem is that we can't pick any charset except "C", which is the
default. There is no guarantee that you will have the charset installed
in the system. For example I only have en_GB.utf8, cs_CZ.utf8 and the
defaults, C and POSIX. We could, maybe, programmatically construct the
locale in the test itself, but that's way big of a hammer IMO.
In order to test this it is enough to start a machine with a name for
which trimming it after 20 bytes would create invalid sequence (e.g.
1234567890123456789č where č is any multi-byte character). Then start
the domain and restart libvirtd. The domain would disappear because
such illegal sequence will not go through the XML parser. And that's
not a bug of the parser, it should not be in the XML in the first
place, but since we don't use any sophisticated formatter, just
mash some strings together, the formatting succeeds.
If the domain was started before these patches, then the domain is still
hidden since the shortened path names won't match... Such is life...
Such domain will never be loaded. It's not because the paths won't
match. We don't generate the paths for running domains, but we get them
from the state XML (that's needed so that we can change the way we
shorten characters and still be able to find domains with older
"shortening rules"). The problem with the domains that get lost is that
the XML consists of an invalid string literal and it is already saved on
the disk. The parser (libxml2) will fail (rightfully) with an error
that such XML is invalid. We can't do anything with that. And,
honestly, I can't say I care that much about that as this really isn't
someone would notice in production.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1448766
Signed-off-by: Martin Kletzander
---
src/conf/domain_conf.c | 45 ++---
1 file changed, 42 insertions(+), 3 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 47eba4dbb315..dd73158f028b 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -27131,6 +27131,8 @@ virDomainDefHasMemballoon(const virDomainDef *def)
}
+#define VIR_DOMAIN_SHORT_NAME_MAX 20
+
/**
* virDomainObjGetShortName:
* @vm: Machine for which to get a name
@@ -27141,15 +27143,52 @@ virDomainDefHasMemballoon(const virDomainDef *def)
char *
virDomainObjGetShortName(const virDomainDef *def)
{
-const int dommaxlen = 20;
+wchar_t wshortname[VIR_DOMAIN_SHORT_NAME_MAX + 1] = {0};
+size_t len = 0;
+char *shortname = NULL;
char *ret = NULL;
-ignore_value(virAsprintf(&ret, "%d-%.*s",
- def->id, dommaxlen, def->name));
+/* No need to do the whole conversion thing when there are no multibyte
+ * characters. The same applies for illegal sequences as they can occur
+ * with incompatible locales. */
+len = mbstowcs(NULL, def->name, 0);
+if ((len == (size_t) -1 && errno == EILSEQ) ||
+len == strlen(def->name)) {
+ignore_value(virAsprintf(&ret, "%d-%.*s", def->id,
+ VIR_DOMAIN_SHORT_NAME_MAX, def->name));
+return ret;
consistently speaking
return NULL;
Not really, ret will be NULL here if and only if virAsprintf() failed.
This is not an error path, but rather a fallback path if either the
locale is incompatible or there are no wide characters (your
suggestion). I hoped I explained that enough in the comment.
+}
+
+if (len == (size_t) -1 ||
+mbstowcs(wshortname, def->name, VIR_DOMAIN_SHORT_NAME_MAX) == (size_t)
-1) {
+virReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("Cannot convert d
Re: [libvirt] [PATCH 0/2] Alter refresh algorithm for volWipe
On Fri, Aug 25, 2017 at 06:11:49PM -0400, John Ferlan wrote: On 08/25/2017 08:28 AM, Martin Kletzander wrote: On Fri, Aug 25, 2017 at 07:52:25AM -0400, John Ferlan wrote: On 08/25/2017 05:44 AM, Martin Kletzander wrote: On Thu, Aug 24, 2017 at 06:28:27PM -0400, John Ferlan wrote: Alter wipeVol to do same refresh operation as pool refresh would do. I think we should rather keep the format as it is. Did I miss something here or isn't the source of the problem just the fact that we wipe the volume without keeping the format? Once you "wipe" the file/image the format that was there (such as qcow2 as noted in the bz from patch2) is no longer there. Consider the following sequence: virsh vol-create-as default bz 10M --format qcow2 virsh vol-dumpxml bz default | grep format qemu-img info /home/vm-images/bz image: /home/vm-images/bz file format: qcow2 virtual size: 10M (10485760 bytes) disk size: 196K cluster_size: 65536 Format specific information: compat: 0.10 refcount bits: 16 virsh vol-wipe bz default qemu-img info /home/vm-images/bz image: /home/vm-images/bz file format: raw virtual size: 196K (200704 bytes) disk size: 196K virsh vol-dumpxml bz default | grep format (without the patch in 2/2 of course) BTW: I did consider just changing the format to RAW regardless, but figured that was just too simple and may not be the right answer for every case. Sure, but that's not my point. My point is that instead of rewriting the data with zeros, we could re-initialize it. Anyway, from what I see in the docs, we "fixed" it by documenting this behaviour already, so my point is moo: https://libvirt.org/html/libvirt-libvirt-storage.html#virStorageVolWipe Doh! I should have looked there too... Interesting though... So should we instead close this bug as documented to work that way? Or should the text there be modified as well since the bz will cause the alteration to at least RAW for "most" storage backends - I think it's only the disk backend w/ an extended partition and a sparse logical volume that cannot be wiped, but those fail the wipe - so we'd never get to the change the target.format code. I think that all the problems are essentially caused by people misunderstanding the API's purpose. That's why I ACKed it, as we at least report the right information. I think your solution is precisely the midpoint between not doing anything and fixing all corner cases. John (I guess I should know that when I pushed that documentation into the tree) So ACK series. signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
