Fwd: FW: [libvirt PATCH 0/6] Introduce Local Migration Support in Libvirt
On 2/3/20, 7:16 PM, "Daniel P. Berrangé" wrote: On Mon, Feb 03, 2020 at 10:42:48AM -0300, Daniel Henrique Barboza wrote: > Hi Daniel, > > I am happy that Libvirt is pushing local migration/live patching support, but > at the same time I am wondering what changed from what you said here: Err, this isn't libvirt pushing local migration. I'm simply re-posting these patches on behalf of Shaju who is unable to post the patches due to our broken mail server. Don't take this as meaning that I approve of the patches. They're simply here for discussion as any other patch proposal is. Thank you for forwarding the patch to the list, Danpb. That is largely still my view. Sure, and we will be happy to discuss this further, as noted below :) > To give you a background, we have live patching enhancements in IBM backlog > since a few years ago, and one on the reasons these were being postponed > time and time again were the lack of Libvirt support and this direction of > "Libvirt is not interested in supporting it". And this message above was being > used internally as the rationale for it. Hi Daniel HB, Thank you for pointing out the fact that this has been in discussion since 2013. While Shaju's patches were independent as an RFC, we will be happy to collaborate to push for a joint solution. The fact that this has been requested time and again, and the fact that most commercial cloud deployments out there already have an in-place upgrade story [1] [2] -- should be good reason we holistically examine the use case once again. [1] https://kb.vmware.com/s/article/2005389 [2] https://dl.acm.org/doi/10.1145/3297858.3304034 Danpb had explained in much detail as to why mangling file and particularly socket paths can be messy in this patchset. However, even if libvirtd blocks in-place migrations for such legacy VMs until apps switch to more stringent XML semantics, it still may help cutting edge apps that intend to leverage this. I understand the presence of collision-causing file and socket paths can easily be checked as pre-migration checks, and should be trivial to implement. We can include a revised patchset with this check in place. Support for this feature has been present in qemu for a while for this use-case, and so maybe it is time we pass on the goodness up the stack as well. Happy to discuss more details on implementation and semantics, Warm regards, Prerna Saxena
Re: [libvirt] [ v3 2/4] 1) Loader: Add a more elaborate definition.
On Mon, Jun 4, 2018 at 6:24 PM, John Ferlan wrote:
>
> On 05/21/2018 07:10 AM, Prerna Saxena wrote:
> > Augment definition to include virStorageSourcePtr that
> > more comprehensively describes the nature of backing element.
> > Also include flags for annotating if input XML definition is
> > old-style or new-style.
> >
> > 2) Parse domain XML to generate virDomainLoaderDef & virDomainNvramDef.
> >
> > This patch is used to interpret domain XML and store the Loader &
> > Nvram's backing definitions as virStorageSource.
> > It also identifies if input XML used old or new-style declaration.
> > (This will later be used by the formatter).
> >
> > 3) Format the loader source appropriately.
> >
> > If the initial XML used the old-style declaration as follows:
> > /path/to/file
> >
> > we format it as was read.
> >
> > However, if it used new-style declaration:
> >
> >
> >
> >
> > The formatter identifies that this is a new-style format
> > and renders it likewise.
> >
> > 4) Plumb the loader source into generation of QEMU command line.
> >
> > Given that nvram & loader elements can now be backed by a non-local
> > source too, adjust all steps leading to generation of
> > QEMU command line.
> >
> > 5) Fix the domain def inference logic to correctly account for
> network-backed
> > pflash devices.
> >
> > 6) Bhyve: Fix command line generation to correctly pick up local loader
> path.
> >
> > 7) virt-aa-helper: Adjust references to loader & nvram elements to
> correctly
> > parse the virStorageSource types.
> >
> > 8) Vbox: Adjust references to 'loader' and 'nvram' elements given that
> > these are now represented by virStorageSourcePtr.
> >
> > 9)Xen: Adjust all references to loader & nvram elements given that they
> are now backed by virStorageSourcePtr
> > ---
> > src/bhyve/bhyve_command.c | 6 +-
> > src/conf/domain_conf.c | 250 ++
> +++---
> > src/conf/domain_conf.h | 11 +-
> > src/qemu/qemu_cgroup.c | 13 ++-
> > src/qemu/qemu_command.c | 21 +++-
> > src/qemu/qemu_domain.c | 31 +++--
> > src/qemu/qemu_driver.c | 7 +-
> > src/qemu/qemu_parse_command.c | 30 -
> > src/qemu/qemu_process.c | 54 ++---
> > src/security/security_dac.c | 6 +-
> > src/security/security_selinux.c | 6 +-
> > src/security/virt-aa-helper.c | 14 ++-
> > src/vbox/vbox_common.c | 11 +-
> > src/xenapi/xenapi_driver.c | 4 +-
> > src/xenconfig/xen_sxpr.c| 19 +--
> > src/xenconfig/xen_xm.c | 9 +-
> > 16 files changed, 409 insertions(+), 83 deletions(-)
> >
>
> The $SUBJ and commit message are just poorly formatted.
>
> But it pretty much shows that everything just got thrown into this one
> patch and you went from there.
>
> This series needs to progress a bit more "reasonably" to the desired
> goal. Consider this progression (with the following as patch 1 of the
> entire series):
>
> 1. Change path of loader to union:
>
> struct _virDomainLoaderDef {
> union {
> char *path;
> } loader;
>
> then compile/fix up references.
>
> 2. Create an accessor to loader.path - that way future consumers can
> just fetch the source path, e.g.:
>
> const char *
> virDomainLoaderDefGetLoaderPath(virDomainLoaderDefPtr loader)
> {
> return loader->loader.path;
> }
>
> Anything that accesses loader.path should change to use this via
> something like:
>
> const char *loaderPath;
> ...
> loaderPath = virDomainLoaderDefGetLoaderPath(xxx)
> ...
>
> Eventually this code will return either loader.path or loader.src->path
> since both FILE and NETWORK storage use src->path.
>
> 3. Add virStorageSourcePtr to the loader union and modify places in the
> code to use/read it. Update the domaincommon.rng, update formatdomain to
> describe usage of for , add genericxml2xmltests for
> both "loader-source-file" and "loader-source-network" type formats for
> at least "type='rom'". You could add type='pflash' tests too...
>
> ...
> union {
> char *path;
> virStorageSourcePtr src;
> } loader;
> bool useLoaderSrc;
> ...
>
> The patch code to parse needs to be changed to follow more closely what
> virDomainDisk{BackingStore|DefMirror}Parse does... Alter ctxt locally
>
Re: [libvirt] [ v3 0/4] Introduce network-backed loader & NVRAM.
On Mon, May 21, 2018 at 4:40 PM, Prerna Saxena wrote: > Libvirt domain XML allows only local filepaths to specify a loader element > or its matching NVRAM. Given that VMs may themselves move across hypervisor > hosts, it should be possible to allocate loaders/NVRAM disks on network > storage > for uninterrupted access. > > This series extends the loader & NVRAM disk elements to be described as > virStorageSource* elements, as discussed in : > https://www.redhat.com/archives/libvir-list/2018-March/msg01721.html > > Sample XML with new annotation: > > > > > > > > > > > References: > -- > v0/ Proposal: https://www.redhat.com/archives/libvir-list/2018- > March/msg01721.html.v1 > v1: https://www.redhat.com/archives/libvir-list/2018-April/msg02024.html > v2: https://www.redhat.com/archives/libvir-list/2018-May/msg00948.html > > Changelog: > - > Changes since v2: > - Consolidated patches with related data structures to avoid build > breakage. > - Passes make check & make syntax-check. > > Prerna Saxena (4): > Schema: Introduce XML schema for network-backed loader and nvram > elements. > Loader: Add a more elaborate definition. > Test: Add a test snippet to evaluate command line generation for > loader/nvram specified via virStorageSource > Documentation: Add a blurb for the newly added XML snippets for loader > and nvram. > > docs/formatdomain.html.in | 36 +++- > docs/schemas/domaincommon.rng | 108 +-- > src/bhyve/bhyve_command.c | 6 +- > src/conf/domain_conf.c | 250 > +++-- > src/conf/domain_conf.h | 11 +- > src/qemu/qemu_cgroup.c | 13 +- > src/qemu/qemu_command.c| 21 ++- > src/qemu/qemu_domain.c | 31 ++- > src/qemu/qemu_driver.c | 7 +- > src/qemu/qemu_parse_command.c | 30 ++- > src/qemu/qemu_process.c| 54 -- > src/security/security_dac.c| 6 +- > src/security/security_selinux.c| 6 +- > src/security/virt-aa-helper.c | 14 +- > src/vbox/vbox_common.c | 11 +- > src/xenapi/xenapi_driver.c | 4 +- > src/xenconfig/xen_sxpr.c | 19 +- > src/xenconfig/xen_xm.c | 9 +- > tests/qemuxml2argvdata/bios-nvram-network.args | 31 +++ > tests/qemuxml2argvdata/bios-nvram-network.xml | 42 + > tests/qemuxml2argvtest.c | 1 + > 21 files changed, 606 insertions(+), 104 deletions(-) > create mode 100644 tests/qemuxml2argvdata/bios-nvram-network.args > create mode 100644 tests/qemuxml2argvdata/bios-nvram-network.xml > > -- > Just FYI, I will be on vacation starting tomorrow until June 4. I will address all review comments as soon as I'm back. Regards, Prerna -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [ v3 2/4] 1) Loader: Add a more elaborate definition.
Augment definition to include virStorageSourcePtr that
more comprehensively describes the nature of backing element.
Also include flags for annotating if input XML definition is
old-style or new-style.
2) Parse domain XML to generate virDomainLoaderDef & virDomainNvramDef.
This patch is used to interpret domain XML and store the Loader &
Nvram's backing definitions as virStorageSource.
It also identifies if input XML used old or new-style declaration.
(This will later be used by the formatter).
3) Format the loader source appropriately.
If the initial XML used the old-style declaration as follows:
/path/to/file
we format it as was read.
However, if it used new-style declaration:
The formatter identifies that this is a new-style format
and renders it likewise.
4) Plumb the loader source into generation of QEMU command line.
Given that nvram & loader elements can now be backed by a non-local
source too, adjust all steps leading to generation of
QEMU command line.
5) Fix the domain def inference logic to correctly account for network-backed
pflash devices.
6) Bhyve: Fix command line generation to correctly pick up local loader path.
7) virt-aa-helper: Adjust references to loader & nvram elements to correctly
parse the virStorageSource types.
8) Vbox: Adjust references to 'loader' and 'nvram' elements given that
these are now represented by virStorageSourcePtr.
9)Xen: Adjust all references to loader & nvram elements given that they are now
backed by virStorageSourcePtr
---
src/bhyve/bhyve_command.c | 6 +-
src/conf/domain_conf.c | 250 +---
src/conf/domain_conf.h | 11 +-
src/qemu/qemu_cgroup.c | 13 ++-
src/qemu/qemu_command.c | 21 +++-
src/qemu/qemu_domain.c | 31 +++--
src/qemu/qemu_driver.c | 7 +-
src/qemu/qemu_parse_command.c | 30 -
src/qemu/qemu_process.c | 54 ++---
src/security/security_dac.c | 6 +-
src/security/security_selinux.c | 6 +-
src/security/virt-aa-helper.c | 14 ++-
src/vbox/vbox_common.c | 11 +-
src/xenapi/xenapi_driver.c | 4 +-
src/xenconfig/xen_sxpr.c| 19 +--
src/xenconfig/xen_xm.c | 9 +-
16 files changed, 409 insertions(+), 83 deletions(-)
diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
index e3f7ded..9e53f40 100644
--- a/src/bhyve/bhyve_command.c
+++ b/src/bhyve/bhyve_command.c
@@ -521,10 +521,12 @@ virBhyveProcessBuildBhyveCmd(virConnectPtr conn,
virCommandAddArgList(cmd, "-s", "0:0,hostbridge", NULL);
if (def->os.bootloader == NULL &&
-def->os.loader) {
+def->os.loader &&
+def->os.loader->src &&
+def->os.loader->src->type == VIR_STORAGE_TYPE_FILE) {
if ((bhyveDriverGetCaps(conn) & BHYVE_CAP_LPC_BOOTROM)) {
virCommandAddArg(cmd, "-l");
-virCommandAddArgFormat(cmd, "bootrom,%s", def->os.loader->path);
+virCommandAddArgFormat(cmd, "bootrom,%s",
def->os.loader->src->path);
add_lpc = true;
} else {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 3689ac0..df2ed3a 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2884,8 +2884,8 @@ virDomainLoaderDefFree(virDomainLoaderDefPtr loader)
if (!loader)
return;
-VIR_FREE(loader->path);
-VIR_FREE(loader->nvram);
+virStorageSourceFree(loader->src);
+virStorageSourceFree(loader->nvram);
VIR_FREE(loader->templt);
VIR_FREE(loader);
}
@@ -18087,30 +18087,81 @@
virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def)
static int
virDomainLoaderDefParseXML(xmlNodePtr node,
+ xmlXPathContextPtr ctxt,
virDomainLoaderDefPtr loader)
{
int ret = -1;
char *readonly_str = NULL;
char *secure_str = NULL;
char *type_str = NULL;
+char *tmp = NULL;
+char *lpath = NULL;
+xmlNodePtr cur;
+xmlXPathContextPtr cur_ctxt = ctxt;
+
+if (VIR_ALLOC(loader->src) < 0)
+goto error;
+
+loader->src->type = VIR_STORAGE_TYPE_LAST;
+loader->oldStyleLoader = false;
readonly_str = virXMLPropString(node, "readonly");
secure_str = virXMLPropString(node, "secure");
type_str = virXMLPropString(node, "type");
-loader->path = (char *) xmlNodeGetContent(node);
+
+if ((tmp = virXMLPropString(node, "backing")) &&
+(loader->src->type = virStorageTypeFromString(tmp)) <= 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unknown loader type '%s'"), tmp);
+VIR_FREE(tmp);
+goto error;
+}
+VIR_FREE(tmp);
+
+for (cur = node->children; cur != NULL; cur = cur->next) {
+if (cur->type != XML_ELEMENT_NODE)
+continue;
+
+if (virXMLNodeNameEqual(cur, "source")) {
+/* new-style declaration foun
[libvirt] [ v3 4/4] Documentation: Add a blurb for the newly added XML snippets for loader and nvram.
Signed-off-by: Prerna Saxena --- docs/formatdomain.html.in | 36 +--- 1 file changed, 33 insertions(+), 3 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 0d0fd3b..2ba2761 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -112,6 +112,20 @@ </os> ... +Where loader/NVRAM can also be described as: + + +... + <loader readonly='yes' secure='no' type='rom' backing='file'> +<source file='/usr/share/OVMF/OVMF_CODE.fd'/> + </loader> + <nvram backing='network' template='/usr/share/OVMF/OVMF_VARS.fd'> +<source protocol='iscsi' name='iqn.2013-07.com.example:iscsi-nopool/2'> + <host name='example.com' port='3260'/> +</source> + </nvram> +... + type The content of the type element specifies the @@ -143,7 +157,15 @@ pflash. Moreover, some firmwares may implement the Secure boot feature. Attribute secure can be used then to control it. -Since 2.1.0 +Since 2.1.0Since 4.5.0 +Loader element can also be specified as a remote disk. The attribute +backing (accepted values are +file and network)can be used to represent +local absolute paths as well as network-backed disks. The details of +backing file may be specified as storage source +Allowed backing type file replaces the old +specification and extends to all hypervisors that supported it, while +type network is only supported by QEMU. nvram Some UEFI firmwares may want to use a non-volatile memory to store some variables. In the host, this is represented as a file and the @@ -154,7 +176,15 @@ from the config file. Note, that for transient domains if the NVRAM file has been created by libvirt it is left behind and it is management application's responsibility to save and remove file (if needed to be -persistent). Since 1.2.8 +persistent). Since 1.2.8 +Since 4.5.0, attribute +backing(accepted values file +and network)can be used to specify a +storage source +of type file or network. The value filedescribes +absolute NVRAM paths and extends the present specification +for all hypervisors that support this, while +network applies only to QEMU. boot The dev attribute takes one of the values "fd", "hd", "cdrom" or "network" and is used to specify the next boot device @@ -2726,7 +2756,7 @@ - source + Disk source Representation of the disk source depends on the disk type attribute value as follows: -- 1.8.1.2 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [ v3 3/4] Test: Add a test snippet to evaluate command line generation for loader/nvram specified via virStorageSource
Signed-off-by: Prerna Saxena
---
tests/qemuxml2argvdata/bios-nvram-network.args | 31 +++
tests/qemuxml2argvdata/bios-nvram-network.xml | 42 ++
tests/qemuxml2argvtest.c | 1 +
3 files changed, 74 insertions(+)
create mode 100644 tests/qemuxml2argvdata/bios-nvram-network.args
create mode 100644 tests/qemuxml2argvdata/bios-nvram-network.xml
diff --git a/tests/qemuxml2argvdata/bios-nvram-network.args
b/tests/qemuxml2argvdata/bios-nvram-network.args
new file mode 100644
index 000..2f98fe6
--- /dev/null
+++ b/tests/qemuxml2argvdata/bios-nvram-network.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-x86_64 \
+-name test-bios \
+-S \
+-machine pc,accel=tcg,usb=off,dump-guest-core=off \
+-drive file=/usr/share/OVMF/OVMF_CODE.fd,if=pflash,format=raw,unit=0,\
+readonly=on \
+-drive file=iscsi://example.org:6000/iqn.1992-01.com.example/0,if=pflash,\
+format=raw,unit=1 \
+-m 1024 \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid 362d1fc1-df7d-193e-5c18-49a71bd1da66 \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-test-bios/monitor.sock,\
+server,nowait \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-boot order=c,menu=on \
+-usb \
+-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
+-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \
+-device usb-tablet,id=input0,bus=usb.0,port=1 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git a/tests/qemuxml2argvdata/bios-nvram-network.xml
b/tests/qemuxml2argvdata/bios-nvram-network.xml
new file mode 100644
index 000..d0b04ea
--- /dev/null
+++ b/tests/qemuxml2argvdata/bios-nvram-network.xml
@@ -0,0 +1,42 @@
+
+ test-bios
+ 362d1fc1-df7d-193e-5c18-49a71bd1da66
+ 1048576
+ 1048576
+ 1
+
+hvm
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+ destroy
+ restart
+ restart
+
+/usr/bin/qemu-system-x86_64
+
+
+
+
+
+
+
+
+
+
+
+
+
+
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 78454ac..e49186c 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -853,6 +853,7 @@ mymain(void)
QEMU_CAPS_DEVICE_ISA_SERIAL,
QEMU_CAPS_SGA);
DO_TEST("bios-nvram", NONE);
+DO_TEST("bios-nvram-network", NONE);
DO_TEST("bios-nvram-secure",
QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
QEMU_CAPS_DEVICE_PCI_BRIDGE,
--
1.8.1.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [ v3 0/4] Introduce network-backed loader & NVRAM.
Libvirt domain XML allows only local filepaths to specify a loader element or its matching NVRAM. Given that VMs may themselves move across hypervisor hosts, it should be possible to allocate loaders/NVRAM disks on network storage for uninterrupted access. This series extends the loader & NVRAM disk elements to be described as virStorageSource* elements, as discussed in : https://www.redhat.com/archives/libvir-list/2018-March/msg01721.html Sample XML with new annotation: References: -- v0/ Proposal: https://www.redhat.com/archives/libvir-list/2018-March/msg01721.html.v1 v1: https://www.redhat.com/archives/libvir-list/2018-April/msg02024.html v2: https://www.redhat.com/archives/libvir-list/2018-May/msg00948.html Changelog: - Changes since v2: - Consolidated patches with related data structures to avoid build breakage. - Passes make check & make syntax-check. Prerna Saxena (4): Schema: Introduce XML schema for network-backed loader and nvram elements. Loader: Add a more elaborate definition. Test: Add a test snippet to evaluate command line generation for loader/nvram specified via virStorageSource Documentation: Add a blurb for the newly added XML snippets for loader and nvram. docs/formatdomain.html.in | 36 +++- docs/schemas/domaincommon.rng | 108 +-- src/bhyve/bhyve_command.c | 6 +- src/conf/domain_conf.c | 250 +++-- src/conf/domain_conf.h | 11 +- src/qemu/qemu_cgroup.c | 13 +- src/qemu/qemu_command.c| 21 ++- src/qemu/qemu_domain.c | 31 ++- src/qemu/qemu_driver.c | 7 +- src/qemu/qemu_parse_command.c | 30 ++- src/qemu/qemu_process.c| 54 -- src/security/security_dac.c| 6 +- src/security/security_selinux.c| 6 +- src/security/virt-aa-helper.c | 14 +- src/vbox/vbox_common.c | 11 +- src/xenapi/xenapi_driver.c | 4 +- src/xenconfig/xen_sxpr.c | 19 +- src/xenconfig/xen_xm.c | 9 +- tests/qemuxml2argvdata/bios-nvram-network.args | 31 +++ tests/qemuxml2argvdata/bios-nvram-network.xml | 42 + tests/qemuxml2argvtest.c | 1 + 21 files changed, 606 insertions(+), 104 deletions(-) create mode 100644 tests/qemuxml2argvdata/bios-nvram-network.args create mode 100644 tests/qemuxml2argvdata/bios-nvram-network.xml -- 1.8.1.2 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [ v3 1/4] Schema: Introduce XML schema for network-backed loader and nvram elements.
Today, 'loader' and 'nvram' elements are supposed to be backed by a local disk. Given that NVRAM data could be network-backed for high availability, this patch defines XML spec for serving loader & nvram disks via the network. Signed-off-by: Prerna Saxena --- docs/schemas/domaincommon.rng | 108 +++--- 1 file changed, 90 insertions(+), 18 deletions(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 71ac3d0..64f4319 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -276,7 +276,42 @@ - + + + + file + network + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + @@ -287,7 +322,40 @@ - + + + file + network + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + @@ -1499,25 +1567,29 @@ - - - - - - - - - - - - - - - - + + + + + + + + + + + + + + + + + + + + block -- 1.8.1.2 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 01/12] Schema: Introduce XML schema for network-backed loader and nvram elements.
Hi John, Thanks for the review. On Thu, May 17, 2018 at 3:45 AM, John Ferlan wrote: > $SUBJ: > > Is a bit long - goal is <= 70-ish characters > Agree, I'll fix this. > > On 05/14/2018 07:15 AM, Prerna Saxena wrote: > > Today, 'loader' and 'nvram' elements are supposed to be backed by a > local disk. > > Given that NVRAM data could be network-backed for high availability, > this patch > > defines XML spec for serving loader & nvram disks via the network. > > > > Signed-off-by: Prerna Saxena > > --- > > docs/schemas/domaincommon.rng | 108 ++ > +--- > > 1 file changed, 90 insertions(+), 18 deletions(-) > > > > First off: applying all the patches and running make w/ "check" and > "syntax-check" fails during "check" w/ qemuxml2argvtest and > qemuxml2xmltest failing. > > I had thought make rpm ran both, but later found that it did not. Have the next series ready which : (1) combines all the related patches into ones that do not break the build. (2) Pass make check and syntax-check. > Before posting patches those must pass for each patch. When I get to > patch 2, the build breaks. While one can appreciate having less to > review in each patch, it's not possible to accept or review patches > which cause a build break. Shouldn't be up to the reviewer to figure out > how to make the series work. The rule is - each patch must be separately > compileable and capable of passing check and syntax-check. If one ever > needs to git bisect to determine where a problem lies, it'd be very > awful if the build broke. > > As for this patch in particular, there are two things going on in this > patch - #1 altering the and schema and #2 extracting > out part of the diskSourceFile to be used in #1. Ironically, Thing 2 is > creating a referenced name to be included in part of Thing 1; however, > Thing 1 is essentially a cut-n-paste of the same thing. All those > elements that are cut-n-pasted are part of diskSourceNetwork, except you > didn't include VxHS in your list. > Did not include that since I was not sure if it is really useful. > > Still, a question in my mind is whether you really need to format the > file using source. If the goal is to provide the ability to access > networked storage, why provide the option to allow someone to change > their XML from: > > /usr/lib/xen/boot/hvmloader > > to > > > > > > Yes, they are equivalent, but it seems the reason for it was because you > wanted to format the former into the latter at one point in time. If you limit to network only, then perhaps your optional attribute > changes to be network='yes' which means to parse a which may > clear up some of the "odd" pieces of the grammar below. > > Just accounting for network source alone using and directly specifying absolute paths does not look like a clean design to me. When the tag can describe all storage sources, we can unify the parsing and formatting of data using the helpers for virStorageSource*. If not, redundant code needs to be maintained for treating the two types separately. Please note that I am maintaining helpers to format-as-you-read, because that seems to be a requirement. But the underlying implementation should be able to unify code treating these two formats as mere rendering choices. > > diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon. > rng > > index 0a6b29b..a6207db 100644 > > --- a/docs/schemas/domaincommon.rng > > +++ b/docs/schemas/domaincommon.rng > > @@ -276,7 +276,42 @@ > > > > > > > > - > > + > > + > > + > > + file > > + network > > + > > + > > + > > + > > + > > + > > + > > + > > This won't be equivalent to what you started with. Prior to this change > absFilePath was required, now it would be optional. > absFilePath will become optional as there is now > 1 way to specify the storage. This is an intended side effect of broaden
[libvirt] [PATCH 07/12] Bhyve: Fix command line generation to correctly pick up local loader path.
Signed-off-by: Prerna Saxena
---
src/bhyve/bhyve_command.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
index 9413ae5..2b67014 100644
--- a/src/bhyve/bhyve_command.c
+++ b/src/bhyve/bhyve_command.c
@@ -518,10 +518,12 @@ virBhyveProcessBuildBhyveCmd(virConnectPtr conn,
virCommandAddArgList(cmd, "-s", "0:0,hostbridge", NULL);
if (def->os.bootloader == NULL &&
-def->os.loader) {
+def->os.loader &&
+def->os.loader->src &&
+def->os.loader->src->type == VIR_STORAGE_TYPE_FILE) {
if ((bhyveDriverGetCaps(conn) & BHYVE_CAP_LPC_BOOTROM)) {
virCommandAddArg(cmd, "-l");
-virCommandAddArgFormat(cmd, "bootrom,%s", def->os.loader->path);
+virCommandAddArgFormat(cmd, "bootrom,%s",
def->os.loader->src->path);
add_lpc = true;
} else {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
--
1.8.1.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 09/12] Vbox: Adjust references to 'loader' and 'nvram' elements given that these are now represented by virStorageSourcePtr.
Signed-off-by: Prerna Saxena
---
src/vbox/vbox_common.c | 11 ---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index 72a24a3..60451a3 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -998,11 +998,16 @@ vboxSetBootDeviceOrder(virDomainDefPtr def, vboxDriverPtr
data,
VIR_DEBUG("def->os.initrd %s", def->os.initrd);
VIR_DEBUG("def->os.cmdline %s", def->os.cmdline);
VIR_DEBUG("def->os.root %s", def->os.root);
-if (def->os.loader) {
-VIR_DEBUG("def->os.loader->path %s", def->os.loader->path);
+if (def->os.loader &&
+def->os.loader->src &&
+def->os.loader->src->type == VIR_STORAGE_TYPE_FILE) {
+
+VIR_DEBUG("def->os.loader->src->path %s",
def->os.loader->src->path);
VIR_DEBUG("def->os.loader->readonly %d", def->os.loader->readonly);
VIR_DEBUG("def->os.loader->type %d", def->os.loader->type);
-VIR_DEBUG("def->os.loader->nvram%s", def->os.loader->nvram);
+if (def->os.loader->nvram &&
+def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE)
+VIR_DEBUG("def->os.loader->nvram->path%s",
def->os.loader->nvram->path);
}
VIR_DEBUG("def->os.bootloader %s", def->os.bootloader);
VIR_DEBUG("def->os.bootloaderArgs %s", def->os.bootloaderArgs);
--
1.8.1.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 11/12] Test: Add a test snippet to evaluate command line generation for loader/nvram specified via virStorageSource
Signed-off-by: Prerna Saxena
---
tests/qemuxml2argvdata/bios-nvram-network.args | 31 +++
tests/qemuxml2argvdata/bios-nvram-network.xml | 42 ++
tests/qemuxml2argvtest.c | 1 +
3 files changed, 74 insertions(+)
create mode 100644 tests/qemuxml2argvdata/bios-nvram-network.args
create mode 100644 tests/qemuxml2argvdata/bios-nvram-network.xml
diff --git a/tests/qemuxml2argvdata/bios-nvram-network.args
b/tests/qemuxml2argvdata/bios-nvram-network.args
new file mode 100644
index 000..3fc68ef
--- /dev/null
+++ b/tests/qemuxml2argvdata/bios-nvram-network.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-x86_64 \
+-name test-bios \
+-S \
+-machine pc,accel=tcg,usb=off,dump-guest-core=off \
+-drive file=/usr/share/OVMF/OVMF_CODE.fd,if=pflash,format=raw,unit=0,\
+readonly=on \
+-drive file=iscsi://example.org:6000/iqn.1992-01.com.example/0,\
+if=pflash,format=raw,unit=1 \
+-m 1024 \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid 362d1fc1-df7d-193e-5c18-49a71bd1da66 \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-test-bios/monitor.sock,\
+server,nowait \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-boot order=c,menu=on \
+-usb \
+-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
+-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \
+-device usb-tablet,id=input0,bus=usb.0,port=1 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git a/tests/qemuxml2argvdata/bios-nvram-network.xml
b/tests/qemuxml2argvdata/bios-nvram-network.xml
new file mode 100644
index 000..bad114c
--- /dev/null
+++ b/tests/qemuxml2argvdata/bios-nvram-network.xml
@@ -0,0 +1,42 @@
+
+ test-bios
+ 362d1fc1-df7d-193e-5c18-49a71bd1da66
+ 1048576
+ 1048576
+ 1
+
+hvm
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+ destroy
+ restart
+ restart
+
+/usr/bin/qemu-system-x86_64
+
+
+
+
+
+
+
+
+
+
+
+
+
+
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index ddf567b..7338dba 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -853,6 +853,7 @@ mymain(void)
QEMU_CAPS_DEVICE_ISA_SERIAL,
QEMU_CAPS_SGA);
DO_TEST("bios-nvram", NONE);
+DO_TEST("bios-nvram-network", NONE);
DO_TEST("bios-nvram-secure",
QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
QEMU_CAPS_DEVICE_PCI_BRIDGE,
--
1.8.1.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 05/12] Plumb the loader source into generation of QEMU command line.
Given that nvram & loader elements can now be backed by a non-local
source too, adjust all steps leading to generation of
QEMU command line.
Signed-off-by: Prerna Saxena
---
src/qemu/qemu_cgroup.c | 13 +
src/qemu/qemu_command.c | 21 -
src/qemu/qemu_domain.c | 31 +-
src/qemu/qemu_driver.c | 7 ---
src/qemu/qemu_process.c | 42 -
src/security/security_dac.c | 6 --
src/security/security_selinux.c | 6 --
7 files changed, 88 insertions(+), 38 deletions(-)
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index d88eb78..2068eb0 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -580,16 +580,21 @@ qemuSetupMemoryCgroup(virDomainObjPtr vm)
static int
qemuSetupFirmwareCgroup(virDomainObjPtr vm)
{
+virStorageSourcePtr src = NULL;
+
if (!vm->def->os.loader)
return 0;
-if (vm->def->os.loader->path &&
-qemuSetupImagePathCgroup(vm, vm->def->os.loader->path,
- vm->def->os.loader->readonly ==
VIR_TRISTATE_BOOL_YES) < 0)
+src = vm->def->os.loader->src;
+
+if (src->type == VIR_STORAGE_TYPE_FILE &&
+qemuSetupImagePathCgroup(vm, src->path,
+ src->readonly == VIR_TRISTATE_BOOL_YES) < 0)
return -1;
if (vm->def->os.loader->nvram &&
-qemuSetupImagePathCgroup(vm, vm->def->os.loader->nvram, false) < 0)
+vm->def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE &&
+qemuSetupImagePathCgroup(vm, vm->def->os.loader->nvram->path, false) <
0)
return -1;
return 0;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 08f67a4..e9d6e4b 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9260,6 +9260,7 @@ qemuBuildDomainLoaderCommandLine(virCommandPtr cmd,
virDomainLoaderDefPtr loader = def->os.loader;
virBuffer buf = VIR_BUFFER_INITIALIZER;
int unit = 0;
+char *source = NULL;
if (!loader)
return;
@@ -9267,7 +9268,7 @@ qemuBuildDomainLoaderCommandLine(virCommandPtr cmd,
switch ((virDomainLoader) loader->type) {
case VIR_DOMAIN_LOADER_TYPE_ROM:
virCommandAddArg(cmd, "-bios");
-virCommandAddArg(cmd, loader->path);
+virCommandAddArg(cmd, loader->src->path);
break;
case VIR_DOMAIN_LOADER_TYPE_PFLASH:
@@ -9279,9 +9280,14 @@ qemuBuildDomainLoaderCommandLine(virCommandPtr cmd,
NULL);
}
+if (qemuGetDriveSourceString(loader->src, NULL, &source) < 0)
+break;
+
virBufferAddLit(&buf, "file=");
-virQEMUBuildBufferEscapeComma(&buf, loader->path);
-virBufferAsprintf(&buf, ",if=pflash,format=raw,unit=%d", unit);
+virQEMUBuildBufferEscapeComma(&buf, source);
+free(source);
+virBufferAsprintf(&buf, ",if=pflash,format=raw,unit=%d",
+ unit);
unit++;
if (loader->readonly) {
@@ -9294,9 +9300,14 @@ qemuBuildDomainLoaderCommandLine(virCommandPtr cmd,
if (loader->nvram) {
virBufferFreeAndReset(&buf);
+if (qemuGetDriveSourceString(loader->nvram, NULL, &source) < 0)
+break;
+
virBufferAddLit(&buf, "file=");
-virQEMUBuildBufferEscapeComma(&buf, loader->nvram);
-virBufferAsprintf(&buf, ",if=pflash,format=raw,unit=%d", unit);
+virQEMUBuildBufferEscapeComma(&buf, source);
+virBufferAsprintf(&buf, ",if=pflash,format=raw,unit=%d",
+ unit);
+unit++;
virCommandAddArg(cmd, "-drive");
virCommandAddArgBuffer(cmd, &buf);
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 9bb6d8a..2d4e299 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3318,6 +3318,9 @@ qemuDomainDefPostParse(virDomainDefPtr def,
* function shall not fail in that case. It will be re-run on VM startup
* with the capabilities populated. */
virQEMUCapsPtr qemuCaps = parseOpaque;
+virDomainLoaderDefPtr ldr = NULL;
+char *nvramPath = NULL;
+
int ret = -1;
if (def->os.bootloader || def->os.bootloaderArgs) {
@@ -3332,13 +3335,20 @@ qemuDomainDefPostParse(virDomainDefPtr def,
goto cleanup;
}
-if (def->os.loader &&
-def->os.loader->type == VIR_DOMAIN_LOADER_TYPE_PFLASH &&
-def->os.loader->readonly == VIR_TRISTATE_SWITCH_ON &&
-!def->
[libvirt] [PATCH 06/12] Fix the domain def inference logic to correctly account for network-backed pflash devices.
Signed-off-by: Prerna Saxena
---
src/qemu/qemu_parse_command.c | 30 +++---
1 file changed, 27 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_parse_command.c b/src/qemu/qemu_parse_command.c
index 351425f..9b1a86e 100644
--- a/src/qemu/qemu_parse_command.c
+++ b/src/qemu/qemu_parse_command.c
@@ -650,6 +650,7 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt,
int idx = -1;
int busid = -1;
int unitid = -1;
+bool is_firmware = false;
if (qemuParseKeywords(val,
&keywords,
@@ -772,6 +773,9 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt,
def->bus = VIR_DOMAIN_DISK_BUS_VIRTIO;
} else if (STREQ(values[i], "xen")) {
def->bus = VIR_DOMAIN_DISK_BUS_XEN;
+} else if (STREQ(values[i], "pflash")) {
+def->bus = VIR_DOMAIN_DISK_BUS_LAST;
+is_firmware = true;
} else if (STREQ(values[i], "sd")) {
def->bus = VIR_DOMAIN_DISK_BUS_SD;
}
@@ -943,8 +947,25 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt,
ignore_value(VIR_STRDUP(def->dst, "hda"));
}
-if (!def->dst)
-goto error;
+if (!def->dst) {
+if (is_firmware && def->bus == VIR_DOMAIN_DISK_BUS_LAST) {
+if (!dom->os.loader && (VIR_ALLOC(dom->os.loader) < 0))
+goto error;
+if (def->src->readonly) {
+/* Loader spec */
+dom->os.loader->src = def->src;
+dom->os.loader->type = VIR_DOMAIN_LOADER_TYPE_PFLASH;
+} else {
+/* NVRAM Spec */
+if (!dom->os.loader->nvram &&
(VIR_ALLOC(dom->os.loader->nvram) < 0))
+goto error;
+dom->os.loader->nvram = def->src;
+}
+} else {
+goto error;
+}
+}
+
if (STREQ(def->dst, "xvda"))
def->dst[3] = 'a' + idx;
else
@@ -2215,8 +2236,11 @@ qemuParseCommandLine(virCapsPtr caps,
} else if (STREQ(arg, "-bios")) {
WANT_VALUE();
if (VIR_ALLOC(def->os.loader) < 0 ||
-VIR_STRDUP(def->os.loader->path, val) < 0)
+VIR_ALLOC(def->os.loader->src) < 0 ||
+VIR_STRDUP(def->os.loader->src->path, val) < 0)
goto error;
+def->os.loader->src->type = VIR_STORAGE_TYPE_FILE;
+def->os.loader->type = VIR_DOMAIN_LOADER_TYPE_ROM;
} else if (STREQ(arg, "-initrd")) {
WANT_VALUE();
if (VIR_STRDUP(def->os.initrd, val) < 0)
--
1.8.1.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 04/12] Format the loader source appropriately.
If the initial XML used the old-style declaration as follows:
/path/to/file
we format it as was read.
However, if it used new-style declaration:
The formatter identifies that this is a new-style format
and renders it likewise.
Signed-off-by: Prerna Saxena
---
src/conf/domain_conf.c | 86 --
1 file changed, 76 insertions(+), 10 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index be43695..d59a579 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -18094,7 +18094,7 @@ virDomainLoaderNvramDefParseXML(xmlNodePtr node,
}
loader->nvram->type = VIR_STORAGE_TYPE_LAST;
-loader->nvram->oldStyleNvram = false;
+loader->oldStyleNvram = false;
if ((tmp = virXMLPropString(node, "backing")) &&
(loader->nvram->type = virStorageTypeFromString(tmp)) <= 0) {
@@ -26212,11 +26212,19 @@ virDomainHugepagesFormat(virBufferPtr buf,
static void
virDomainLoaderDefFormat(virBufferPtr buf,
- virDomainLoaderDefPtr loader)
+ virDomainLoaderDefPtr loader,
+ unsigned int flags)
{
const char *readonly = virTristateBoolTypeToString(loader->readonly);
const char *secure = virTristateBoolTypeToString(loader->secure);
const char *type = virDomainLoaderTypeToString(loader->type);
+const char *backing = NULL;
+
+virBuffer attrBuf = VIR_BUFFER_INITIALIZER;
+virBuffer childBuf = VIR_BUFFER_INITIALIZER;
+
+virBufferSetChildIndent(&childBuf, buf);
+
virBufferAddLit(buf, "secure)
virBufferAsprintf(buf, " secure='%s'", secure);
-virBufferAsprintf(buf, " type='%s'>", type);
+virBufferAsprintf(buf, " type='%s'", type);
+if (loader->src &&
+loader->src->type < VIR_STORAGE_TYPE_LAST) {
+if (!loader->oldStyleLoader) {
+/* Format this in the new style, using the
+ * sub-element */
+if (virDomainStorageSourceFormat(&attrBuf, &childBuf, loader->src,
+ flags, 0) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Cannot format loader source"));
+goto cleanup;
+}
+
+backing = virStorageTypeToString(loader->src->type);
+virBufferAsprintf(buf, " backing='%s'>", backing);
+
+if (virXMLFormatElement(buf, "source", &attrBuf, &childBuf) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Cannot format loader source"));
+goto cleanup;
+}
+
+} else
+/* Format this in the old-style, using absolute paths directly. */
+virBufferAsprintf(buf, ">%s", loader->src->path);
+} else
+virBufferAddLit(buf, ">\n");
+
+virBufferAddLit(buf, "\n");
-virBufferEscapeString(buf, "%s\n", loader->path);
if (loader->nvram || loader->templt) {
+ignore_value(virBufferContentAndReset(&attrBuf));
+ignore_value(virBufferContentAndReset(&childBuf));
+virBufferSetChildIndent(&childBuf, buf);
+
virBufferAddLit(buf, "templt);
-if (loader->nvram)
-virBufferEscapeString(buf, ">%s\n", loader->nvram);
-else
-virBufferAddLit(buf, "/>\n");
+
+if (loader->templt)
+virBufferEscapeString(buf, " template='%s'", loader->templt);
+
+if (loader->nvram) {
+backing = virStorageTypeToString(loader->nvram->type);
+if (!loader->oldStyleNvram) {
+if (virDomainStorageSourceFormat(&attrBuf, &childBuf,
+ loader->nvram, flags, 0) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Cannot format NVRAM source"));
+virBufferAddLit(buf, ">\n\n");
+goto cleanup;
+}
+
+virBufferEscapeString(buf, " backing='%s'>", backing);
+if (virXMLFormatElement(buf, "source", &attrBuf, &childBuf) <
0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Cannot format NVRAM source"));
+virBufferAddLit(buf, "\n");
+goto cleanup;
+}
+} else {
+/* old-style NVRAM declaration found
[libvirt] [PATCH 08/12] virt-aa-helper: Adjust references to loader & nvram elements to correctly parse the virStorageSource types.
Signed-off-by: Prerna Saxena
---
src/security/virt-aa-helper.c | 14 ++
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index d0f9876..8217d67 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -1063,12 +1063,18 @@ get_files(vahControl * ctl)
if (vah_add_file(&buf, ctl->def->os.slic_table, "r") != 0)
goto cleanup;
-if (ctl->def->os.loader && ctl->def->os.loader->path)
-if (vah_add_file(&buf, ctl->def->os.loader->path, "rk") != 0)
+if (ctl->def->os.loader &&
+ctl->def->os.loader->src &&
+ctl->def->os.loader->src->type == VIR_STORAGE_TYPE_FILE &&
+ctl->def->os.loader->src->path)
+if (vah_add_file(&buf, ctl->def->os.loader->src->path, "rk") != 0)
goto cleanup;
-if (ctl->def->os.loader && ctl->def->os.loader->nvram)
-if (vah_add_file(&buf, ctl->def->os.loader->nvram, "rwk") != 0)
+if (ctl->def->os.loader &&
+ctl->def->os.loader->nvram &&
+ctl->def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE &&
+ctl->def->os.loader->nvram->path)
+if (vah_add_file(&buf, ctl->def->os.loader->nvram->path, "rwk") != 0)
goto cleanup;
for (i = 0; i < ctl->def->ngraphics; i++) {
--
1.8.1.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 01/12] Schema: Introduce XML schema for network-backed loader and nvram elements.
Today, 'loader' and 'nvram' elements are supposed to be backed by a local disk. Given that NVRAM data could be network-backed for high availability, this patch defines XML spec for serving loader & nvram disks via the network. Signed-off-by: Prerna Saxena --- docs/schemas/domaincommon.rng | 108 +++--- 1 file changed, 90 insertions(+), 18 deletions(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 0a6b29b..a6207db 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -276,7 +276,42 @@ - + + + + file + network + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + @@ -287,7 +322,40 @@ - + + + file + network + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + @@ -1494,25 +1562,29 @@ - - - - - - - - - - - - - - - - + + + + + + + + + + + + + + + + + + + + block -- 1.8.1.2 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 10/12] Xen: Adjust all references to loader & nvram elements given that they are now backed by virStorageSourcePtr
Signed-off-by: Prerna Saxena
---
src/xenapi/xenapi_driver.c | 4 +++-
src/xenconfig/xen_sxpr.c | 19 +++
src/xenconfig/xen_xm.c | 9 ++---
3 files changed, 20 insertions(+), 12 deletions(-)
diff --git a/src/xenapi/xenapi_driver.c b/src/xenapi/xenapi_driver.c
index 42b305d..4070660 100644
--- a/src/xenapi/xenapi_driver.c
+++ b/src/xenapi/xenapi_driver.c
@@ -1444,10 +1444,12 @@ xenapiDomainGetXMLDesc(virDomainPtr dom, unsigned int
flags)
char *value = NULL;
defPtr->os.type = VIR_DOMAIN_OSTYPE_XEN;
if (VIR_ALLOC(defPtr->os.loader) < 0 ||
-VIR_STRDUP(defPtr->os.loader->path, "pygrub") < 0) {
+VIR_ALLOC(defPtr->os.loader->src) < 0 ||
+VIR_STRDUP(defPtr->os.loader->src->path, "pygrub") < 0) {
VIR_FREE(boot_policy);
goto error;
}
+defPtr->os.loader->src->type = VIR_STORAGE_TYPE_FILE;
xen_vm_get_pv_kernel(session, &value, vm);
if (STRNEQ(value, "")) {
if (VIR_STRDUP(defPtr->os.kernel, value) < 0) {
diff --git a/src/xenconfig/xen_sxpr.c b/src/xenconfig/xen_sxpr.c
index e868c05..fd3165c 100644
--- a/src/xenconfig/xen_sxpr.c
+++ b/src/xenconfig/xen_sxpr.c
@@ -87,15 +87,17 @@ xenParseSxprOS(const struct sexpr *node,
int hvm)
{
if (hvm) {
-if (VIR_ALLOC(def->os.loader) < 0)
+if ((VIR_ALLOC(def->os.loader) < 0) ||
+(VIR_ALLOC(def->os.loader->src) < 0))
goto error;
-if (sexpr_node_copy(node, "domain/image/hvm/loader",
&def->os.loader->path) < 0)
+def->os.loader->src->type = VIR_STORAGE_TYPE_FILE;
+if (sexpr_node_copy(node, "domain/image/hvm/loader",
&def->os.loader->src->path) < 0)
goto error;
-if (def->os.loader->path == NULL) {
-if (sexpr_node_copy(node, "domain/image/hvm/kernel",
&def->os.loader->path) < 0)
+if (def->os.loader->src->path == NULL) {
+if (sexpr_node_copy(node, "domain/image/hvm/kernel",
&def->os.loader->src->path) < 0)
goto error;
-if (def->os.loader->path == NULL) {
+if (def->os.loader->src->path == NULL) {
virReportError(VIR_ERR_INTERNAL_ERROR,
"%s", _("domain information incomplete, missing
HVM loader"));
return -1;
@@ -124,7 +126,8 @@ xenParseSxprOS(const struct sexpr *node,
/* If HVM kenrel == loader, then old xend, so kill off kernel */
if (hvm &&
def->os.kernel &&
-STREQ(def->os.kernel, def->os.loader->path)) {
+def->os.loader->src &&
+STREQ(def->os.kernel, def->os.loader->src->path)) {
VIR_FREE(def->os.kernel);
}
/* Drop kernel argument that has no value */
@@ -2259,9 +2262,9 @@ xenFormatSxpr(virConnectPtr conn, virDomainDefPtr def)
if (hvm) {
char bootorder[VIR_DOMAIN_BOOT_LAST+1];
if (def->os.kernel)
-virBufferEscapeSexpr(&buf, "(loader '%s')",
def->os.loader->path);
+virBufferEscapeSexpr(&buf, "(loader '%s')",
def->os.loader->src->path);
else
-virBufferEscapeSexpr(&buf, "(kernel '%s')",
def->os.loader->path);
+virBufferEscapeSexpr(&buf, "(kernel '%s')",
def->os.loader->src->path);
virBufferAsprintf(&buf, "(vcpus %u)",
virDomainDefGetVcpusMax(def));
if (virDomainDefHasVcpusOffline(def))
diff --git a/src/xenconfig/xen_xm.c b/src/xenconfig/xen_xm.c
index 8ef68bb..c6a1f03 100644
--- a/src/xenconfig/xen_xm.c
+++ b/src/xenconfig/xen_xm.c
@@ -47,8 +47,10 @@ xenParseXMOS(virConfPtr conf, virDomainDefPtr def)
const char *boot;
if (VIR_ALLOC(def->os.loader) < 0 ||
-xenConfigCopyString(conf, "kernel", &def->os.loader->path) < 0)
+VIR_ALLOC(def->os.loader->src) < 0 ||
+xenConfigCopyString(conf, "kernel", &def->os.loader->src->path) <
0)
return -1;
+def->os.loader->src->type = VIR_STORAGE_TYPE_FILE;
if (xenConfigGetString(conf, "boot", &boot, "c") < 0)
return -1;
@@ -481,9 +483,10 @@ xenFormatXMOS(virConfPtr conf, virDomainDefPtr def)
if (xenConfigSetString(conf, "builder", "hvm") < 0)
return -1;
-if (def->os.loader && def->os.loader->path &&
-xenConfigSetString(conf, "kernel", def->os.loader->path) < 0)
+if (def->os.loader && def->os.loader->src &&
+xenConfigSetString(conf, "kernel", def->os.loader->src->path) < 0)
return -1;
+def->os.loader->src->type = VIR_STORAGE_TYPE_FILE;
for (i = 0; i < def->os.nBootDevs; i++) {
switch (def->os.bootDevs[i]) {
--
1.8.1.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 03/12] Parse domain XML to generate virDomainLoaderDef & virDomainNvramDef.
This patch is used to interpret domain XML and store the Loader &
Nvram's backing definitions as virStorageSource.
It also identifies if input XML used old or new-style declaration.
(This will later be used by the formatter).
Signed-off-by: Prerna Saxena
---
src/conf/domain_conf.c | 131 ++---
1 file changed, 124 insertions(+), 7 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index f678e26..be43695 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2884,8 +2884,8 @@ virDomainLoaderDefFree(virDomainLoaderDefPtr loader)
if (!loader)
return;
-VIR_FREE(loader->path);
-VIR_FREE(loader->nvram);
+virStorageSourceFree(loader->src);
+virStorageSourceFree(loader->nvram);
VIR_FREE(loader->templt);
VIR_FREE(loader);
}
@@ -17986,17 +17986,62 @@
virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def)
static int
virDomainLoaderDefParseXML(xmlNodePtr node,
+ xmlXPathContextPtr ctxt,
virDomainLoaderDefPtr loader)
{
int ret = -1;
char *readonly_str = NULL;
char *secure_str = NULL;
char *type_str = NULL;
+char *tmp = NULL;
+xmlNodePtr cur;
+xmlXPathContextPtr cur_ctxt = ctxt;
+
+if (VIR_ALLOC(loader->src)) {
+goto cleanup;
+}
+loader->src->type = VIR_STORAGE_TYPE_LAST;
+loader->oldStyleLoader = false;
readonly_str = virXMLPropString(node, "readonly");
secure_str = virXMLPropString(node, "secure");
type_str = virXMLPropString(node, "type");
-loader->path = (char *) xmlNodeGetContent(node);
+
+if ((tmp = virXMLPropString(node, "backing")) &&
+(loader->src->type = virStorageTypeFromString(tmp)) <= 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unknown loader type '%s'"), tmp);
+goto cleanup;
+}
+VIR_FREE(tmp);
+
+for (cur = node->children; cur != NULL; cur = cur->next) {
+if (cur->type != XML_ELEMENT_NODE) {
+continue;
+}
+
+if (virXMLNodeNameEqual(cur, "source")) {
+/* new-style declaration found */
+if (virDomainStorageSourceParse(cur, cur_ctxt, loader->src, 0) <
0) {
+virReportError(VIR_ERR_XML_DETAIL,
+ _("Error parsing Loader source element"));
+goto cleanup;
+}
+break;
+}
+}
+
+/* Old-style absolute path found ? */
+if (loader->src->type == VIR_STORAGE_TYPE_LAST) {
+if (!(loader->src->path = (char *) xmlNodeGetContent(node))) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("missing loader source"));
+goto cleanup;
+} else {
+loader->src->type = VIR_STORAGE_TYPE_FILE;
+loader->oldStyleLoader = true;
+}
+}
if (readonly_str &&
(loader->readonly = virTristateBoolTypeFromString(readonly_str)) <= 0)
{
@@ -18023,13 +18068,81 @@ virDomainLoaderDefParseXML(xmlNodePtr node,
}
ret = 0;
- cleanup:
+goto exit;
+cleanup:
+if (loader->src)
+ VIR_FREE(loader->src);
+exit:
VIR_FREE(readonly_str);
VIR_FREE(secure_str);
VIR_FREE(type_str);
+
return ret;
}
+static int
+virDomainLoaderNvramDefParseXML(xmlNodePtr node,
+ xmlXPathContextPtr ctxt,
+ virDomainLoaderDefPtr loader)
+{
+int ret = -1;
+char *tmp = NULL;
+xmlNodePtr cur;
+
+if (VIR_ALLOC(loader->nvram)) {
+goto cleanup;
+}
+
+loader->nvram->type = VIR_STORAGE_TYPE_LAST;
+loader->nvram->oldStyleNvram = false;
+
+if ((tmp = virXMLPropString(node, "backing")) &&
+(loader->nvram->type = virStorageTypeFromString(tmp)) <= 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unknown nvram type '%s'"), tmp);
+goto cleanup;
+}
+VIR_FREE(tmp);
+
+for (cur = node->children; cur != NULL; cur = cur->next) {
+if (cur->type != XML_ELEMENT_NODE) {
+continue;
+}
+
+if (virXMLNodeNameEqual(cur, "template")) {
+loader->templt = virXPathString("string(./os/nvram[1]/@template)",
ctxt);
+continue;
+}
+
+if (virXMLNodeNameEqual(cur, "source")) {
+if (virDomainStorageSourceParse(cur, ctxt, loader->nvram, 0) < 0) {
+virReportError(VIR_ERR_XML_DETAIL,
+ _("Error parsing nvram source element"));
+goto cleanup;
[libvirt] [PATCH 12/12] Documentation: Add a blurb for the newly added XML snippets for loader and nvram.
Signed-off-by: Prerna Saxena --- docs/formatdomain.html.in | 36 +--- 1 file changed, 33 insertions(+), 3 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index caeb14e..b8cb7ac 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -112,6 +112,20 @@ </os> ... +Where loader/NVRAM can also be described as: + + +... + <loader readonly='yes' secure='no' type='rom' backing='file'> +<source file='/usr/share/OVMF/OVMF_CODE.fd'/> + </loader> + <nvram backing='network' template='/usr/share/OVMF/OVMF_VARS.fd'> +<source protocol='iscsi' name='iqn.2013-07.com.example:iscsi-nopool/2'> + <host name='example.com' port='3260'/> +</source> + </nvram> +... + type The content of the type element specifies the @@ -143,7 +157,15 @@ pflash. Moreover, some firmwares may implement the Secure boot feature. Attribute secure can be used then to control it. -Since 2.1.0 +Since 2.1.0Since 4.5.0 +Loader element can also be specified as a remote disk. The attribute +backing (accepted values are +file and network)can be used to represent +local absolute paths as well as network-backed disks. The details of +backing file may be specified as storage source +Allowed backing type file replaces the old +specification and extends to all hypervisors that supported it, while +type network is only supported by QEMU. nvram Some UEFI firmwares may want to use a non-volatile memory to store some variables. In the host, this is represented as a file and the @@ -154,7 +176,15 @@ from the config file. Note, that for transient domains if the NVRAM file has been created by libvirt it is left behind and it is management application's responsibility to save and remove file (if needed to be -persistent). Since 1.2.8 +persistent). Since 1.2.8 +Since 4.5.0, attribute +backing(accepted values file +and network)can be used to specify a +storage source +of type file or network. The value filedescribes +absolute NVRAM paths and extends the present specification +for all hypervisors that support this, while +network applies only to QEMU. boot The dev attribute takes one of the values "fd", "hd", "cdrom" or "network" and is used to specify the next boot device @@ -2707,7 +2737,7 @@ - source + Disk source Representation of the disk source depends on the disk type attribute value as follows: -- 1.8.1.2 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 00/12][v2] Introduce network-backed loader & nvram.
Libvirt domain XML today only allows local filepaths that can be used to specify a loader element or its matching NVRAM disk. Given that Vms may themselves move across hypervisor hosts, it should be possible to allocate loaders/NVRAM disks on network storage for uninterrupted access. This series extends the firmware loader & NVRAM disks to be hosted over network-backed disks, for high availability. It achieves this by embedding virStorageSource elements for loader & nvram into _virDomainLoaderDef, as discussed in https://www.redhat.com/archives/libvir-list/2018-March/msg01721.html. Currently, the source type is annotated by introducing a new attribute "backing" for both 'loader' and 'nvram' elements. Hence, a sample XML with new annotation looks like this: References: -- v0 / Proposal: https://www.redhat.com/archives/libvir-list/2018-March/msg01721.html. v1 : https://www.redhat.com/archives/libvir-list/2018-April/msg02024.html Changelog: - Changes since v1: 1. Split up the patch into smaller units. 2. Added doc snippet & tests. 3. Changed the formatting code in patch 4 to format a domain's XML in the style that was read. I found that encryption seems to be a property of the storage volume. I didnt include that in this series since it does not use backing type as volume. Will include that later once the basic network support patches get done. Looking forward to feedback, Prerna Prerna Saxena (12): Schema: Introduce XML schema for network-backed loader and nvram elements. Loader: Add a more elaborate definition. Parse domain XML to generate virDomainLoaderDef & virDomainNvramDef. Format the loader source appropriately. Plumb the loader source into generation of QEMU command line. Fix the domain def inference logic to correctly account for network-backed pflash devices. Bhyve: Fix command line generation to correctly pick up local loader path. virt-aa-helper: Adjust references to loader & nvram elements to correctly parse the virStorageSource types. Vbox: Adjust references to 'loader' and 'nvram' elements given that these are now represented by virStorageSourcePtr. Xen: Adjust all references to loader & nvram elements given that they are now backed by virStorageSourcePtr Test: Add a test snippet to evaluate command line generation for loader/nvram specified via virStorageSource Documentation: Add a blurb for the newly added XML snippets for loader and nvram. docs/formatdomain.html.in | 36 - docs/schemas/domaincommon.rng | 108 ++--- src/bhyve/bhyve_command.c | 6 +- src/conf/domain_conf.c | 215 +++-- src/conf/domain_conf.h | 11 +- src/qemu/qemu_cgroup.c | 13 +- src/qemu/qemu_command.c| 21 ++- src/qemu/qemu_domain.c | 31 ++-- src/qemu/qemu_driver.c | 7 +- src/qemu/qemu_parse_command.c | 30 +++- src/qemu/qemu_process.c| 42 +++-- src/security/security_dac.c| 6 +- src/security/security_selinux.c| 6 +- src/security/virt-aa-helper.c | 14 +- src/vbox/vbox_common.c | 11 +- src/xenapi/xenapi_driver.c | 4 +- src/xenconfig/xen_sxpr.c | 19 ++- src/xenconfig/xen_xm.c | 9 +- tests/qemuxml2argvdata/bios-nvram-network.args | 31 tests/qemuxml2argvdata/bios-nvram-network.xml | 42 + tests/qemuxml2argvtest.c | 1 + 21 files changed, 562 insertions(+), 101 deletions(-) create mode 100644 tests/qemuxml2argvdata/bios-nvram-network.args create mode 100644 tests/qemuxml2argvdata/bios-nvram-network.xml -- 1.8.1.2 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 02/12] Loader: Add a more elaborate definition.
Augment definition to include virStorageSourcePtr that
more comprehensively describes the nature of backing element.
Also include flags for annotating if input XML definition is
old-style or new-style.
Signed-off-by: Prerna Saxena
---
src/conf/domain_conf.h | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 15d228b..bbd021b 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1855,15 +1855,22 @@ typedef enum {
VIR_ENUM_DECL(virDomainLoader)
+struct _virStorageSource;
+typedef struct _virStorageSource* virStorageSourcePtr;
+
typedef struct _virDomainLoaderDef virDomainLoaderDef;
typedef virDomainLoaderDef *virDomainLoaderDefPtr;
struct _virDomainLoaderDef {
-char *path;
+virStorageSourcePtr src;
int readonly; /* enum virTristateBool */
virDomainLoader type;
int secure; /* enum virTristateBool */
-char *nvram;/* path to non-volatile RAM */
+virStorageSourcePtr nvram; /* path to non-voliatile RAM */
char *templt; /* user override of path to master nvram */
+bool oldStyleLoader; /* is this an old-style XML formatting,
+ * ie, absolute path is directly specified? */
+bool oldStyleNvram; /* is this an old-style XML formatting,
+ * ie, absolute path is directly specified? */
};
void virDomainLoaderDefFree(virDomainLoaderDefPtr loader);
--
1.8.1.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Qemu driver: Support network-backed pflash disks.
On Sat, Apr 28, 2018 at 1:11 AM, John Ferlan wrote: > > > On 04/20/2018 04:59 AM, Prerna Saxena wrote: > > So far libvirt domain XML only allows local filepaths that can be > > used to specify a loader element or its matching NVRAM disk. > > Given that Vms may themselves move across hypervisor hosts, it should be > > possible to allocate loaders/NVRAM disks on network storage for > > uninterrupted access. > > > > Signed-off-by: Prerna Saxena > > --- > > docs/schemas/domaincommon.rng | 108 +++ > > src/conf/domain_conf.c | 188 ++ > ++ > > src/conf/domain_conf.h | 7 +- > > src/qemu/qemu_cgroup.c | 13 ++- > > src/qemu/qemu_command.c | 21 +++-- > > src/qemu/qemu_domain.c | 16 ++-- > > src/qemu/qemu_driver.c | 7 +- > > src/qemu/qemu_parse_command.c | 30 ++- > > src/qemu/qemu_process.c | 33 --- > > src/security/security_dac.c | 6 +- > > src/security/security_selinux.c | 6 +- > > 11 files changed, 361 insertions(+), 74 deletions(-) > > > > I know on IRC you asked Peter or Michal to review (and they're CC'd > here), but before they get a chance next week some time - I'll give you > Thank you for taking a look. > a quick look. You do understand Peter, Michal, myself, and other Red Hat > libvirt developers follow libvir-list anyway - so CC'ing any of us > doesn't do much since we filter into a mail folder anyway... > > I understand. Peter & Michal had participated in the original IRC discussion around the need for extending loader as a network backed device, and I had cc'ed them for context. > This will need a v2 anyway because the patch has too much going on and > needs to be split up more. > Will do. I should have properly mentioned this was an RFC rather than a ready-to-merge patch, and that this currently excluded test and documentation fixes. > > You need to break out the domain_conf, docs, etc. changes into one > patch. Much of what you put into the cover that describes the XML should Got it. > go into this patch's commit message. There should be xml2xml test > changes as well as adjustments to formatdomain.html.in to describe the > new options. The part of the cover that says automatically reformatting > to use the storage source cannot happen. There's precedence for that when the and moved *into* the storage source we > still have to accommodate for them outside. Internally, it can be placed > as expected, but when formatting, we have to format as we read. After > Ok. I had explicitly asked around on IRC if it was okay "breaking" the existing XML semantics. Peter had mentioned it was okay to have the XML read as its old semantic. The formatter could later "translate" the old -style absolute filepaths into the "new-style" source-description that is introduced. I had kept that in mind while implementing this patch. If that is not to be done, I will need to redo parts of this patch. > that, perhaps add the security changes in another, the cgroup in > another, and finally the qemu adjustments in the last. Not even sure if > you need a capability as well. > > Why do you think we'd need a capability for this? I'd be keen to understand why changes to XML spec is not enough. > Finally this doesn't even compile for me. You removed @path from > _virDomainLoaderDef but neglected to adjust all consumers. I suggest > using cscope and egrep'ing on "os.*loader->path" as well as ->nvram > since you changed it's type. > I know why. I had run and tested this to work, but my build configuration included the qemu driver and excluded every other driver. Given that this element extends to other drivers, I can understand the build issues. Again, should have mentioned this was an RFC. > > I assume you've considered that network storage types need to deal with > authentication and encryption passphrases, right? What about using a > srcpool storage source? > > Erm, no. This patch does not include support for encryption/authenticaton. I will need to add those. > I'll briefly scan the rest. > > > diff --git a/docs/schema
[libvirt] [PATCH] Qemu driver: Support network-backed pflash disks.
So far libvirt domain XML only allows local filepaths that can be
used to specify a loader element or its matching NVRAM disk.
Given that Vms may themselves move across hypervisor hosts, it should be
possible to allocate loaders/NVRAM disks on network storage for
uninterrupted access.
Signed-off-by: Prerna Saxena
---
docs/schemas/domaincommon.rng | 108 +++
src/conf/domain_conf.c | 188
src/conf/domain_conf.h | 7 +-
src/qemu/qemu_cgroup.c | 13 ++-
src/qemu/qemu_command.c | 21 +++--
src/qemu/qemu_domain.c | 16 ++--
src/qemu/qemu_driver.c | 7 +-
src/qemu/qemu_parse_command.c | 30 ++-
src/qemu/qemu_process.c | 33 ---
src/security/security_dac.c | 6 +-
src/security/security_selinux.c | 6 +-
11 files changed, 361 insertions(+), 74 deletions(-)
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 4cab55f..32db395 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -276,7 +276,42 @@
-
+
+
+
+ file
+ network
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
@@ -287,7 +322,40 @@
-
+
+
+ file
+ network
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
@@ -1494,25 +1562,29 @@
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
block
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 35666c1..c80f9d9 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2883,8 +2883,8 @@ virDomainLoaderDefFree(virDomainLoaderDefPtr loader)
if (!loader)
return;
-VIR_FREE(loader->path);
-VIR_FREE(loader->nvram);
+virStorageSourceFree(loader->loader_src);
+virStorageSourceFree(loader->nvram);
VIR_FREE(loader->templt);
VIR_FREE(loader);
}
@@ -17961,17 +17961,59 @@
virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def)
static int
virDomainLoaderDefParseXML(xmlNodePtr node,
+ xmlXPathContextPtr ctxt,
virDomainLoaderDefPtr loader)
{
int ret = -1;
char *readonly_str = NULL;
char *secure_str = NULL;
char *type_str = NULL;
+char *tmp = NULL;
+xmlNodePtr cur;
+xmlXPathContextPtr cur_ctxt = ctxt;
+
+if (VIR_ALLOC(loader->loader_src)) {
+goto cleanup;
+}
+loader->loader_src->type = VIR_STORAGE_TYPE_LAST;
readonly_str = virXMLPropString(node, "readonly");
secure_str = virXMLPropString(node, "secure");
type_str = virXMLPropString(node, "type");
-loader->path = (char *) xmlNodeGetContent(node);
+
+if ((tmp = virXMLPropString(node, "backing")) &&
+(loader->loader_src->type = virStorageTypeFromString(tmp)) <= 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unknown loader type '%s'"), tmp);
+goto cleanup;
+}
+VIR_FREE(tmp);
+
+for (cur = node->children; cur != NULL; cur = cur->next) {
+if (cur->type != XML_ELEMENT_NODE) {
+continue;
+}
+
+if (virXMLNodeNameEqual(cur, "source")) {
+if (virDomainStorageSourceParse(cur, cur_ctxt, loader->load
[libvirt] [PATCH] Qemu driver: Support network-backed pflash disks.
This implements support for firmware loader & NVRAM disks over network-backed disks. As discussed in https://www.redhat.com/archives/libvir-list/2018-March/msg01721.html, the patch embeds the spec for disks in and elements as well. Currently, the source type is annotated by introducing a new attribute "backing" for both 'loader' and 'nvram' elements. Hence, a sample XML with new annotation looks like this: The patche automatically re-formats any older-stype declaration into this new style. Templates can be used to create a new NVRAM only if the nvram backing = 'file'. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] RFC: Extending UEFI XML specification
On Wed, Mar 28, 2018 at 4:41 PM, Peter Krempa wrote: > On Wed, Mar 28, 2018 at 16:15:50 +0530, Prerna wrote: > > Hi Michal, > > The , tags of os element in domain XML ( > > https://libvirt.org/formatdomain.html#elementsOSBIOS) currently expects > > absolute path of the local file which would be used to back the the > pflash > > disk representing the non-volatile RAM: > > > > > type='rom'>/usr/lib/xen/boot/hvmloader > > > template='/usr/share/OVMF/OVMF_VARS.fd'>/var/lib/ > libvirt/nvram/guest_VARS.fd > > > > However, given that for virtualized environments, it is possible that the > > VM could be started on different hosts at various points in time, and so > we > > need to expose the firmware/nvram tuple over a network device so as to be > > accessible from various hosts. > > I propose extending of the existing config by adding a new element, > > "backing". This could be one of : > > - 'file': for local filesystem paths > > - 'network': for network-attached storage. > > Since this is as any other storage volume for the hypervisor, you should > treat it as a virStorageSource, including the 'block' and 'volume' > types. > > > > > As an example: > > > > > 'file'>/usr/share/OVMF/OVMF_CODE.fd > > > template='/usr/share/OVMF/OVMF_VARS.fd'>/var/lib/ > libvirt/nvram/guest_VARS.fd > > > > For network-attached storage: > > > 'network'>/usr/share/OVMF/OVMF_CODE.fd > > I presume you wanted to add the section here as well. > > Agree, I'd missed it while writing up, but I intended to cover this. > > > > > > > > > > > > In addition to this we should also support the 'storage-source' like > definition for backing='file' too: > > > > > > Yes, this would be required for uniformity. But this would inadvertantly break the existing XML description for local files. So I was not sure if this would be acceptable. > With that encryption of the image can be defined as well. > > > > > Note that 'template' attribute in NVRAM should be explicitly disallowed > for > > backing type "network". This is because libvirtd may not be able to > access > > the backing store to copy the contents of the template. > > This should be hypervisor-defined. While it will not be easy, you can > use qemu-img in the qemu driver to populate network disks via the > 'convert' command. > > You probably meant qemu-nbd? But other backends such as gluster/iSCSI may not just work with that. Were you describing some other scenario ? > > I would like to capture thoughts from the community to extend the current > > firmware spec. > > > > Regards, > > Prerna > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] RFC: Extending UEFI XML specification
Hi Michal, The , tags of os element in domain XML ( https://libvirt.org/formatdomain.html#elementsOSBIOS) currently expects absolute path of the local file which would be used to back the the pflash disk representing the non-volatile RAM: /usr/lib/xen/boot/hvmloader /var/lib/libvirt/nvram/guest_VARS.fd However, given that for virtualized environments, it is possible that the VM could be started on different hosts at various points in time, and so we need to expose the firmware/nvram tuple over a network device so as to be accessible from various hosts. I propose extending of the existing config by adding a new element, "backing". This could be one of : - 'file': for local filesystem paths - 'network': for network-attached storage. As an example: /usr/share/OVMF/OVMF_CODE.fd /var/lib/libvirt/nvram/guest_VARS.fd For network-attached storage: /usr/share/OVMF/OVMF_CODE.fd Note that 'template' attribute in NVRAM should be explicitly disallowed for backing type "network". This is because libvirtd may not be able to access the backing store to copy the contents of the template. I would like to capture thoughts from the community to extend the current firmware spec. Regards, Prerna -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [[RFC] 0/8] Implement async QEMU event handling in libvirtd.
Hi Marc, Currently the block job handling needs to be sorted out before events can assume independent handling from RPC contexts. Sorry, I have not been able to revisit this in the past 2 months, but this is something I would very much like to fix. I will try looking up the exact dependency of the block layer so that this series could make some progress. regards, Prerna On Wed, Mar 21, 2018 at 1:24 PM, Marc Hartmayer wrote: > On Tue, Oct 24, 2017 at 07:34 PM +0200, Prerna Saxena < > saxenap@gmail.com> wrote: > > As noted in > > https://www.redhat.com/archives/libvir-list/2017-May/msg00016.html > > libvirt-QEMU driver handles all async events from the main loop. > > Each event handling needs the per-VM lock to make forward progress. In > > the case where an async event is received for the same VM which has an > > RPC running, the main loop is held up contending for the same lock. > > > > This impacts scalability, and should be addressed on priority. > > > > Note that libvirt does have a 2-step deferred handling for a few event > > categories, but (1) That is insufficient since blockign happens before > > the handler could disambiguate which one needs to be posted to this > > other queue. > > (2) There needs to be homogeniety. > > > > The current series builds a framework for recording and handling VM > > events. > > It initializes per-VM event queue, and a global event queue pointing to > > events from all the VMs. Event handling is staggered in 2 stages: > > - When an event is received, it is enqueued in the per-VM queue as well > > as the global queues. > > - The global queue is built into the QEMU Driver as a threadpool > > (currently with a single thread). > > - Enqueuing of a new event triggers the global event worker thread, which > > then attempts to take a lock for this event's VM. > > - If the lock is available, the event worker runs the function > handling > > this event type. Once done, it dequeues this event from the global > > as well as per-VM queues. > > - If the lock is unavailable(ie taken by RPC thread), the event > worker > > thread leaves this as-is and picks up the next event. > > - Once the RPC thread completes, it looks for events pertaining to the > > VM in the per-VM event queue. It then processes the events serially > > (holding the VM lock) until there are no more events remaining for > > this VM. At this point, the per-VM lock is relinquished. > > > > Patch Series status: > > Strictly RFC only. No compilation issues. I have not had a chance to > > (stress) test it after rebase to latest master. > > Note that documentation and test coverage is TBD, since a few open > > points remain. > > > > Known issues/ caveats: > > - RPC handling time will become non-deterministic. > > - An event will only be "notified" to a client once the RPC for same VM > completes. > > - Needs careful consideration in all cases where a QMP event is used to > > "signal" an RPC thread, else will deadlock. > > > > Will be happy to drive more discussion in the community and completely > > implement it. > > > > Prerna Saxena (8): > > Introduce virObjectTrylock() > > QEMU Event handling: Introduce async event helpers in qemu_event.[ch] > > Setup global and per-VM event queues. Also initialize per-VM queues > > when libvirt reconnects to an existing VM. > > Events: Allow monitor to "enqueue" events to a queue. Also introduce a > > framework of handlers for each event type, that can be called when > > the handler is running an event. > > Events: Plumb event handling calls before a domain's APIs complete. > > Code refactor: Move helper functions of doCoreDump*, syncNicRxFilter*, > > and qemuOpenFile* to qemu_process.[ch] > > Fold back the 2-stage event implementation for a few events : > > Watchdog, Monitor EOF, Serial changed, Guest panic, Nic RX filter > > changed .. into single level. > > Initialize the per-VM event queues in context of domain init. > > > > src/Makefile.am |1 + > > src/conf/domain_conf.h |3 + > > src/libvirt_private.syms |1 + > > src/qemu/qemu_conf.h |4 + > > src/qemu/qemu_driver.c | 1710 +++ > > src/qemu/qemu_event.c| 317 +++ > > src/qemu/qemu_event.h| 231 + > > src/qemu/qemu_monitor.c | 592 ++-- > > src/qemu/qemu_monitor.h | 80 +- > > src/qemu/qemu_monitor_jso
Re: [libvirt] [PATCH] Migration: Preserve the failed job in case migration job is terminated beyond the perform phase.
Hi Jirka,
On Thu, Jan 25, 2018 at 8:43 PM, Jiri Denemark wrote:
> On Thu, Jan 25, 2018 at 19:51:23 +0530, Prerna Saxena wrote:
> > In case of non-p2p migration, in case libvirt client gets disconnected
> from source libvirt
> > after PERFORM phase is over, the daemon just resets the current
> migration job.
> > However, the VM could be left paused on both source and destination in
> such case. In case
> > the client reconnects and queries migration status, the job has been
> blanked out from source libvirt,
> > and this reconnected client has no clear way of figuring out if an
> unclean migration had previously
> > been aborted.
>
> The virDomainGetState API should return VIR_DOMAIN_PAUSED with
> VIR_DOMAIN_PAUSED_MIGRATION reason. Is this not enough?
>
>
I understand that a client application should poll source libvirtd for
status of migration job completion using virDomainGetJobStats().
However, as you explained above, cleanup callbacks clear the job info so a
client should additionally be polling for virDomainGetState() too.
Would it not be cleaner to have a single API reflect the source of truth?
> > This patch calls out a "potentially" incomplete migration as a failed
> > job, so that a client may
>
> As you say it's potentially incomplete, so marking it as failed is not
> completely correct. It's a split brain when the source cannot
> distinguish whether the migration was successful or not.
>
Agree, it might have run to completion too, as we observed in some cases.
Do you think marking the job status as "UNKNOWN" is better articulation of
the current state?
>
> > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > index e8e0313..7c60d17 100644
> > --- a/src/qemu/qemu_domain.c
> > +++ b/src/qemu/qemu_domain.c
> > @@ -4564,6 +4564,22 @@ qemuDomainObjDiscardAsyncJob(virQEMUDriverPtr
> driver, virDomainObjPtr obj)
> > qemuDomainObjSaveJob(driver, obj);
> > }
> >
> > +
> > +void
> > +qemuDomainObjFailAsyncJob(virQEMUDriverPtr driver, virDomainObjPtr obj)
> > +{
> > +qemuDomainObjPrivatePtr priv = obj->privateData;
> > +VIR_FREE(priv->job.completed);
> > +if (VIR_ALLOC(priv->job.completed) == 0) {
> > +priv->job.current->type = VIR_DOMAIN_JOB_FAILED;
> > +priv->job.completed = priv->job.current;
>
> This will just leak the memory allocated for priv->job.completed by
> overwriting the pointer to the one from priv->job.current, ...
>
> > +} else {
> > +VIR_WARN("Unable to allocate job.completed for VM %s",
> obj->def->name);
> > +}
> > +qemuDomainObjResetAsyncJob(priv);
>
> which will point to a freed memory after this call.
>
Agree, I will fix this.
>
> > +qemuDomainObjEndJob(driver, obj);
>
> And while this is almost certainly (I didn't really check though) not
> something you should call from a close callback, you don't save the
> changes to the status XML so once libvirtd restarts, it will think the
> domain is still being migrated.
>
I will add the same to status XML.
I am suggesting that strengthening the job data would be additionally
useful. If the daemon has not restarted, job information can still get us
fairly accurate status of migration. Pls let me know if you think this is
not useful, I will be happy to learn the rationale.
Regards,
Prerna
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] Migration: Preserve the failed job in case migration job is terminated beyond the perform phase.
In case of non-p2p migration, in case libvirt client gets disconnected from
source libvirt
after PERFORM phase is over, the daemon just resets the current migration job.
However, the VM could be left paused on both source and destination in such
case. In case
the client reconnects and queries migration status, the job has been blanked
out from source libvirt,
and this reconnected client has no clear way of figuring out if an unclean
migration had previously
been aborted.
This patch calls out a "potentially" incomplete migration as a failed job, so
that a client may
be able to watch previously failed jobs for this VM and take corrective actions
as needed.
Signed-off-by: Prerna Saxena
---
src/qemu/qemu_domain.c| 16
src/qemu/qemu_domain.h|2 ++
src/qemu/qemu_migration.c |4 ++--
3 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index e8e0313..7c60d17 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4564,6 +4564,22 @@ qemuDomainObjDiscardAsyncJob(virQEMUDriverPtr driver,
virDomainObjPtr obj)
qemuDomainObjSaveJob(driver, obj);
}
+
+void
+qemuDomainObjFailAsyncJob(virQEMUDriverPtr driver, virDomainObjPtr obj)
+{
+qemuDomainObjPrivatePtr priv = obj->privateData;
+VIR_FREE(priv->job.completed);
+if (VIR_ALLOC(priv->job.completed) == 0) {
+priv->job.current->type = VIR_DOMAIN_JOB_FAILED;
+priv->job.completed = priv->job.current;
+} else {
+VIR_WARN("Unable to allocate job.completed for VM %s", obj->def->name);
+}
+qemuDomainObjResetAsyncJob(priv);
+qemuDomainObjEndJob(driver, obj);
+}
+
void
qemuDomainObjReleaseAsyncJob(virDomainObjPtr obj)
{
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index c33af36..6465603 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -497,6 +497,8 @@ void qemuDomainObjRestoreJob(virDomainObjPtr obj,
struct qemuDomainJobObj *job);
void qemuDomainObjDiscardAsyncJob(virQEMUDriverPtr driver,
virDomainObjPtr obj);
+void qemuDomainObjFailAsyncJob(virQEMUDriverPtr driver,
+ virDomainObjPtr obj);
void qemuDomainObjReleaseAsyncJob(virDomainObjPtr obj);
qemuMonitorPtr qemuDomainGetMonitor(virDomainObjPtr vm)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 69eb231..fd8950e 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1911,8 +1911,8 @@ qemuMigrationCleanup(virDomainObjPtr vm,
VIR_WARN("Migration of domain %s finished but we don't know if the"
" domain was successfully started on destination or not",
vm->def->name);
-/* clear the job and let higher levels decide what to do */
-qemuDomainObjDiscardAsyncJob(driver, vm);
+/* clearly "fail" the job and let higher levels decide what to do */
+qemuDomainObjFailAsyncJob(driver, vm);
break;
case QEMU_MIGRATION_PHASE_PERFORM3:
--
1.7.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [[RFC] 0/8] Implement async QEMU event handling in libvirtd.
I spent a while trying to work through this proposal, here are a few points which need more thought: On Wed, Nov 8, 2017 at 7:22 PM, Daniel P. Berrange wrote: > On Mon, Nov 06, 2017 at 06:43:12AM +0100, Prerna wrote: > > Thanks for your reply Daniel. I am still on vacation all of this week so > > have not been able to respond. > > Few questions inline: > > > > On Thu, Oct 26, 2017 at 2:43 PM, Daniel P. Berrange > > > wrote: > > > > > On Tue, Oct 24, 2017 at 10:34:53AM -0700, Prerna Saxena wrote: > > > > > > > > As noted in > > > > https://www.redhat.com/archives/libvir-list/2017-May/msg00016.html > > > > libvirt-QEMU driver handles all async events from the main loop. > > > > Each event handling needs the per-VM lock to make forward progress. > In > > > > the case where an async event is received for the same VM which has > an > > > > RPC running, the main loop is held up contending for the same lock. > > > > > > > > This impacts scalability, and should be addressed on priority. > > > > > > > > Note that libvirt does have a 2-step deferred handling for a few > event > > > > categories, but (1) That is insufficient since blockign happens > before > > > > the handler could disambiguate which one needs to be posted to this > > > > other queue. > > > > (2) There needs to be homogeniety. > > > > > > > > The current series builds a framework for recording and handling VM > > > > events. > > > > It initializes per-VM event queue, and a global event queue pointing > to > > > > events from all the VMs. Event handling is staggered in 2 stages: > > > > - When an event is received, it is enqueued in the per-VM queue as > well > > > > as the global queues. > > > > - The global queue is built into the QEMU Driver as a threadpool > > > > (currently with a single thread). > > > > - Enqueuing of a new event triggers the global event worker thread, > which > > > > then attempts to take a lock for this event's VM. > > > > - If the lock is available, the event worker runs the function > > > handling > > > > this event type. Once done, it dequeues this event from the > global > > > > as well as per-VM queues. > > > > - If the lock is unavailable(ie taken by RPC thread), the event > > > worker > > > > thread leaves this as-is and picks up the next event. > > > > - Once the RPC thread completes, it looks for events pertaining to > the > > > > VM in the per-VM event queue. It then processes the events serially > > > > (holding the VM lock) until there are no more events remaining for > > > > this VM. At this point, the per-VM lock is relinquished. > > > > > > One of the nice aspects of processing the QEMU events in the main event > > > loop is that handling of them is self-throttling. ie if one QEMU > process > > > goes mental and issues lots of events, we'll spend alot of time > processing > > > them all, but our memory usage is still bounded. > > > > > > If we take events from the VM and put them on a queue that is processed > > > asynchronously, and the event processing thread gets stuck for some > > > reason, then libvirt will end up queuing an unbounded number of events. > > > This could cause considerable memory usage in libvirt. This could be > > > exploited by a malicious VM to harm libvirt. eg a malicious QEMU could > > > stop responding to monitor RPC calls, which would tie up the RPC > threads > > > and handling of RPC calls. This would in turn prevent events being > > > processed due to being unable to acquire the state lock. Now the VM > > > can flood libvirtd with events which will all be read off the wire > > > and queued in memory, potentially forever. So libvirt memory usage > > > will balloon to an arbitrary level. > > > > > > Now, the current code isn't great when faced with malicious QEMU > > > because with the same approach, QEMU can cause libvirtd main event > > > loop to stall as you found. This feels less bad than unbounded > > > memory usage though - if libvirt uses lots of memory, this will > > > push other apps on the host into swap, and/or trigger the OOM > > > killer. > > > > > > So I agree that we need to make event processing asynchronous from > > > the main loop. When doing that th
Re: [libvirt] [[RFC] 0/8] Implement async QEMU event handling in libvirtd.
On Wed, Nov 15, 2017 at 12:07 AM, Marc Hartmayer <
mhart...@linux.vnet.ibm.com> wrote:
> On Tue, Oct 24, 2017 at 07:34 PM +0200, Prerna Saxena <
> saxenap@gmail.com> wrote:
> > As noted in
> > https://www.redhat.com/archives/libvir-list/2017-May/msg00016.html
> > libvirt-QEMU driver handles all async events from the main loop.
> > Each event handling needs the per-VM lock to make forward progress. In
> > the case where an async event is received for the same VM which has an
> > RPC running, the main loop is held up contending for the same lock.
>
> What's about the remaining qemuMonitorCallbacks? The main event loop can
> still be 'blocked' by e.g. qemuProcessHandleMonitorEOF if the VM is
> already locked by a worker thread. In fact, we currently have a problem
> with D-Bus which causes a D-Bus call (of a worker thread) to run into
> the timeout of 30 seconds. During these 30 seconds the main event loop
> is stuck.
>
>
EOF handling in the current series is still yet another event, and hence is
done only once worker threads complete.
It needs to go into a priority thread pool, and should wake up RPC workers.
> I tried the patch series and got a segmentation fault:
>
> Thread 1 "libvirtd" received signal SIGSEGV, Segmentation fault.
> 0x03ff98faa452 in virEnqueueVMEvent (qlist=0x3ff908ce760,
> ev=) at ../../src/qemu/qemu_event.c:153 153
> vmq_entry->ev->ev_id = vmq->last->ev->ev_id + 1;
> (gdb) bt
> #0 0x03ff98faa452 in virEnqueueVMEvent (qlist=0x3ff908ce760,
> ev=) at ../../src/qemu/qemu_event.c:153
> #1 0x03ff98fc3564 in qemuProcessEnqueueEvent (mon=,
> vm=, ev=, opaque=0x3ff90548ec0) at
> ../../src/qemu/qemu_process.c:1864
> #2 0x03ff98fe4804 in qemuMonitorEnqueueEvent
> (mon=mon@entry=0x3ff4c007440, ev=0x2aa1e0104c0) at
> ../../src/qemu/qemu_monitor.c:1325
> #3 0x03ff98fe7102 in qemuMonitorEmitShutdown
> (mon=mon@entry=0x3ff4c007440, guest=,
> seconds=seconds@entry=1510683878, micros=micros@entry=703956) at
> ../../src/qemu/qemu_monitor.c:1365
> #4 0x03ff98ffc19a in qemuMonitorJSONHandleShutdown
> (mon=0x3ff4c007440, data=, seconds=1510683878,
> micros=) at ../../src/qemu/qemu_monitor_json.c:552
> #5 0x03ff98ffbb8a in qemuMonitorJSONIOProcessEvent
> (mon=mon@entry=0x3ff4c007440, obj=obj@entry=0x2aa1e012030) at
> ../../src/qemu/qemu_monitor_json.c:208
> #6 0x03ff99002138 in qemuMonitorJSONIOProcessLine
> (mon=mon@entry=0x3ff4c007440, line=0x2aa1e010460 "{\"timestamp\":
> {\"seconds\": 1510683878, \"microseconds\": 703956}, \"event\":
> \"SHUTDOWN\"}", msg=msg@entry=0x0) at
> ../../src/qemu/qemu_monitor_json.c:237
> #7 0x03ff990022b4 in qemuMonitorJSONIOProcess
> (mon=mon@entry=0x3ff4c007440, data=0x2aa1e014bc0 "{\"timestamp\":
> {\"seconds\": 1510683878, \"microseconds\": 703956}, \"event\":
> \"SHUTDOWN\"}\r\n", len=85, msg=msg@entry=0x0) at
> ../../src/qemu/qemu_monitor_json.c:279
> #8 0x03ff98fe4b44 in qemuMonitorIOProcess
> (mon=mon@entry=0x3ff4c007440) at ../../src/qemu/qemu_monitor.c:443
> #9 0x03ff98fe5d00 in qemuMonitorIO (watch=,
> fd=, events=0, opaque=0x3ff4c007440) at
> ../../src/qemu/qemu_monitor.c:697
> #10 0x03ffa68d6442 in virEventPollDispatchHandles (nfds= out>, fds=0x2aa1e013990) at ../../src/util/vireventpoll.c:508
> #11 0x03ffa68d66c8 in virEventPollRunOnce () at
> ../../src/util/vireventpoll.c:657
> #12 0x03ffa68d44e4 in virEventRunDefaultImpl () at
> ../../src/util/virevent.c:327
> #13 0x03ffa6a83c5e in virNetDaemonRun (dmn=0x2aa1dfe3eb0) at
> ../../src/rpc/virnetdaemon.c:838
> #14 0x02aa1df29cc4 in main (argc=, argv= out>) at ../../daemon/libvirtd.c:1494
>
>
Thanks for trying it out. Let me look into this.
> >
> > This impacts scalability, and should be addressed on priority.
> >
> > Note that libvirt does have a 2-step deferred handling for a few event
> > categories, but (1) That is insufficient since blockign happens before
> > the handler could disambiguate which one needs to be posted to this
> > other queue.
> > (2) There needs to be homogeniety.
> >
> > The current series builds a framework for recording and handling VM
> > events.
> > It initializes per-VM event queue, and a global event queue pointing to
> > events from all the VMs. Event handling is staggered in 2 stages:
> > - When an event is received, it is enqueued in the per-VM queue as well
> > as the global queues.
> > - The global queue is built into the QEMU Driver as a threadpool
> > (currently with a single thread).
Re: [libvirt] [[RFC] 0/8] Implement async QEMU event handling in libvirtd.
Thanks for your reply Daniel. I am still on vacation all of this week so have not been able to respond. Few questions inline: On Thu, Oct 26, 2017 at 2:43 PM, Daniel P. Berrange wrote: > On Tue, Oct 24, 2017 at 10:34:53AM -0700, Prerna Saxena wrote: > > > > As noted in > > https://www.redhat.com/archives/libvir-list/2017-May/msg00016.html > > libvirt-QEMU driver handles all async events from the main loop. > > Each event handling needs the per-VM lock to make forward progress. In > > the case where an async event is received for the same VM which has an > > RPC running, the main loop is held up contending for the same lock. > > > > This impacts scalability, and should be addressed on priority. > > > > Note that libvirt does have a 2-step deferred handling for a few event > > categories, but (1) That is insufficient since blockign happens before > > the handler could disambiguate which one needs to be posted to this > > other queue. > > (2) There needs to be homogeniety. > > > > The current series builds a framework for recording and handling VM > > events. > > It initializes per-VM event queue, and a global event queue pointing to > > events from all the VMs. Event handling is staggered in 2 stages: > > - When an event is received, it is enqueued in the per-VM queue as well > > as the global queues. > > - The global queue is built into the QEMU Driver as a threadpool > > (currently with a single thread). > > - Enqueuing of a new event triggers the global event worker thread, which > > then attempts to take a lock for this event's VM. > > - If the lock is available, the event worker runs the function > handling > > this event type. Once done, it dequeues this event from the global > > as well as per-VM queues. > > - If the lock is unavailable(ie taken by RPC thread), the event > worker > > thread leaves this as-is and picks up the next event. > > - Once the RPC thread completes, it looks for events pertaining to the > > VM in the per-VM event queue. It then processes the events serially > > (holding the VM lock) until there are no more events remaining for > > this VM. At this point, the per-VM lock is relinquished. > > One of the nice aspects of processing the QEMU events in the main event > loop is that handling of them is self-throttling. ie if one QEMU process > goes mental and issues lots of events, we'll spend alot of time processing > them all, but our memory usage is still bounded. > > If we take events from the VM and put them on a queue that is processed > asynchronously, and the event processing thread gets stuck for some > reason, then libvirt will end up queuing an unbounded number of events. > This could cause considerable memory usage in libvirt. This could be > exploited by a malicious VM to harm libvirt. eg a malicious QEMU could > stop responding to monitor RPC calls, which would tie up the RPC threads > and handling of RPC calls. This would in turn prevent events being > processed due to being unable to acquire the state lock. Now the VM > can flood libvirtd with events which will all be read off the wire > and queued in memory, potentially forever. So libvirt memory usage > will balloon to an arbitrary level. > > Now, the current code isn't great when faced with malicious QEMU > because with the same approach, QEMU can cause libvirtd main event > loop to stall as you found. This feels less bad than unbounded > memory usage though - if libvirt uses lots of memory, this will > push other apps on the host into swap, and/or trigger the OOM > killer. > > So I agree that we need to make event processing asynchronous from > the main loop. When doing that through, I think we need to put an > upper limit on the number of events we're willing to queue from a > VM. When we get that limit, we should update the monitor event loop > watch so that we stop reading further events, until existing events > have been processed. > > We can update the watch at the monitor socket level. Ie, if we have hit threshold limits reading events off the monitor socket, we ignore this socket FD going forward. Now, this also means that we will miss any replies coming off the same socket. It will mean that legitimate RPC replies coming off the same socket will get ignored too. And in this case, we deadlock, since event notifications will not be processed until the ongoing RPC completes. > The other attractive thing bout the way events currently work is that > it is also automatically avoids lock acquisition priority problems > wrt incoming RPC calls. ie, we are guaranteed that once thread > workers have finished all currnetly queued
Re: [libvirt] [[RFC] 0/8] Implement async QEMU event handling in libvirtd.
On Wed, Oct 25, 2017 at 4:12 PM, Jiri Denemark wrote: > On Tue, Oct 24, 2017 at 10:34:53 -0700, Prerna Saxena wrote: > > > > As noted in > > https://www.redhat.com/archives/libvir-list/2017-May/msg00016.html > > libvirt-QEMU driver handles all async events from the main loop. > > Each event handling needs the per-VM lock to make forward progress. In > > the case where an async event is received for the same VM which has an > > RPC running, the main loop is held up contending for the same lock. > > > > This impacts scalability, and should be addressed on priority. > > > > Note that libvirt does have a 2-step deferred handling for a few event > > categories, but (1) That is insufficient since blockign happens before > > the handler could disambiguate which one needs to be posted to this > > other queue. > > (2) There needs to be homogeniety. > > > > The current series builds a framework for recording and handling VM > > events. > > It initializes per-VM event queue, and a global event queue pointing to > > events from all the VMs. Event handling is staggered in 2 stages: > > - When an event is received, it is enqueued in the per-VM queue as well > > as the global queues. > > - The global queue is built into the QEMU Driver as a threadpool > > (currently with a single thread). > > - Enqueuing of a new event triggers the global event worker thread, which > > then attempts to take a lock for this event's VM. > > - If the lock is available, the event worker runs the function > handling > > this event type. Once done, it dequeues this event from the global > > as well as per-VM queues. > > - If the lock is unavailable(ie taken by RPC thread), the event > worker > > thread leaves this as-is and picks up the next event. > > If I get it right, the event is either processed immediately when its VM > object is unlocked or it has to wait until the current job running on > the VM object finishes even though the lock may be released before that. > Correct? If so, this needs to be addressed. > In most cases, the lock is released just before we end the API. However, it is a small change that can be made. > > > - Once the RPC thread completes, it looks for events pertaining to the > > VM in the per-VM event queue. It then processes the events serially > > (holding the VM lock) until there are no more events remaining for > > this VM. At this point, the per-VM lock is relinquished. > > > > Patch Series status: > > Strictly RFC only. No compilation issues. I have not had a chance to > > (stress) test it after rebase to latest master. > > Note that documentation and test coverage is TBD, since a few open > > points remain. > > > > Known issues/ caveats: > > - RPC handling time will become non-deterministic. > > - An event will only be "notified" to a client once the RPC for same VM > completes. > > - Needs careful consideration in all cases where a QMP event is used to > > "signal" an RPC thread, else will deadlock. > > This last issue is actually a show stopper here. We need to make sure > QMP events are processed while a job is still active on the same domain. > Otherwise thinks kile block jobs and migration, which are long running > jobs driven by events, will break. > > Jirka > Completely agree, which is why I have explicitly mentioned this. However, I do not completely follow why it needs to be this way. Can the block job APIs between QEMU <--> libvirt be fixed so that such behaviour is avoided ? Regards, Prerna -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [[RFC] 2/8] QEMU Event handling: Introduce async event helpers in qemu_event.[ch]
These contain basic functions for setting up event lists (global
as well as per-VM). Also include methods for enqueuing and
dequeuing events.
Per-event metadata is also encoded herewith.
Signed-off-by: Prerna Saxena
---
src/Makefile.am | 1 +
src/qemu/qemu_event.c | 75 +
src/qemu/qemu_event.h | 224 ++
3 files changed, 300 insertions(+)
create mode 100644 src/qemu/qemu_event.c
create mode 100644 src/qemu/qemu_event.h
diff --git a/src/Makefile.am b/src/Makefile.am
index b74856b..73a98ca 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -903,6 +903,7 @@ QEMU_DRIVER_SOURCES =
\
qemu/qemu_domain.c qemu/qemu_domain.h \
qemu/qemu_domain_address.c qemu/qemu_domain_address.h \
qemu/qemu_cgroup.c qemu/qemu_cgroup.h \
+ qemu/qemu_event.c qemu/qemu_event.h \
qemu/qemu_hostdev.c qemu/qemu_hostdev.h \
qemu/qemu_hotplug.c qemu/qemu_hotplug.h \
qemu/qemu_hotplugpriv.h \
diff --git a/src/qemu/qemu_event.c b/src/qemu/qemu_event.c
new file mode 100644
index 000..e27ea0d
--- /dev/null
+++ b/src/qemu/qemu_event.c
@@ -0,0 +1,75 @@
+/*
+ * qemu_event.c:
+ *optimize qemu async event handling.
+ *
+ * Copyright (C) 2017-2026 Nutanix, Inc.
+ * Copyright (C) 2017 Prerna Saxena
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library. If not, see
+ * <http://www.gnu.org/licenses/>.
+ *
+ * Author: Prerna Saxena
+ */
+
+#include "config.h"
+#include "internal.h"
+# include "qemu_monitor.h"
+# include "qemu_conf.h"
+# include "qemu_event.h"
+#include "qemu_process.h"
+
+#include "virerror.h"
+#include "viralloc.h"
+#include "virlog.h"
+#include "virobject.h"
+#include "virstring.h"
+
+#define VIR_FROM_THIS VIR_FROM_QEMU
+
+VIR_LOG_INIT("qemu.qemu_event");
+
+VIR_ENUM_IMPL(qemuMonitorEvent,
+ QEMU_EVENT_LAST,
+ "ACPI Event", "Balloon Change", "Block IO Error",
+ "Block Job Event",
+ "Block Write Threshold", "Device Deleted",
+ "Device Tray Moved", "Graphics", "Guest Panicked",
+ "Migration", "Migration pass",
+ "Nic RX Filter Changed", "Powerdown", "Reset", "Resume",
+ "RTC Change", "Shutdown", "Stop",
+ "Suspend", "Suspend To Disk",
+ "Virtual Serial Port Change",
+ "Wakeup", "Watchdog");
+
+virQemuEventList* virQemuEventListInit(void)
+{
+virQemuEventList *ev_list;
+if (VIR_ALLOC(ev_list) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ "Unable to allocate virQemuEventList");
+return NULL;
+}
+
+if (virMutexInit(&ev_list->lock) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ "%s", _("cannot initialize mutex"));
+VIR_FREE(ev_list);
+return NULL;
+}
+
+ev_list->head = NULL;
+ev_list->last = NULL;
+
+return ev_list;
+}
diff --git a/src/qemu/qemu_event.h b/src/qemu/qemu_event.h
new file mode 100644
index 000..9781795
--- /dev/null
+++ b/src/qemu/qemu_event.h
@@ -0,0 +1,224 @@
+/*
+ * qemu_event.h: interaction with QEMU JSON monitor event layer
+ * Carve out improved interactions with qemu.
+ *
+ * Copyright (C) 2017-2026 Nutanix, Inc.
+ * Copyright (C) 2017 Prerna Saxena
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOS
[libvirt] [[RFC] 4/8] Events: Allow monitor to "enqueue" events to a queue. Also introduce a framework of handlers for each event type, that can be called when the handler is running an event.
Signed-off-by: Prerna Saxena
---
src/qemu/qemu_event.c| 11 +
src/qemu/qemu_event.h| 8 +
src/qemu/qemu_monitor.c | 592 +++-
src/qemu/qemu_monitor.h | 80 +++--
src/qemu/qemu_monitor_json.c | 291 ++--
src/qemu/qemu_process.c | 789 +++
src/qemu/qemu_process.h | 2 +
tests/qemumonitortestutils.c | 2 +-
8 files changed, 1273 insertions(+), 502 deletions(-)
diff --git a/src/qemu/qemu_event.c b/src/qemu/qemu_event.c
index d52fad2..beb309f 100644
--- a/src/qemu/qemu_event.c
+++ b/src/qemu/qemu_event.c
@@ -50,6 +50,7 @@ VIR_ENUM_IMPL(qemuMonitorEvent,
"RTC Change", "Shutdown", "Stop",
"Suspend", "Suspend To Disk",
"Virtual Serial Port Change",
+ "Spice migrated",
"Wakeup", "Watchdog");
virQemuEventList* virQemuEventListInit(void)
@@ -302,3 +303,13 @@ void virDomainConsumeVMEvents(virDomainObjPtr vm, void
*opaque)
}
return;
}
+
+extern void qemuProcessEmitMonitorEvent(qemuEventPtr ev, void *opaque);
+
+void virEventRunHandler(qemuEventPtr ev, void *opaque)
+{
+if (!ev)
+return;
+
+return qemuProcessEmitMonitorEvent(ev, opaque);
+}
diff --git a/src/qemu/qemu_event.h b/src/qemu/qemu_event.h
index 4173834..8552fd1 100644
--- a/src/qemu/qemu_event.h
+++ b/src/qemu/qemu_event.h
@@ -51,6 +51,7 @@ typedef enum {
QEMU_EVENT_SUSPEND,
QEMU_EVENT_SUSPEND_DISK,
QEMU_EVENT_SERIAL_CHANGE,
+QEMU_EVENT_SPICE_MIGRATED,
QEMU_EVENT_WAKEUP,
QEMU_EVENT_WATCHDOG,
@@ -102,7 +103,12 @@ struct qemuEventTrayChangeData {
int reason;
};
+struct qemuEventShutdownData {
+virTristateBool guest_initiated;
+};
+
struct qemuEventGuestPanicData {
+void *info;
};
struct qemuEventMigrationStatusData {
@@ -159,6 +165,7 @@ struct _qemuEvent {
struct qemuEventBlockThresholdData ev_threshold;
struct qemuEventDeviceDeletedData ev_deviceDel;
struct qemuEventTrayChangeData ev_tray;
+struct qemuEventShutdownData ev_shutdown;
struct qemuEventGuestPanicData ev_panic;
struct qemuEventMigrationStatusData ev_migStatus;
struct qemuEventMigrationPassData ev_migPass;
@@ -219,5 +226,6 @@ int virQemuVmEventListInit(virDomainObjPtr vm);
int virEnqueueVMEvent(virQemuEventList *qlist, qemuEventPtr ev);
qemuEventPtr virDequeueVMEvent(virQemuEventList *qlist, virDomainObjPtr vm);
void virEventWorkerScanQueue(void *dummy, void *opaque);
+void virEventRunHandler(qemuEventPtr ev, void *opaque);
void virDomainConsumeVMEvents(virDomainObjPtr vm, void *opaque);
#endif
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 7a26785..4e45cf9 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -34,6 +34,7 @@
#include "qemu_monitor_json.h"
#include "qemu_domain.h"
#include "qemu_process.h"
+#include "qemu_event.h"
#include "virerror.h"
#include "viralloc.h"
#include "virlog.h"
@@ -1316,6 +1317,14 @@ qemuMonitorGetDiskSecret(qemuMonitorPtr mon,
return ret;
}
+static int
+qemuMonitorEnqueueEvent(qemuMonitorPtr mon, qemuEventPtr ev)
+{
+int ret = -1;
+QEMU_MONITOR_CALLBACK(mon, ret, domainEnqueueEvent, ev->vm, ev);
+
+return ret;
+}
int
qemuMonitorEmitEvent(qemuMonitorPtr mon, const char *event,
@@ -1332,90 +1341,189 @@ qemuMonitorEmitEvent(qemuMonitorPtr mon, const char
*event,
int
-qemuMonitorEmitShutdown(qemuMonitorPtr mon, virTristateBool guest)
+qemuMonitorEmitShutdown(qemuMonitorPtr mon, virTristateBool guest,
+long long seconds, unsigned int micros)
{
int ret = -1;
-VIR_DEBUG("mon=%p guest=%u", mon, guest);
mon->willhangup = 1;
+qemuEventPtr ev;
+
+if (VIR_ALLOC(ev) < 0){
+return ret;
+}
+
+ev->ev_type = QEMU_EVENT_SHUTDOWN;
+ev->vm = mon->vm;
+ev->seconds = seconds;
+ev->micros = micros;
+ev->evData.ev_shutdown.guest_initiated = guest;
-QEMU_MONITOR_CALLBACK(mon, ret, domainShutdown, mon->vm, guest);
+VIR_DEBUG("Vm %s received shutdown event initiated by %u",
+mon->vm->def->name, guest);
+virObjectRef(ev->vm);
+ret = qemuMonitorEnqueueEvent(mon, ev);
return ret;
}
int
-qemuMonitorEmitReset(qemuMonitorPtr mon)
+qemuMonitorEmitReset(qemuMonitorPtr mon, long long seconds, unsigned int
micros)
{
int ret = -1;
-VIR_DEBUG("mon=%p", mon);
+qemuEventPtr ev;
+
+if (VIR_ALLOC(ev) < 0){
+return ret;
+}
+
+ev->ev_type = QEMU_EVENT_RESET;
+ev->vm = mon->vm;
+ev->seconds = seconds;
+ev->micros = micros;
-QEMU_MONITOR_CAL
[libvirt] [[RFC] 7/8] Fold back the 2-stage event implementation for a few events : Watchdog, Monitor EOF, Serial changed, Guest panic, Nic RX filter changed .. into single level.
Also, the enqueuing of a new event now triggers virEventWorkerScanQueue()
Signed-off-by: Prerna Saxena
---
src/qemu/qemu_driver.c | 61 ++--
src/qemu/qemu_process.c | 121 +++-
2 files changed, 29 insertions(+), 153 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 9d495fb..881f253 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -138,8 +138,6 @@ VIR_LOG_INIT("qemu.qemu_driver");
#define QEMU_NB_BANDWIDTH_PARAM 7
-static void qemuProcessEventHandler(void *data, void *opaque);
-
static int qemuStateCleanup(void);
static int qemuDomainObjStart(virConnectPtr conn,
@@ -936,7 +934,9 @@ qemuStateInitialize(bool privileged,
qemuProcessReconnectAll(conn, qemu_driver);
-qemu_driver->workerPool = virThreadPoolNew(0, 1, 0,
qemuProcessEventHandler, qemu_driver);
+qemu_driver->workerPool = virThreadPoolNew(0, 1, 0,
virEventWorkerScanQueue,
+
qemu_driver);
+
if (!qemu_driver->workerPool)
goto error;
@@ -3645,61 +3645,6 @@ qemuDomainScreenshot(virDomainPtr dom,
}
-
-
-
-
-
-
-
-
-static void qemuProcessEventHandler(void *data, void *opaque)
-{
-struct qemuProcessEvent *processEvent = data;
-virDomainObjPtr vm = processEvent->vm;
-virQEMUDriverPtr driver = opaque;
-
-VIR_DEBUG("vm=%p, event=%d", vm, processEvent->eventType);
-
-virObjectLock(vm);
-
-switch (processEvent->eventType) {
-case QEMU_PROCESS_EVENT_WATCHDOG:
-processWatchdogEvent(driver, vm, processEvent->action);
-break;
-case QEMU_PROCESS_EVENT_GUESTPANIC:
-processGuestPanicEvent(driver, vm, processEvent->action,
- processEvent->data);
-break;
-case QEMU_PROCESS_EVENT_DEVICE_DELETED:
-processDeviceDeletedEvent(driver, vm, processEvent->data);
-break;
-case QEMU_PROCESS_EVENT_NIC_RX_FILTER_CHANGED:
-processNicRxFilterChangedEvent(driver, vm, processEvent->data);
-break;
-case QEMU_PROCESS_EVENT_SERIAL_CHANGED:
-processSerialChangedEvent(driver, vm, processEvent->data,
- processEvent->action);
-break;
-case QEMU_PROCESS_EVENT_BLOCK_JOB:
-processBlockJobEvent(driver, vm,
- processEvent->data,
- processEvent->action,
- processEvent->status);
-break;
-case QEMU_PROCESS_EVENT_MONITOR_EOF:
-processMonitorEOFEvent(driver, vm);
-break;
-case QEMU_PROCESS_EVENT_LAST:
-break;
-}
-
-virDomainConsumeVMEvents(vm, driver);
-virDomainObjEndAPI(&vm);
-VIR_FREE(processEvent);
-}
-
-
static int
qemuDomainSetVcpusAgent(virDomainObjPtr vm,
unsigned int nvcpus)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index d2b5fe8..f9270e0 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -962,21 +962,11 @@ qemuProcessEventHandleWatchdog1(qemuEventPtr ev,
}
if (vm->def->watchdog->action == VIR_DOMAIN_WATCHDOG_ACTION_DUMP) {
-struct qemuProcessEvent *processEvent;
-if (VIR_ALLOC(processEvent) == 0) {
-processEvent->eventType = QEMU_PROCESS_EVENT_WATCHDOG;
-processEvent->action = VIR_DOMAIN_WATCHDOG_ACTION_DUMP;
-processEvent->vm = vm;
-/* Hold an extra reference because we can't allow 'vm' to be
- * deleted before handling watchdog event is finished.
- */
-virObjectRef(vm);
-if (virThreadPoolSendJob(driver->workerPool, 0, processEvent) < 0)
{
-if (!virObjectUnref(vm))
-vm = NULL;
-VIR_FREE(processEvent);
-}
-}
+/* Hold an extra reference because we can't allow 'vm' to be
+ * deleted before handling watchdog event is finished.*/
+virObjectRef(vm);
+processWatchdogEvent(driver, vm, VIR_DOMAIN_WATCHDOG_ACTION_DUMP);
+virObjectUnref(vm);
}
qemuDomainEventQueue(driver, watchdogEvent);
@@ -1068,12 +1058,10 @@ qemuProcessEventHandleBlockJob(qemuEventPtr ev,
void *opaque)
{
virQEMUDriverPtr driver = opaque;
-struct qemuProcessEvent *processEvent = NULL;
virDomainDiskDefPtr disk;
qemuDomainDiskPrivatePtr diskPriv;
-char *data = NULL;
virDomainObjPtr vm;
-const char *diskAlias;
+char *diskAlias = NULL;
int type, status;
if (!ev)
@@ -1082,7 +1070,7 @@ qemuProcessEventHandleBlockJob(qemuEventPtr ev,
if (!ev->vm) {
VIR_WARN("Unable to locate VM, dropping Block Job event");
-goto cleanup;
+
[libvirt] [[RFC] 5/8] Events: Plumb event handling calls before a domain's APIs complete.
Signed-off-by: Prerna Saxena
---
src/qemu/qemu_driver.c | 131 +++--
1 file changed, 128 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 8a005d0..b249347 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1900,6 +1900,7 @@ static int qemuDomainSuspend(virDomainPtr dom)
qemuDomainObjEndJob(driver, vm);
cleanup:
+virDomainConsumeVMEvents(vm, driver);
virDomainObjEndAPI(&vm);
qemuDomainEventQueue(driver, event);
@@ -1967,6 +1968,7 @@ static int qemuDomainResume(virDomainPtr dom)
qemuDomainObjEndJob(driver, vm);
cleanup:
+virDomainConsumeVMEvents(vm, driver);
virDomainObjEndAPI(&vm);
qemuDomainEventQueue(driver, event);
virObjectUnref(cfg);
@@ -2057,6 +2059,7 @@ static int qemuDomainShutdownFlags(virDomainPtr dom,
unsigned int flags)
qemuDomainObjEndJob(driver, vm);
cleanup:
+virDomainConsumeVMEvents(vm, driver);
virDomainObjEndAPI(&vm);
return ret;
}
@@ -2159,6 +2162,7 @@ qemuDomainReboot(virDomainPtr dom, unsigned int flags)
qemuDomainObjEndJob(driver, vm);
cleanup:
+virDomainConsumeVMEvents(vm, driver);
virDomainObjEndAPI(&vm);
return ret;
}
@@ -2206,6 +2210,7 @@ qemuDomainReset(virDomainPtr dom, unsigned int flags)
qemuDomainObjEndJob(driver, vm);
cleanup:
+virDomainConsumeVMEvents(vm, driver);
virDomainObjEndAPI(&vm);
return ret;
}
@@ -2294,6 +2299,7 @@ qemuDomainDestroyFlags(virDomainPtr dom,
qemuDomainObjEndJob(driver, vm);
cleanup:
+virDomainConsumeVMEvents(vm, driver);
virDomainObjEndAPI(&vm);
qemuDomainEventQueue(driver, event);
return ret;
@@ -2307,6 +2313,7 @@ qemuDomainDestroy(virDomainPtr dom)
static char *qemuDomainGetOSType(virDomainPtr dom) {
virDomainObjPtr vm;
+virQEMUDriverPtr driver = dom->conn->privateData;
char *type = NULL;
if (!(vm = qemuDomObjFromDomain(dom)))
@@ -2318,6 +2325,7 @@ static char *qemuDomainGetOSType(virDomainPtr dom) {
ignore_value(VIR_STRDUP(type, virDomainOSTypeToString(vm->def->os.type)));
cleanup:
+virDomainConsumeVMEvents(vm, driver);
virDomainObjEndAPI(&vm);
return type;
}
@@ -2328,6 +2336,7 @@ qemuDomainGetMaxMemory(virDomainPtr dom)
{
virDomainObjPtr vm;
unsigned long long ret = 0;
+virQEMUDriverPtr driver = dom->conn->privateData;
if (!(vm = qemuDomObjFromDomain(dom)))
goto cleanup;
@@ -2338,6 +2347,7 @@ qemuDomainGetMaxMemory(virDomainPtr dom)
ret = virDomainDefGetMemoryTotal(vm->def);
cleanup:
+virDomainConsumeVMEvents(vm, driver);
virDomainObjEndAPI(&vm);
return ret;
}
@@ -2455,6 +2465,7 @@ static int qemuDomainSetMemoryFlags(virDomainPtr dom,
unsigned long newmem,
qemuDomainObjEndJob(driver, vm);
cleanup:
+virDomainConsumeVMEvents(vm, driver);
virDomainObjEndAPI(&vm);
virObjectUnref(cfg);
return ret;
@@ -2543,6 +2554,7 @@ static int qemuDomainSetMemoryStatsPeriod(virDomainPtr
dom, int period,
qemuDomainObjEndJob(driver, vm);
cleanup:
+virDomainConsumeVMEvents(vm, driver);
virDomainObjEndAPI(&vm);
virObjectUnref(cfg);
return ret;
@@ -2583,6 +2595,7 @@ static int qemuDomainInjectNMI(virDomainPtr domain,
unsigned int flags)
qemuDomainObjEndJob(driver, vm);
cleanup:
+virDomainConsumeVMEvents(vm, driver);
virDomainObjEndAPI(&vm);
return ret;
}
@@ -2646,6 +2659,7 @@ static int qemuDomainSendKey(virDomainPtr domain,
qemuDomainObjEndJob(driver, vm);
cleanup:
+virDomainConsumeVMEvents(vm, driver);
virDomainObjEndAPI(&vm);
return ret;
}
@@ -2702,6 +2716,7 @@ qemuDomainGetInfo(virDomainPtr dom,
ret = 0;
cleanup:
+virDomainConsumeVMEvents(vm, driver);
virDomainObjEndAPI(&vm);
return ret;
}
@@ -2739,6 +2754,7 @@ qemuDomainGetControlInfo(virDomainPtr dom,
virDomainObjPtr vm;
qemuDomainObjPrivatePtr priv;
int ret = -1;
+virQEMUDriverPtr driver = dom->conn->privateData;
virCheckFlags(0, -1);
@@ -2788,6 +2804,7 @@ qemuDomainGetControlInfo(virDomainPtr dom,
ret = 0;
cleanup:
+virDomainConsumeVMEvents(vm, driver);
virDomainObjEndAPI(&vm);
return ret;
}
@@ -3577,6 +3594,7 @@ qemuDomainSaveFlags(virDomainPtr dom, const char *path,
const char *dxml,
compressedpath, dxml, flags);
cleanup:
+virDomainConsumeVMEvents(vm, driver);
virDomainObjEndAPI(&vm);
VIR_FREE(compressedpath);
virObjectUnref(cfg);
@@ -3653,6 +3671,7 @@ qemuDomainManagedSave(virDomainPtr dom, unsigned int
flags)
vm->hasManagedSave = true;
cleanup:
+virDomainConsumeVMEvents(vm, driver);
virDomainObjEndAPI(&vm);
VIR_FREE(name);
VIR_FREE(compressedpath);
@@ -3689,7 +3708,7 @@ qemuDoma
[libvirt] [[RFC] 8/8] Initialize the per-VM event queues in context of domain init.
Signed-off-by: Prerna Saxena
---
src/qemu/qemu_driver.c | 20
src/qemu/qemu_event.c | 2 ++
2 files changed, 22 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 881f253..e4b6d06 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1781,6 +1781,11 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr
conn,
VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
NULL)))
goto cleanup;
+
+if (virQemuVmEventListInit(vm) < 0) {
+virDomainObjListRemove(driver->domains, vm);
+goto cleanup;
+}
virObjectRef(vm);
def = NULL;
@@ -5531,6 +5536,11 @@ qemuDomainRestoreFlags(virConnectPtr conn,
VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
NULL)))
goto cleanup;
+
+if (virQemuVmEventListInit(vm) < 0) {
+virDomainObjListRemove(driver->domains, vm);
+goto cleanup;
+}
virObjectRef(vm);
def = NULL;
@@ -6208,6 +6218,11 @@ qemuDomainDefineXMLFlags(virConnectPtr conn,
0, &oldDef)))
goto cleanup;
+if (virQemuVmEventListInit(vm) < 0) {
+virDomainObjListRemove(driver->domains, vm);
+goto cleanup;
+}
+
virObjectRef(vm);
def = NULL;
if (qemuDomainHasBlockjob(vm, true)) {
@@ -15073,6 +15088,11 @@ static virDomainPtr qemuDomainQemuAttach(virConnectPtr
conn,
NULL)))
goto cleanup;
+if (virQemuVmEventListInit(vm) < 0) {
+virDomainObjListRemove(driver->domains, vm);
+goto cleanup;
+}
+
virObjectRef(vm);
def = NULL;
diff --git a/src/qemu/qemu_event.c b/src/qemu/qemu_event.c
index beb309f..00a06ee 100644
--- a/src/qemu/qemu_event.c
+++ b/src/qemu/qemu_event.c
@@ -145,6 +145,8 @@ int virEnqueueVMEvent(virQemuEventList *qlist, qemuEventPtr
ev)
/* Insert into per-Vm queue */
vmq = ev->vm->vmq;
+if (!vmq)
+goto error;
virMutexLock(&(vmq->lock));
if (vmq->last) {
--
2.9.5
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [[RFC] 6/8] Code refactor: Move helper functions of doCoreDump*, syncNicRxFilter*, and qemuOpenFile* to qemu_process.[ch]
Signed-off-by: Prerna Saxena
---
src/qemu/qemu_driver.c | 1161 ---
src/qemu/qemu_process.c | 1133 +
src/qemu/qemu_process.h | 86
3 files changed, 1219 insertions(+), 1161 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index b249347..9d495fb 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -151,11 +151,6 @@ static int qemuDomainObjStart(virConnectPtr conn,
static int qemuDomainManagedSaveLoad(virDomainObjPtr vm,
void *opaque);
-static int qemuOpenFileAs(uid_t fallback_uid, gid_t fallback_gid,
- bool dynamicOwnership,
- const char *path, int oflags,
- bool *needUnlink, bool *bypassSecurityDriver);
-
static int qemuGetDHCPInterfaces(virDomainPtr dom,
virDomainObjPtr vm,
virDomainInterfacePtr **ifaces);
@@ -2819,38 +2814,6 @@ qemuDomainGetControlInfo(virDomainPtr dom,
verify(sizeof(QEMU_SAVE_MAGIC) == sizeof(QEMU_SAVE_PARTIAL));
-typedef enum {
-QEMU_SAVE_FORMAT_RAW = 0,
-QEMU_SAVE_FORMAT_GZIP = 1,
-QEMU_SAVE_FORMAT_BZIP2 = 2,
-/*
- * Deprecated by xz and never used as part of a release
- * QEMU_SAVE_FORMAT_LZMA
- */
-QEMU_SAVE_FORMAT_XZ = 3,
-QEMU_SAVE_FORMAT_LZOP = 4,
-/* Note: add new members only at the end.
- These values are used in the on-disk format.
- Do not change or re-use numbers. */
-
-QEMU_SAVE_FORMAT_LAST
-} virQEMUSaveFormat;
-
-VIR_ENUM_DECL(qemuSaveCompression)
-VIR_ENUM_IMPL(qemuSaveCompression, QEMU_SAVE_FORMAT_LAST,
- "raw",
- "gzip",
- "bzip2",
- "xz",
- "lzop")
-
-VIR_ENUM_DECL(qemuDumpFormat)
-VIR_ENUM_IMPL(qemuDumpFormat, VIR_DOMAIN_CORE_DUMP_FORMAT_LAST,
- "elf",
- "kdump-zlib",
- "kdump-lzo",
- "kdump-snappy")
-
typedef struct _virQEMUSaveHeader virQEMUSaveHeader;
typedef virQEMUSaveHeader *virQEMUSaveHeaderPtr;
struct _virQEMUSaveHeader {
@@ -3062,214 +3025,6 @@ qemuCompressGetCommand(virQEMUSaveFormat compression)
return ret;
}
-/**
- * qemuOpenFile:
- * @driver: driver object
- * @vm: domain object
- * @path: path to file to open
- * @oflags: flags for opening/creation of the file
- * @needUnlink: set to true if file was created by this function
- * @bypassSecurityDriver: optional pointer to a boolean that will be set to
true
- *if security driver operations are pointless (due to
- *NFS mount)
- *
- * Internal function to properly create or open existing files, with
- * ownership affected by qemu driver setup and domain DAC label.
- *
- * Returns the file descriptor on success and negative errno on failure.
- *
- * This function should not be used on storage sources. Use
- * qemuDomainStorageFileInit and storage driver APIs if possible.
- **/
-static int
-qemuOpenFile(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- const char *path,
- int oflags,
- bool *needUnlink,
- bool *bypassSecurityDriver)
-{
-int ret = -1;
-virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
-uid_t user = cfg->user;
-gid_t group = cfg->group;
-bool dynamicOwnership = cfg->dynamicOwnership;
-virSecurityLabelDefPtr seclabel;
-
-virObjectUnref(cfg);
-
-/* TODO: Take imagelabel into account? */
-if (vm &&
-(seclabel = virDomainDefGetSecurityLabelDef(vm->def, "dac")) != NULL &&
-seclabel->label != NULL &&
-(virParseOwnershipIds(seclabel->label, &user, &group) < 0))
-goto cleanup;
-
-ret = qemuOpenFileAs(user, group, dynamicOwnership,
- path, oflags, needUnlink, bypassSecurityDriver);
-
- cleanup:
-return ret;
-}
-
-static int
-qemuOpenFileAs(uid_t fallback_uid, gid_t fallback_gid,
- bool dynamicOwnership,
- const char *path, int oflags,
- bool *needUnlink, bool *bypassSecurityDriver)
-{
-struct stat sb;
-bool is_reg = true;
-bool need_unlink = false;
-bool bypass_security = false;
-unsigned int vfoflags = 0;
-int fd = -1;
-int path_shared = virFileIsSharedFS(path);
-uid_t uid = geteuid();
-gid_t gid = getegid();
-
-/* path might be a pre-existing block dev, in which case
- * we need to skip the create step, and also avoid unlink
- * in the failure case */
-if (oflags & O_CREAT) {
-need_unlink = true;
-
-/* Don't force chown on network-shared FS
- * as it is likely to fail. */
-if (path_shared <=
[libvirt] [[RFC] 1/8] Introduce virObjectTrylock()
This is a wrapper function that:
(1) Attempts to take a lock on the object.
(2) gracefully returns if the object is already locked.
Signed-off-by: Prerna Saxena
---
src/libvirt_private.syms | 1 +
src/util/virobject.c | 26 ++
src/util/virobject.h | 4
src/util/virthread.c | 5 +
src/util/virthread.h | 1 +
5 files changed, 37 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 9243c55..c0ab8b5 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2362,6 +2362,7 @@ virObjectRWLockableNew;
virObjectRWLockRead;
virObjectRWLockWrite;
virObjectRWUnlock;
+virObjectTrylock;
virObjectUnlock;
virObjectUnref;
diff --git a/src/util/virobject.c b/src/util/virobject.c
index cfa821c..796ea06 100644
--- a/src/util/virobject.c
+++ b/src/util/virobject.c
@@ -495,6 +495,32 @@ virObjectRWLockWrite(void *anyobj)
/**
+ * virObjectTrylock:
+ * @anyobj: any instance of virObjectLockable or virObjectRWLockable
+ *
+ * Attempt to acquire a lock on @anyobj. The lock must be released by
+ * virObjectUnlock.
+ * Returns:
+ *0: If the lock was successfully taken.
+ *errno : Indicates error.
+ *
+ * The caller is expected to have acquired a reference
+ * on the object before locking it (eg virObjectRef).
+ * The object must be unlocked before releasing this
+ * reference.
+ */
+int
+virObjectTrylock(void *anyobj)
+{
+virObjectLockablePtr obj = virObjectGetLockableObj(anyobj);
+
+if (!obj)
+return -1;
+
+return virMutexTrylock(&obj->lock);
+}
+
+/**
* virObjectUnlock:
* @anyobj: any instance of virObjectLockable
*
diff --git a/src/util/virobject.h b/src/util/virobject.h
index ac6cf22..402ea32 100644
--- a/src/util/virobject.h
+++ b/src/util/virobject.h
@@ -124,6 +124,10 @@ void
virObjectLock(void *lockableobj)
ATTRIBUTE_NONNULL(1);
+int
+virObjectTrylock(void *lockableobj)
+ATTRIBUTE_NONNULL(1);
+
void
virObjectRWLockRead(void *lockableobj)
ATTRIBUTE_NONNULL(1);
diff --git a/src/util/virthread.c b/src/util/virthread.c
index 6c49515..07b7a3f 100644
--- a/src/util/virthread.c
+++ b/src/util/virthread.c
@@ -89,6 +89,11 @@ void virMutexLock(virMutexPtr m)
pthread_mutex_lock(&m->lock);
}
+int virMutexTrylock(virMutexPtr m)
+{
+return pthread_mutex_trylock(&m->lock);
+}
+
void virMutexUnlock(virMutexPtr m)
{
pthread_mutex_unlock(&m->lock);
diff --git a/src/util/virthread.h b/src/util/virthread.h
index e466d9b..8e3da2c 100644
--- a/src/util/virthread.h
+++ b/src/util/virthread.h
@@ -132,6 +132,7 @@ int virMutexInitRecursive(virMutexPtr m)
ATTRIBUTE_RETURN_CHECK;
void virMutexDestroy(virMutexPtr m);
void virMutexLock(virMutexPtr m);
+int virMutexTrylock(virMutexPtr m);
void virMutexUnlock(virMutexPtr m);
--
2.9.5
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [[RFC] 3/8] Setup global and per-VM event queues. Also initialize per-VM queues when libvirt reconnects to an existing VM.
Signed-off-by: Prerna Saxena
---
src/conf/domain_conf.h | 3 +
src/qemu/qemu_conf.h| 4 +
src/qemu/qemu_driver.c | 9 ++
src/qemu/qemu_event.c | 229
src/qemu/qemu_event.h | 1 -
src/qemu/qemu_process.c | 2 +
6 files changed, 247 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index a42efcf..7fe38e7 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2496,6 +2496,9 @@ struct _virDomainObj {
unsigned long long original_memlock; /* Original RLIMIT_MEMLOCK, zero if no
* restore will be required later */
+
+/* Pointer to per-VM Event Queue */
+void *vmq;
};
typedef bool (*virDomainObjListACLFilter)(virConnectPtr conn,
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 13b6f81..e63dc98 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -33,6 +33,7 @@
# include "domain_conf.h"
# include "snapshot_conf.h"
# include "domain_event.h"
+# include "qemu_event.h"
# include "virthread.h"
# include "security/security_manager.h"
# include "virpci.h"
@@ -235,6 +236,9 @@ struct _virQEMUDriver {
/* Immutable pointer, self-locking APIs */
virDomainObjListPtr domains;
+/* Immutable pointer, contains Qemu Driver Event List */
+virQemuEventList *ev_list;
+
/* Immutable pointer */
char *qemuImgBinary;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 7c6f167..8a005d0 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -52,6 +52,7 @@
#include "qemu_command.h"
#include "qemu_parse_command.h"
#include "qemu_cgroup.h"
+#include "qemu_event.h"
#include "qemu_hostdev.h"
#include "qemu_hotplug.h"
#include "qemu_monitor.h"
@@ -650,6 +651,14 @@ qemuStateInitialize(bool privileged,
if (!(qemu_driver->domains = virDomainObjListNew()))
goto error;
+/* Init domain Async QMP events */
+qemu_driver->ev_list = virQemuEventListInit();
+if (!qemu_driver->ev_list) {
+virReportSystemError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Unable to initialize QMP event queues"));
+goto error;
+}
+
/* Init domain events */
qemu_driver->domainEventState = virObjectEventStateNew();
if (!qemu_driver->domainEventState)
diff --git a/src/qemu/qemu_event.c b/src/qemu/qemu_event.c
index e27ea0d..d52fad2 100644
--- a/src/qemu/qemu_event.c
+++ b/src/qemu/qemu_event.c
@@ -73,3 +73,232 @@ virQemuEventList* virQemuEventListInit(void)
return ev_list;
}
+
+int virQemuVmEventListInit(virDomainObjPtr vm)
+{
+virQemuVmEventQueue* vmq;
+if (!vm)
+return -1;
+
+if (VIR_ALLOC(vmq) < 0)
+return -1;
+
+vmq->last = NULL;
+vmq->head = NULL;
+
+if (!virMutexInit(&vmq->lock)) {
+vm->vmq = vmq;
+return 0;
+}
+return -1;
+}
+/**
+ * virEnqueueVMEvent()
+ * Adds a new event to:
+ * - Global event queue
+ * - the event queue for this VM
+ *
+ * Return : 0 (success)
+ * -1 (failure)
+ */
+int virEnqueueVMEvent(virQemuEventList *qlist, qemuEventPtr ev)
+{
+struct _qemuGlobalEventListElement *globalEntry;
+virQemuVmEventQueue *vmq;
+struct _qemuVmEventQueueElement *vmq_entry;
+
+if (!qlist || !ev || !ev->vm || !ev->vm->vmq) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ "No queue list instantiated."
+ "Dropping event %d for Vm %s",
+ ev->ev_type, ev->vm->def->name);
+goto error;
+}
+
+if (VIR_ALLOC(globalEntry) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ "Allocation error."
+ "Dropping event %d for Vm %s",
+ ev->ev_type, ev->vm->def->name);
+goto error;
+}
+
+if (VIR_ALLOC(vmq_entry) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ "Allocation error."
+ "Dropping event %d for Vm %s",
+ ev->ev_type, ev->vm->def->name);
+free(globalEntry);
+goto error;
+}
+
+vmq_entry->ev = ev;
+vmq_entry->next = NULL;
+
+virObjectRef(ev->vm);
+globalEntry->vm = ev->vm;
+globalEntry->next = NULL;
+globalEntry->prev = NULL;
+/* Note that this order needs to be maintained
+ * for dequeue too else ABBA deadlocks will happen */
+
+/* Insert into per-Vm queue */
+vmq = ev->vm->vmq;
+
+virMutexLock(&(vmq->lock));
+if (vmq->last) {
+vmq->last->next = vmq_entry;
+
[libvirt] [[RFC] 0/8] Implement async QEMU event handling in libvirtd.
As noted in https://www.redhat.com/archives/libvir-list/2017-May/msg00016.html libvirt-QEMU driver handles all async events from the main loop. Each event handling needs the per-VM lock to make forward progress. In the case where an async event is received for the same VM which has an RPC running, the main loop is held up contending for the same lock. This impacts scalability, and should be addressed on priority. Note that libvirt does have a 2-step deferred handling for a few event categories, but (1) That is insufficient since blockign happens before the handler could disambiguate which one needs to be posted to this other queue. (2) There needs to be homogeniety. The current series builds a framework for recording and handling VM events. It initializes per-VM event queue, and a global event queue pointing to events from all the VMs. Event handling is staggered in 2 stages: - When an event is received, it is enqueued in the per-VM queue as well as the global queues. - The global queue is built into the QEMU Driver as a threadpool (currently with a single thread). - Enqueuing of a new event triggers the global event worker thread, which then attempts to take a lock for this event's VM. - If the lock is available, the event worker runs the function handling this event type. Once done, it dequeues this event from the global as well as per-VM queues. - If the lock is unavailable(ie taken by RPC thread), the event worker thread leaves this as-is and picks up the next event. - Once the RPC thread completes, it looks for events pertaining to the VM in the per-VM event queue. It then processes the events serially (holding the VM lock) until there are no more events remaining for this VM. At this point, the per-VM lock is relinquished. Patch Series status: Strictly RFC only. No compilation issues. I have not had a chance to (stress) test it after rebase to latest master. Note that documentation and test coverage is TBD, since a few open points remain. Known issues/ caveats: - RPC handling time will become non-deterministic. - An event will only be "notified" to a client once the RPC for same VM completes. - Needs careful consideration in all cases where a QMP event is used to "signal" an RPC thread, else will deadlock. Will be happy to drive more discussion in the community and completely implement it. Prerna Saxena (8): Introduce virObjectTrylock() QEMU Event handling: Introduce async event helpers in qemu_event.[ch] Setup global and per-VM event queues. Also initialize per-VM queues when libvirt reconnects to an existing VM. Events: Allow monitor to "enqueue" events to a queue. Also introduce a framework of handlers for each event type, that can be called when the handler is running an event. Events: Plumb event handling calls before a domain's APIs complete. Code refactor: Move helper functions of doCoreDump*, syncNicRxFilter*, and qemuOpenFile* to qemu_process.[ch] Fold back the 2-stage event implementation for a few events : Watchdog, Monitor EOF, Serial changed, Guest panic, Nic RX filter changed .. into single level. Initialize the per-VM event queues in context of domain init. src/Makefile.am |1 + src/conf/domain_conf.h |3 + src/libvirt_private.syms |1 + src/qemu/qemu_conf.h |4 + src/qemu/qemu_driver.c | 1710 +++ src/qemu/qemu_event.c| 317 +++ src/qemu/qemu_event.h| 231 + src/qemu/qemu_monitor.c | 592 ++-- src/qemu/qemu_monitor.h | 80 +- src/qemu/qemu_monitor_json.c | 291 +++--- src/qemu/qemu_process.c | 2031 ++ src/qemu/qemu_process.h | 88 ++ src/util/virobject.c | 26 + src/util/virobject.h |4 + src/util/virthread.c |5 + src/util/virthread.h |1 + tests/qemumonitortestutils.c |2 +- 17 files changed, 3411 insertions(+), 1976 deletions(-) create mode 100644 src/qemu/qemu_event.c create mode 100644 src/qemu/qemu_event.h -- 2.9.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Symptoms of main loop slowing down in libvirtd
On Tue, May 2, 2017 at 4:27 PM, Peter Krempa wrote:
> On Tue, May 02, 2017 at 16:16:39 +0530, Prerna wrote:
> > On Tue, May 2, 2017 at 4:07 PM, Peter Krempa wrote:
> >
> > > On Tue, May 02, 2017 at 16:01:40 +0530, Prerna wrote:
> > >
> > > [please don't top-post on technical lists]
> > >
> > > > Thanks for the quick response Peter !
> > > > This ratifies the basic approach I had in mind.
> > > > It needs some (not-so-small) cleanup of the qemu driver code, and I
> have
> > > > already started cleaning up some of it. I am planning to have a
> constant
> > > > number of event handler threads to start with. I'll try adding this
> as a
> > > > configurable parameter in qemu.conf once basic functionality is
> > > completed.
> > >
> > > That is wrong, since you can't guarantee that it will not lock up.
> Since
> > > the workers handling monitor events tend to call monitor commands
> > > themselves it's possible that it will get stuck due to unresponsive
> > > qemu. Without having a worst-case-scenario of a thread per VM you can't
> > > guarantee that the pool won't be depleted.
> > >
> >
> > Once a worker thread "picks" an event, it will contend on the per-VM lock
> > for that VM. Consequently, the handling for that event will be delayed
> > until an existing RPC call for that VM completes.
> >
> >
> > >
> > > If you want to fix this properly, you'll need a dynamic pool.
> > >
> >
> > To improve the efficiency of the thread pool, we can try contending for a
> > VM's lock for a specific time, say, N seconds, and then relinquish the
> > lock. The same thread in the pool can then move on to process events of
> the
> > next VM.
>
> This would unnecessarily delay events which are not locked.
>
> > Note that this needs all VMs to be hashed to a constant number of threads
> > in the pool, say 5. This ensures that each worker thread has a unique ,
> > non-overlapping set of VMs to work with.
>
> How would this help?
>
> > As an example, {VM_ID: 1, 6,11,16,21 ..} are handled by the same worker
> > thread. If this particular worker thread cannot find the requisite VM's
> > lock, it will move on to the event list for the next VM and so on. The
> use
> > of pthread_trylock() ensures that the worker thread will never be stuck
> > forever.
>
> No, I think this isn't the right approach at all. You could end up
> having all VM's handled with one thread, with others being idle. I think
> the right approach will be to have a dynamic pool, which will handle
> incomming events. In case when two events for a single VM should be
> handled in parallel, the same thread should pick them up in order they
> arrived. In that way, you will have at most a thread per VM, while
> normally you will have only one.
>
I agree that dynamic threadpool is helpful when there are events from
distinct VMs that need to be processed at the same time.
But I am also concerned about efficiently using the threads in this pool.
If we have a few threads only contend on per-VM locks until the RPCs for
that VM complete, it is not a very efficient use of resources. I would
rather have this thread drop handling of this VM's events and do something
useful while it is unable to grab this VM's lock.
This is the reason I wanted to load-balance incoming events by VM IDs and
hash them onto distinct threads. The idea was that a pthread always has
something else to take up if the current Vm's lock is not available. Would
you have some suggestions on improving the efficacy of the thread pool as a
whole ?
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Symptoms of main loop slowing down in libvirtd
On Tue, May 2, 2017 at 4:07 PM, Peter Krempa wrote:
> On Tue, May 02, 2017 at 16:01:40 +0530, Prerna wrote:
>
> [please don't top-post on technical lists]
>
> > Thanks for the quick response Peter !
> > This ratifies the basic approach I had in mind.
> > It needs some (not-so-small) cleanup of the qemu driver code, and I have
> > already started cleaning up some of it. I am planning to have a constant
> > number of event handler threads to start with. I'll try adding this as a
> > configurable parameter in qemu.conf once basic functionality is
> completed.
>
> That is wrong, since you can't guarantee that it will not lock up. Since
> the workers handling monitor events tend to call monitor commands
> themselves it's possible that it will get stuck due to unresponsive
> qemu. Without having a worst-case-scenario of a thread per VM you can't
> guarantee that the pool won't be depleted.
>
Once a worker thread "picks" an event, it will contend on the per-VM lock
for that VM. Consequently, the handling for that event will be delayed
until an existing RPC call for that VM completes.
>
> If you want to fix this properly, you'll need a dynamic pool.
>
To improve the efficiency of the thread pool, we can try contending for a
VM's lock for a specific time, say, N seconds, and then relinquish the
lock. The same thread in the pool can then move on to process events of the
next VM.
Note that this needs all VMs to be hashed to a constant number of threads
in the pool, say 5. This ensures that each worker thread has a unique ,
non-overlapping set of VMs to work with.
As an example, {VM_ID: 1, 6,11,16,21 ..} are handled by the same worker
thread. If this particular worker thread cannot find the requisite VM's
lock, it will move on to the event list for the next VM and so on. The use
of pthread_trylock() ensures that the worker thread will never be stuck
forever.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Symptoms of main loop slowing down in libvirtd
Thanks for the quick response Peter ! This ratifies the basic approach I had in mind. It needs some (not-so-small) cleanup of the qemu driver code, and I have already started cleaning up some of it. I am planning to have a constant number of event handler threads to start with. I'll try adding this as a configurable parameter in qemu.conf once basic functionality is completed. Thanks, Prerna On Tue, May 2, 2017 at 3:56 PM, Peter Krempa wrote: > (Dropped invalid address from cc-list) > > On Tue, May 02, 2017 at 15:33:47 +0530, Prerna wrote: > > Hi all, > > On my host, I have been seeing instances of keepalive responses slow down > > intermittently when issuing bulk power offs. > > With some tips from Danpb on the channel, I was able to trace via > systemtap > > that the main event loop would not run for about 6-9 seconds. This would > > stall keepalives and kill client connections. > > > > I was able to trace it to the fact that qemuProcessHandleEvent() needed > the > > vm lock, and this was called from the main loop. I had hook scripts that > > slightly elongated the time the power off RPC completed and the > subsequent > > keepalive delays were noticeable. > > I filed a bug about this a while ago: > > https://bugzilla.redhat.com/show_bug.cgi?id=1402921 > > > I agree that the easiest solution is to unblock the Vm lock before hook > > scripts are activated. > > However, I was wondering why we contend on the per-Vm lock directly from > > the main loop at all ? Can we do this instead : have the main loop "park" > > events to a separate event queue, and then have a dedicated thread pool > in > > the qemu driver pick these raw events and then try grabbing the per-vm > lock > > for that VM ? > > That way, we can be sure that the main event loop is _never_ delayed > > irrespective of an RPC dragging on. > > > > If this sounds reasonable I will be happy to post the driver rewrite > > patches to that end. > > And this is the solution I planed to do. Note that in worst case you > need to have one thread per VM (if all are busy), but note that the > thread pool should not be needlesly large. Requests for a single > VM need to be queued with the same thread obviously. > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] Symptoms of main loop slowing down in libvirtd
Hi all, On my host, I have been seeing instances of keepalive responses slow down intermittently when issuing bulk power offs. With some tips from Danpb on the channel, I was able to trace via systemtap that the main event loop would not run for about 6-9 seconds. This would stall keepalives and kill client connections. I was able to trace it to the fact that qemuProcessHandleEvent() needed the vm lock, and this was called from the main loop. I had hook scripts that slightly elongated the time the power off RPC completed and the subsequent keepalive delays were noticeable. I agree that the easiest solution is to unblock the Vm lock before hook scripts are activated. However, I was wondering why we contend on the per-Vm lock directly from the main loop at all ? Can we do this instead : have the main loop "park" events to a separate event queue, and then have a dedicated thread pool in the qemu driver pick these raw events and then try grabbing the per-vm lock for that VM ? That way, we can be sure that the main event loop is _never_ delayed irrespective of an RPC dragging on. If this sounds reasonable I will be happy to post the driver rewrite patches to that end. Regards, Prerna -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 0/2] Augment debug messages.
Ping ! Can someone take a look at this pls ? It is a small changeset.. On Mon, Apr 3, 2017 at 2:05 PM, Prerna Saxena wrote: > This is a v2 of the previous set of debug enhancements > posted at : https://www.redhat.com/archives/libvir-list/2017- > March/msg00959.html > > Changelog: > - > 1) Patch 1/3 : virNetSocketGetFD() is reverted to its old state, > and only new DEBUG messages are introduced. > 2) Patch 2/3 : Dropped, per list suggestions. > 3) Patch 3/3 : Same as v1 > > Prerna Saxena (2): > Debug: Add DEBUG messages for when a client has opened/closed > connection. > Debug: Report VM errors more concisely. > > src/qemu/qemu_process.c | 4 ++-- > src/rpc/virnetserverclient.c | 10 +- > 2 files changed, 11 insertions(+), 3 deletions(-) > > -- > 1.8.1.2 > > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 2/2] Debug: Report VM errors more concisely.
Current logs:
error : qemuProcessFindDomainDiskByAlias:411 : internal error: no disk found
with alias ide0-0-0
There is no way to find which VM was seeing this error.
Makes debugging very hard, and the message itself is no good.
Signed-off-by: Prerna Saxena
---
src/qemu/qemu_process.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index e450d06..0a052e8 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -367,8 +367,8 @@ qemuProcessFindDomainDiskByAlias(virDomainObjPtr vm,
}
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("no disk found with alias %s"),
- alias);
+ _("VM %s: no disk found with alias %s"),
+ vm->def->name, alias);
return NULL;
}
--
1.8.1.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 1/2] Debug: Add DEBUG messages for when a client has opened/closed connection.
While tracing connections from a remote client, it helps to keep track
of the connection lifecycle. Messages such as the following :
error : virNetSocketReadWire:1574 : End of file while reading data:
Input/output error
are rather unhelpful. They do not indicate if the client had earlier asked for
connection closure via libvirt API.
This patch introduces messages to annotate when a client connected/disconnected.
Signed-off-by: Prerna Saxena
---
src/rpc/virnetserverclient.c | 10 +-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
index 85857bc..24c9c33 100644
--- a/src/rpc/virnetserverclient.c
+++ b/src/rpc/virnetserverclient.c
@@ -964,8 +964,11 @@ void virNetServerClientClose(virNetServerClientPtr client)
virNetServerClientCloseFunc cf;
virKeepAlivePtr ka;
+VIR_DEBUG("Free'ing up resources for client=%p sock=%d", client,
+ virNetServerClientGetFD(client));
+
virObjectLock(client);
-VIR_DEBUG("client=%p", client);
+
if (!client->sock) {
virObjectUnlock(client);
return;
@@ -1039,10 +1042,14 @@ void
virNetServerClientDelayedClose(virNetServerClientPtr client)
virObjectLock(client);
client->delayedClose = true;
virObjectUnlock(client);
+VIR_DEBUG("Client=%p sock=%d requested closure of connection.",
+ client, virNetServerClientGetFD(client));
}
void virNetServerClientImmediateClose(virNetServerClientPtr client)
{
+VIR_DEBUG("Client %p sock %d closed the connection", client,
+virNetServerClientGetFD(client));
virObjectLock(client);
client->wantClose = true;
virObjectUnlock(client);
@@ -1151,6 +1158,7 @@ static void
virNetServerClientDispatchRead(virNetServerClientPtr client)
if (client->rx->nfds == 0) {
if (virNetServerClientRead(client) < 0) {
client->wantClose = true;
+VIR_WARN("Client=%p sock=%p closed connection", client,
client->sock);
return; /* Error */
}
}
--
1.8.1.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 0/2] Augment debug messages.
This is a v2 of the previous set of debug enhancements posted at : https://www.redhat.com/archives/libvir-list/2017-March/msg00959.html Changelog: - 1) Patch 1/3 : virNetSocketGetFD() is reverted to its old state, and only new DEBUG messages are introduced. 2) Patch 2/3 : Dropped, per list suggestions. 3) Patch 3/3 : Same as v1 Prerna Saxena (2): Debug: Add DEBUG messages for when a client has opened/closed connection. Debug: Report VM errors more concisely. src/qemu/qemu_process.c | 4 ++-- src/rpc/virnetserverclient.c | 10 +- 2 files changed, 11 insertions(+), 3 deletions(-) -- 1.8.1.2 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] Debug: Add WARN'ing messages for when a client has opened/closed connection.
I looked through what you were suggesting.
I was assuming virNetSocketGetFD()would do a NULL check for the sock arg,
and would return immediately if a different client executed a
virNetServerClientClose() setting client->sock to null.
Since this check is missing, I understand the implicit assumption is to
always have virNetServerClientGetFD() lock the client, and consequently, I
will rearrange debug messages around to prevent lock contention.
Sending out a V2.
On Wed, Mar 22, 2017 at 1:41 PM, Peter Krempa wrote:
> On Wed, Mar 22, 2017 at 01:02:17 -0700, Prerna Saxena wrote:
> > While tracing connections from a remote client, it helps to keep track
> > of the connection lifecycle. Messages such as the following :
> >
> > error : virNetSocketReadWire:1574 : End of file while reading data:
> Input/output error
> >
> > are rather unhelpful. They do not indicate if the client had earlier
> asked for
> > connection closure via libvirt API.
> > This patch introduces messages to annotate when a client
> connected/disconnected.
> >
> > Signed-off-by: Prerna Saxena
> > ---
> > src/rpc/virnetserverclient.c | 18 ++
> > 1 file changed, 14 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
> > index 85857bc..a77feaa 100644
> > --- a/src/rpc/virnetserverclient.c
> > +++ b/src/rpc/virnetserverclient.c
> > @@ -678,14 +678,19 @@ int
> > virNetServerClientGetTLSKeySize(virNetServerClientPtr
> client)
> > return size;
> > }
> > #endif
> > -
> > +/*
> > + * This mostly just needs to publish the client socket FD to logs.
> > + * It does not necessarily need a lock, or will add lock contention
> problems.
> > + * Replace the lock with a reference counting mechanism, to prevent the
> client
> > + * object from being deallocated while this is being run
> > + */
> > int virNetServerClientGetFD(virNetServerClientPtr client)
> > {
> > int fd = -1;
> > -virObjectLock(client);
> > +virObjectRef(client);
> > if (client->sock)
> > fd = virNetSocketGetFD(client->sock);
> > -virObjectUnlock(client);
> > +virObjectUnref(client);
>
> This change is not justified in any way. Also looks wrong. You can't
> access an unlocked object.
>
> > return fd;
> > }
> >
> > @@ -965,7 +970,9 @@ void virNetServerClientClose(virNetServerClientPtr
> client)
> > virKeepAlivePtr ka;
> >
> > virObjectLock(client);
> > -VIR_DEBUG("client=%p", client);
> > +VIR_WARN("Free'ing up resources for client=%p sock=%d", client,
> > + virNetServerClientGetFD(client));
>
> NACK using warnings instead of debug messages is not desired. For debug
> purposes you should use debug logs.
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] Debug: Remove unnecessary errors reported while parsing non-existent sysfs files.
I agree. Thanks for pointing out that this is a behavioural change, which
should not happen.
I should be doing something like this:
if (virFileReadAllQuiet(path, 1024, &buf) < 0 ) {
if (errno != ENOENT) {
virReportSystemError(errno,
_("unable to read: %s"), path);
}
goto cleanup;
}
On Wed, Mar 22, 2017 at 1:56 PM, Peter Krempa wrote:
> On Wed, Mar 22, 2017 at 09:14:41 +0100, Peter Krempa wrote:
> > On Wed, Mar 22, 2017 at 01:02:18 -0700, Prerna Saxena wrote:
> > > Sample from current logs:
> > > error : virFileReadAll:1290 : Failed to open file
> > > '/sys/class/net/tap3/operstate':
> No such file or directory
> > > error : virNetDevGetLinkInfo:1895 : unable to read:
> /sys/class/net/tap3/operstate: No such file or directory
> > >
> > > These have no useful data point and are redundant.
> > >
> > > Signed-off-by: Prerna Saxena
> > > ---
> > > src/util/virnetdev.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
> > > index d123248..3e2f962 100644
> > > --- a/src/util/virnetdev.c
> > > +++ b/src/util/virnetdev.c
> > > @@ -1874,7 +1874,7 @@ virNetDevGetLinkInfo(const char *ifname,
> > > if (virNetDevSysfsFile(&path, ifname, "operstate") < 0)
> > > goto cleanup;
> > >
> > > -if (virFileReadAll(path, 1024, &buf) < 0) {
> > > +if (virFileReadAllQuiet(path, 1024, &buf) < 0 && errno != ENOENT)
> {
> > > virReportSystemError(errno,
> > > _("unable to read: %s"),
> > > path);
> >
> > Remove this message here instead of switching to virFileReadAllQuiet.
> > virFileReadAll reports messages according to the failure itself.
>
> Hmm, So you want to avoid the error message altogether. So in that case
> you should rather call virFileAccess and also note what happens in an
> comment.
>
> Additionally since that would be a semantic change you need to mention
> it in the commit message and also make sure that all callers would
> ignore such failure. Otherwise you may cause a regression where we'd
> report an "unknown error" in cases where it was actually necessary.
>
> Since I found at least one code path that propagates the error
> (nodeDeviceGetXMLDesc) you should re-evaluate your approach.
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] Debug: Report VM errors more concisely.
Oops, sorry. Dropping the list was not intentional. I didnt realise I had
sent a "reply" in place of "reply-all" .
Adding back :)
I still would argue that having the VM's name narrows down the problem
space. If a client knows what operations have been fired for a VM over the
last time window, it is not too difficult to deduce which of them could
have caused this message to occur.
As I said before, use of libvirt Debug logs adds way too much logspew. If
we have more pointed error/warning messages, it can largely help debugging
on its own.
Even if we turn debugging on, libvirt honestly has way too many messages
that look like an en-masse CTRL-C/V operation. Sample:
All qemuMonitorEmit* except one have this message:
VIR_DEBUG("mon=%p", mon);
Even if this message shows up in logs, it is near impossible to trace which
event was seen by the VM. And it is quite another task to see which VM this
monitor belonged to.
Not that more vebosity wont help here.
However, I am only trying to make logs easier to work with. Every message
should ideally contribute to something meaningful, else it has little value
to add while debugging.
On Wed, Mar 22, 2017 at 1:59 PM, Peter Krempa wrote:
> [Please don't drop the list on the responses.]
>
> On Wed, Mar 22, 2017 at 13:52:14 +0530, Prerna wrote:
> > Always enabling debug logs adds a *lot* of logspew.
> > It would be good to have self-sufficient error messages, dont you think ?
> > What is wrong with adding a VM name field here ?
>
> It's mostly useless. You get the VM name, but you don't have the
> operation that failed. So it's useless for debugging.
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/3] Debug: Report VM errors more concisely.
Current logs:
error : qemuProcessFindDomainDiskByAlias:411 : internal error: no disk found
with alias ide0-0-0
There is no way to find which VM was seeing this error.
Makes debugging very hard, and the message itself is no good.
Signed-off-by: Prerna Saxena
---
src/qemu/qemu_process.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index ec0e36d..9f5bc3a 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -366,8 +366,8 @@ qemuProcessFindDomainDiskByAlias(virDomainObjPtr vm,
}
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("no disk found with alias %s"),
- alias);
+ _("VM %s: no disk found with alias %s"),
+ vm->def->name, alias);
return NULL;
}
--
1.8.1.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/3] Debug: Add WARN'ing messages for when a client has opened/closed connection.
While tracing connections from a remote client, it helps to keep track
of the connection lifecycle. Messages such as the following :
error : virNetSocketReadWire:1574 : End of file while reading data:
Input/output error
are rather unhelpful. They do not indicate if the client had earlier asked for
connection closure via libvirt API.
This patch introduces messages to annotate when a client connected/disconnected.
Signed-off-by: Prerna Saxena
---
src/rpc/virnetserverclient.c | 18 ++
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
index 85857bc..a77feaa 100644
--- a/src/rpc/virnetserverclient.c
+++ b/src/rpc/virnetserverclient.c
@@ -678,14 +678,19 @@ int virNetServerClientGetTLSKeySize(virNetServerClientPtr
client)
return size;
}
#endif
-
+/*
+ * This mostly just needs to publish the client socket FD to logs.
+ * It does not necessarily need a lock, or will add lock contention problems.
+ * Replace the lock with a reference counting mechanism, to prevent the client
+ * object from being deallocated while this is being run
+ */
int virNetServerClientGetFD(virNetServerClientPtr client)
{
int fd = -1;
-virObjectLock(client);
+virObjectRef(client);
if (client->sock)
fd = virNetSocketGetFD(client->sock);
-virObjectUnlock(client);
+virObjectUnref(client);
return fd;
}
@@ -965,7 +970,9 @@ void virNetServerClientClose(virNetServerClientPtr client)
virKeepAlivePtr ka;
virObjectLock(client);
-VIR_DEBUG("client=%p", client);
+VIR_WARN("Free'ing up resources for client=%p sock=%d", client,
+ virNetServerClientGetFD(client));
+
if (!client->sock) {
virObjectUnlock(client);
return;
@@ -1039,6 +1046,8 @@ void virNetServerClientDelayedClose(virNetServerClientPtr
client)
virObjectLock(client);
client->delayedClose = true;
virObjectUnlock(client);
+VIR_WARN("Client=%p sock=%d requested closure of connection.",
+ client, virNetServerClientGetFD(client));
}
void virNetServerClientImmediateClose(virNetServerClientPtr client)
@@ -1151,6 +1160,7 @@ static void
virNetServerClientDispatchRead(virNetServerClientPtr client)
if (client->rx->nfds == 0) {
if (virNetServerClientRead(client) < 0) {
client->wantClose = true;
+VIR_WARN("Client=%p sock=%p closed connection", client,
client->sock);
return; /* Error */
}
}
--
1.8.1.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/3] Debug: Remove unnecessary errors reported while parsing non-existent sysfs files.
Sample from current logs:
error : virFileReadAll:1290 : Failed to open file
'/sys/class/net/tap3/operstate': No such file or directory
error : virNetDevGetLinkInfo:1895 : unable to read:
/sys/class/net/tap3/operstate: No such file or directory
These have no useful data point and are redundant.
Signed-off-by: Prerna Saxena
---
src/util/virnetdev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index d123248..3e2f962 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -1874,7 +1874,7 @@ virNetDevGetLinkInfo(const char *ifname,
if (virNetDevSysfsFile(&path, ifname, "operstate") < 0)
goto cleanup;
-if (virFileReadAll(path, 1024, &buf) < 0) {
+if (virFileReadAllQuiet(path, 1024, &buf) < 0 && errno != ENOENT) {
virReportSystemError(errno,
_("unable to read: %s"),
path);
--
1.8.1.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/3] Debug: Improve log messages.
Libvirt logs include many snippets for debugging daemon state, but some of those messages are either missing vital information or end up logging "errors" for normal operating conditions as well. This series improves log messages, also adding additional WARN'ings for connection instantiation and closure. Prerna Saxena (3): Debug: Add WARN'ing messages for when a client has opened/closed connection. Debug: Remove unnecessary errors reported while parsing non-existent sysfs files. Debug: Report VM errors more concisely. src/qemu/qemu_process.c | 4 ++-- src/rpc/virnetserverclient.c | 18 ++ src/util/virnetdev.c | 2 +- 3 files changed, 17 insertions(+), 7 deletions(-) -- 1.8.1.2 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] Qemu : Do not distinguish a 'hangup' from an 'eof'
An errno=ECONNRESET received on a monitor socket reflects that the
guest may have closed the socket.
Today, we just mark it as a 'hangup' and do not trigger
the eof callback.
I've been looking at a slew of such messages in libvirt logs. If
the monitor socket indicated an ECONNRESET, it would possibly make sense
to mark the socket closed so that the "eof" callback may
then be invoked.
Is there a subtle corner case I'm missing here ?
Signed-off-by: Prerna Saxena
---
src/qemu/qemu_monitor.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index a5e14b2..dcafa5a 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -690,10 +690,11 @@ qemuMonitorIO(int watch, int fd, int events, void *opaque)
if (events & VIR_EVENT_HANDLE_HANGUP) {
hangup = true;
+eof = true;
if (!error) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("End of file from qemu monitor"));
-eof = true;
+
events &= ~VIR_EVENT_HANDLE_HANGUP;
}
}
--
1.8.1.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] Debug: Record the VM PID in per-VM logs.
For certain situations such as Out-of-memory scenarios, it is helpful to
record the QEMU PID in the per-VM logs, so that correlation with kernel-reported
events is easier.
While this is already recorded in libvirt daemon logs as a DEBUG message,
I believe it merits more attention :-)
Hence introducing it in per-VM log.
Signed-off-by: Prerna Saxena
---
src/qemu/qemu_process.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 4adb14e..2d3b0f5 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -4983,6 +4983,7 @@ qemuProcessLaunch(virConnectPtr conn,
int ret = -1;
int rv;
int logfile = -1;
+char *msg_buffer = NULL;
qemuDomainLogContextPtr logCtxt = NULL;
qemuDomainObjPrivatePtr priv = vm->privateData;
virCommandPtr cmd = NULL;
@@ -5095,8 +5096,14 @@ qemuProcessLaunch(virConnectPtr conn,
_("Domain %s didn't show up"), vm->def->name);
rv = -1;
}
+
VIR_DEBUG("QEMU vm=%p name=%s running with pid=%llu",
vm, vm->def->name, (unsigned long long)vm->pid);
+ignore_value(virAsprintf(&msg_buffer,
+"QEMU vm=%p name=%s running with pid=%llu",
+vm, vm->def->name, (unsigned long long)vm->pid));
+qemuLogOperation(vm, msg_buffer, NULL, logCtxt);
+virFree(msg_buffer);
} else {
VIR_DEBUG("QEMU vm=%p name=%s failed to spawn",
vm, vm->def->name);
--
1.8.1.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] tests: Fix virnetdaemontest to be skipped in non-yajl environment
Ping ! Do you want me to resend the patch including this error in the commit msg ? Regards, Prerna On Fri, Oct 23, 2015 at 9:49 PM, Prerna wrote: > Here is the more verbose failure snippet : > > ..[snip] > TEST: virnetdaemontest > 1) ExecRestart initial-nomdns... > libvirt: error : internal error: No JSON parser implementation is available > libvirt: error : internal error: No JSON parser implementation is > available > FAILED > 2) ExecRestart anon-clients ... > libvirt: error : internal error: No JSON parser implementation is available > libvirt: error : internal error: No JSON parser implementation is > available > FAILED > 3) ExecRestart anon-clients ... > libvirt: error : internal error: No JSON parser implementation is available > FAILED > 4) ExecRestart admin-nomdns ... > libvirt: error : internal error: No JSON parser implementation is available > libvirt: error : internal error: No JSON parser implementation is > available > FAILED > FAIL: virnetdaemontest > ... > > On Fri, Oct 23, 2015 at 5:54 PM, Peter Krempa wrote: > >> On Fri, Oct 23, 2015 at 15:38:25 +0530, Prerna wrote: >> > When libvirt is compiled without yajl-devel, virnetdaemontest fails. >> > This causes 'make check' to fail with nondescript errors, such as >> following: >> > [snip]... >> > PASS: read-bufsiz >> > PASS: read-non-seekable >> > PASS: start >> > TEST: virsh-uriprecedence >> > 4 OK >> > PASS: virsh-uriprecedence >> > PASS: vcpupin >> > /home/prerna/trees/libvirt/tests/virsh-all: skipping test: >> > This test is very expensive, so it is disabled by default. >> > To run it anyway, rerun: make check VIR_TEST_EXPENSIVE=1 >> > >> > SKIP: virsh-all >> > PASS: virsh-optparse >> > PASS: virsh-schedinfo >> > PASS: virsh-synopsis >> > PASS: virsh-undefine >> > === >> > 1 of 108 tests failed >> > (7 tests were not run) >> > Please report to libvir-list@redhat.com >> > === >> > make[2]: *** [check-TESTS] Error 1 >> > make[2]: Leaving directory `/home/prerna/build/libvirt/tests' >> > make[1]: *** [check-am] Error 2 >> > make[1]: Leaving directory `/home/prerna/build/libvirt/tests' >> > make: *** [check-recursive] Error 1 >> >> The above output does not contain the actual failure. Please post the >> relevant part. >> >> Peter >> > > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] tests: Fix virnetdaemontest to be skipped in non-yajl environment
Here is the more verbose failure snippet : ..[snip] TEST: virnetdaemontest 1) ExecRestart initial-nomdns... libvirt: error : internal error: No JSON parser implementation is available libvirt: error : internal error: No JSON parser implementation is available FAILED 2) ExecRestart anon-clients ... libvirt: error : internal error: No JSON parser implementation is available libvirt: error : internal error: No JSON parser implementation is available FAILED 3) ExecRestart anon-clients ... libvirt: error : internal error: No JSON parser implementation is available FAILED 4) ExecRestart admin-nomdns ... libvirt: error : internal error: No JSON parser implementation is available libvirt: error : internal error: No JSON parser implementation is available FAILED FAIL: virnetdaemontest ... On Fri, Oct 23, 2015 at 5:54 PM, Peter Krempa wrote: > On Fri, Oct 23, 2015 at 15:38:25 +0530, Prerna wrote: > > When libvirt is compiled without yajl-devel, virnetdaemontest fails. > > This causes 'make check' to fail with nondescript errors, such as > following: > > [snip]... > > PASS: read-bufsiz > > PASS: read-non-seekable > > PASS: start > > TEST: virsh-uriprecedence > > 4 OK > > PASS: virsh-uriprecedence > > PASS: vcpupin > > /home/prerna/trees/libvirt/tests/virsh-all: skipping test: > > This test is very expensive, so it is disabled by default. > > To run it anyway, rerun: make check VIR_TEST_EXPENSIVE=1 > > > > SKIP: virsh-all > > PASS: virsh-optparse > > PASS: virsh-schedinfo > > PASS: virsh-synopsis > > PASS: virsh-undefine > > === > > 1 of 108 tests failed > > (7 tests were not run) > > Please report to libvir-list@redhat.com > > === > > make[2]: *** [check-TESTS] Error 1 > > make[2]: Leaving directory `/home/prerna/build/libvirt/tests' > > make[1]: *** [check-am] Error 2 > > make[1]: Leaving directory `/home/prerna/build/libvirt/tests' > > make: *** [check-recursive] Error 1 > > The above output does not contain the actual failure. Please post the > relevant part. > > Peter > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] tests: Fix virnetdaemontest to be skipped in non-yajl environment
When libvirt is compiled without yajl-devel, virnetdaemontest fails.
This causes 'make check' to fail with nondescript errors, such as following:
[snip]...
PASS: read-bufsiz
PASS: read-non-seekable
PASS: start
TEST: virsh-uriprecedence
4 OK
PASS: virsh-uriprecedence
PASS: vcpupin
/home/prerna/trees/libvirt/tests/virsh-all: skipping test:
This test is very expensive, so it is disabled by default.
To run it anyway, rerun: make check VIR_TEST_EXPENSIVE=1
SKIP: virsh-all
PASS: virsh-optparse
PASS: virsh-schedinfo
PASS: virsh-synopsis
PASS: virsh-undefine
===
1 of 108 tests failed
(7 tests were not run)
Please report to libvir-list@redhat.com
===
make[2]: *** [check-TESTS] Error 1
make[2]: Leaving directory `/home/prerna/build/libvirt/tests'
make[1]: *** [check-am] Error 2
make[1]: Leaving directory `/home/prerna/build/libvirt/tests'
make: *** [check-recursive] Error 1
.
This patch skips virnetdaemontest in absence of yajl-devel, so that
make check gracefully runs to completion.
Signed-off-by: Prerna Saxena
---
tests/virnetdaemontest.c |5 +
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/tests/virnetdaemontest.c b/tests/virnetdaemontest.c
index 24cbd54..17d1f7f 100644
--- a/tests/virnetdaemontest.c
+++ b/tests/virnetdaemontest.c
@@ -290,6 +290,11 @@ mymain(void)
{
int ret = 0;
+#if !WITH_YAJL
+fputs("libvirt not compiled with yajl, skipping this test\n", stderr);
+return EXIT_AM_SKIP;
+#endif
+
if (virInitialize() < 0 ||
virEventRegisterDefaultImpl() < 0) {
virDispatchError(NULL);
--
1.7.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] Qemu: add CMT support
Hi Ren, Thank you for clarifying. I really do not have any more comments on the patch. Regards, Prerna On Thu, Jul 9, 2015 at 12:27 PM, Ren, Qiaowei wrote: > On Jul 7, 2015 15:51, Ren, Qiaowei wrote: > > > > > > On Jul 6, 2015 14:49, Prerna wrote: > > > >> On Sun, Jul 5, 2015 at 5:13 PM, Qiaowei Ren >> <mailto:qiaowei@intel.com> > wrote: > >> > >> > >> One RFC in > >> > https://www.redhat.com/archives/libvir-list/2015-June/msg01509.html > >> > >> CMT (Cache Monitoring Technology) can be used to measure the > >> usage of cache by VM running on the host. This patch will > >> extend the bulk stats API (virDomainListGetStats) to add this > >> field. Applications based on libvirt can use this API to achieve > >> cache usage of VM. Because CMT implementation in Linux kernel > >> is based on perf mechanism, this patch will enable perf event > >> for CMT when VM is created and disable it when VM is destroyed. > >> > >> > >> > >> > >> Hi Ren, > >> > >> One query wrt this implementation. I see you make a perf ioctl to > >> gather CMT stats each time the stats API is invoked. > >> > >> If the CMT stats are exposed by a hardware counter, then this implies > >> logging on a per-cpu (or per-socket ???) basis. > >> > >> This also implies that the value read will vary as the CPU (or socket) > >> on which it is being called changes. > >> > >> > >> Now, with this background, if we need real-world stats on a VM, we need > >> this perf ioctl executed on all CPUs/ sockets on which the VM ran. > >> Also, once done, we will need to aggregate results from each of these > >> sources. > >> > >> > >> In this implementation, I am missing this -- there seems no control > >> over which physical CPU the libvirt worker thread will run and collect > >> the perf data from. Data collected from this implementation might not > >> accurately model the system state. > >> > >> I _think_ libvirt currently has no way of directing a worker thread to > >> collect stats from a given CPU -- if we do, I would be happy to learn > >> about it :) > >> > > > > Prerna, thanks for your reply. I checked the CMT implementation in > > kernel, and noticed that the series implement new ->count() of pmu > > driver which can aggregate the results from each cpu if perf type is > > PERF_TYPE_INTEL_CQM . The following is the link for the patch: > > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?i > > d=bfe1fc d2688f557a6b6a88f59ea7619228728bd7 > > > > So I guess that this patch just need to set right perf type and > "cpu=-1". Do you > > think this is ok? > > > > Hi Prerna, > > Do you have more comments on this patch series? I would be glad to update > my implementation. ^-^ > > Qiaowei > > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] Qemu: add CMT support
On Sun, Jul 5, 2015 at 5:13 PM, Qiaowei Ren wrote: > One RFC in > https://www.redhat.com/archives/libvir-list/2015-June/msg01509.html > > CMT (Cache Monitoring Technology) can be used to measure the > usage of cache by VM running on the host. This patch will > extend the bulk stats API (virDomainListGetStats) to add this > field. Applications based on libvirt can use this API to achieve > cache usage of VM. Because CMT implementation in Linux kernel > is based on perf mechanism, this patch will enable perf event > for CMT when VM is created and disable it when VM is destroyed. > > Hi Ren, One query wrt this implementation. I see you make a perf ioctl to gather CMT stats each time the stats API is invoked. If the CMT stats are exposed by a hardware counter, then this implies logging on a per-cpu (or per-socket ???) basis. This also implies that the value read will vary as the CPU (or socket) on which it is being called changes. Now, with this background, if we need real-world stats on a VM, we need this perf ioctl executed on all CPUs/ sockets on which the VM ran. Also, once done, we will need to aggregate results from each of these sources. In this implementation, I am missing this -- there seems no control over which physical CPU the libvirt worker thread will run and collect the perf data from. Data collected from this implementation might not accurately model the system state. I _think_ libvirt currently has no way of directing a worker thread to collect stats from a given CPU -- if we do, I would be happy to learn about it :) Regards, Prerna -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Request for libvirt wiki account creation
Thanks a lot :) On Thu, Jul 2, 2015 at 9:40 PM, Daniel P. Berrange wrote: > On Thu, Jul 02, 2015 at 05:16:47PM +0530, Prerna wrote: > > Hi, > > I am interested in contributing some content to the libvirt wiki; and so > I > > need a libvirt wiki account. Could you pls create an account with > following > > credentials: > > username : prerna > > Created and replied offlist with password. > > Regards, > Daniel > -- > |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ > :| > |: http://libvirt.org -o- http://virt-manager.org > :| > |: http://autobuild.org -o- http://search.cpan.org/~danberr/ > :| > |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc > :| > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] Request for libvirt wiki account creation
Hi, I am interested in contributing some content to the libvirt wiki; and so I need a libvirt wiki account. Could you pls create an account with following credentials: username : prerna Thanks, Prerna Saxena -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 1/2] Storage: Introduce shadow vol for refresh while the main vol builds.
On Tuesday 30 June 2015 06:20 PM, Ján Tomko wrote:
> On Fri, Jun 26, 2015 at 05:05:11PM +0530, Prerna Saxena wrote:
>> Libvirt periodically refreshes all volumes in a storage pool, including
>> the volumes being cloned.
>> While cloning a storage volume from parent, we drop pool locks. Subsequent
>> volume refresh sometimes changes allocation for an ongoing copy, and leads
>> to corrupt images.
>> Fix: Introduce a shadow volume that isolates the volume object under refresh
>> from the base which has a copy ongoing.
>>
>> Signed-off-by: Prerna Saxena
>> ---
>> src/storage/storage_driver.c | 14 --
>> 1 file changed, 12 insertions(+), 2 deletions(-)
>>
> ACK
>
>> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
>> index 57060ab..41de8df 100644
>> --- a/src/storage/storage_driver.c
>> +++ b/src/storage/storage_driver.c
>> @@ -1898,7 +1898,7 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
>> {
>> virStoragePoolObjPtr pool, origpool = NULL;
>> virStorageBackendPtr backend;
>> -virStorageVolDefPtr origvol = NULL, newvol = NULL;
>> +virStorageVolDefPtr origvol = NULL, newvol = NULL, shadowvol = NULL;
>> virStorageVolPtr ret = NULL, volobj = NULL;
>> unsigned long long allocation;
>> int buildret;
>> @@ -2010,6 +2010,15 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
>> if (backend->createVol(obj->conn, pool, newvol) < 0)
>> goto cleanup;
>>
>> +/* Make a shallow copy of the 'defined' volume definition, since the
>> + * original allocation value will change as the user polls 'info',
>> + * but we only need the initial requested values
>> + */
>> +if (VIR_ALLOC(shadowvol) < 0)
>> +goto cleanup;
>> +
>> +memcpy(shadowvol, newvol, sizeof(*newvol));
>> +
>> pool->volumes.objs[pool->volumes.count++] = newvol;
>> volobj = virGetStorageVol(obj->conn, pool->def->name, newvol->name,
>>newvol->key, NULL, NULL);
>> @@ -2029,7 +2038,7 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
>> virStoragePoolObjUnlock(origpool);
>> }
>>
>> -buildret = backend->buildVolFrom(obj->conn, pool, newvol, origvol,
>> flags);
>> +buildret = backend->buildVolFrom(obj->conn, pool, shadowvol, origvol,
>> flags);
>>
> newvol->target.allocation should also use the value from shadowvol.
> I have made the adjustment and pushed the patch.
>
> Jan
Hi Jan,
Thanks.
Could you also take a look at the second patch in this series ?
Regards,
--
Prerna Saxena
Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 2/2] Storage : Fix cloning of raw, sparse volumes
When virsh vol-clone is attempted on a raw file where capacity > allocation,
the resulting cloned volume has a size that matches the virtual-size of
the parent; in place of matching its actual, disk size.
This patch fixes the cloned disk to have same _allocated_size_ as
the parent file from which it was cloned.
Ref: http://www.redhat.com/archives/libvir-list/2015-May/msg00050.html
Also fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1130739
Signed-off-by: Prerna Saxena
---
src/storage/storage_backend.c | 23 ++-
src/storage/storage_driver.c | 5 -
2 files changed, 10 insertions(+), 18 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index ce59f63..492b344 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -342,7 +342,7 @@ virStorageBackendCreateBlockFrom(virConnectPtr conn
ATTRIBUTE_UNUSED,
goto cleanup;
}
-remain = vol->target.allocation;
+remain = vol->target.capacity;
if (inputvol) {
int res = virStorageBackendCopyToFD(vol, inputvol,
@@ -397,7 +397,8 @@ createRawFile(int fd, virStorageVolDefPtr vol,
virStorageVolDefPtr inputvol,
bool reflink_copy)
{
-bool need_alloc = true;
+bool want_sparse = (inputvol &&
+ (inputvol->target.capacity > vol->target.allocation));
int ret = 0;
unsigned long long remain;
@@ -420,10 +421,9 @@ createRawFile(int fd, virStorageVolDefPtr vol,
* to writing zeroes block by block in case fallocate isn't
* available, and since we're going to copy data from another
* file it doesn't make sense to write the file twice. */
-if (vol->target.allocation) {
-if (fallocate(fd, 0, 0, vol->target.allocation) == 0) {
-need_alloc = false;
-} else if (errno != ENOSYS && errno != EOPNOTSUPP) {
+if (vol->target.allocation && !want_sparse) {
+if ((fallocate(fd, 0, 0, vol->target.allocation) != 0) &&
+ (errno != ENOSYS && errno != EOPNOTSUPP)) {
ret = -errno;
virReportSystemError(errno,
_("cannot allocate %llu bytes in file '%s'"),
@@ -433,22 +433,19 @@ createRawFile(int fd, virStorageVolDefPtr vol,
}
#endif
-remain = vol->target.allocation;
+remain = vol->target.capacity;
if (inputvol) {
/* allow zero blocks to be skipped if we've requested sparse
- * allocation (allocation < capacity) or we have already
- * been able to allocate the required space. */
-bool want_sparse = !need_alloc ||
-(vol->target.allocation < inputvol->target.capacity);
-
+ * allocation (allocation < capacity)
+ */
ret = virStorageBackendCopyToFD(vol, inputvol, fd, &remain,
want_sparse, reflink_copy);
if (ret < 0)
goto cleanup;
}
-if (remain && need_alloc) {
+if (remain && !want_sparse) {
if (safezero(fd, vol->target.allocation - remain, remain) < 0) {
ret = -errno;
virReportSystemError(errno, _("cannot fill file '%s'"),
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 41de8df..b8ed006 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -1976,11 +1976,6 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
if (newvol->target.capacity < origvol->target.capacity)
newvol->target.capacity = origvol->target.capacity;
-/* Make sure allocation is at least as large as the destination cap,
- * to make absolutely sure we copy all possible contents */
-if (newvol->target.allocation < origvol->target.capacity)
-newvol->target.allocation = origvol->target.capacity;
-
if (!backend->buildVolFrom) {
virReportError(VIR_ERR_NO_SUPPORT,
"%s", _("storage pool does not support"
--
1.8.3.1
--
Prerna Saxena
Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 1/2] Storage: Introduce shadow vol for refresh while the main vol builds.
Libvirt periodically refreshes all volumes in a storage pool, including
the volumes being cloned.
While cloning a storage volume from parent, we drop pool locks. Subsequent
volume refresh sometimes changes allocation for an ongoing copy, and leads
to corrupt images.
Fix: Introduce a shadow volume that isolates the volume object under refresh
from the base which has a copy ongoing.
Signed-off-by: Prerna Saxena
---
src/storage/storage_driver.c | 14 --
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 57060ab..41de8df 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -1898,7 +1898,7 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
{
virStoragePoolObjPtr pool, origpool = NULL;
virStorageBackendPtr backend;
-virStorageVolDefPtr origvol = NULL, newvol = NULL;
+virStorageVolDefPtr origvol = NULL, newvol = NULL, shadowvol = NULL;
virStorageVolPtr ret = NULL, volobj = NULL;
unsigned long long allocation;
int buildret;
@@ -2010,6 +2010,15 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
if (backend->createVol(obj->conn, pool, newvol) < 0)
goto cleanup;
+/* Make a shallow copy of the 'defined' volume definition, since the
+ * original allocation value will change as the user polls 'info',
+ * but we only need the initial requested values
+ */
+if (VIR_ALLOC(shadowvol) < 0)
+goto cleanup;
+
+memcpy(shadowvol, newvol, sizeof(*newvol));
+
pool->volumes.objs[pool->volumes.count++] = newvol;
volobj = virGetStorageVol(obj->conn, pool->def->name, newvol->name,
newvol->key, NULL, NULL);
@@ -2029,7 +2038,7 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
virStoragePoolObjUnlock(origpool);
}
-buildret = backend->buildVolFrom(obj->conn, pool, newvol, origvol, flags);
+buildret = backend->buildVolFrom(obj->conn, pool, shadowvol, origvol,
flags);
storageDriverLock();
virStoragePoolObjLock(pool);
@@ -2071,6 +2080,7 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
cleanup:
virObjectUnref(volobj);
virStorageVolDefFree(newvol);
+VIR_FREE(shadowvol);
if (pool)
virStoragePoolObjUnlock(pool);
if (origpool)
--
1.8.3.1
--
Prerna Saxena
Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/2] [V3] Libvirt : Storage fixes
Here is V3 of storage fixes, which addresses review comments of v2. Summary: Prerna Saxena (2): Storage: Introduce shadow vol for refresh while the main vol builds. Storage : Fix cloning of raw, sparse volumes src/storage/storage_backend.c | 23 ++- src/storage/storage_driver.c | 19 --- 2 files changed, 22 insertions(+), 20 deletions(-) V2 : http://www.redhat.com/archives/libvir-list/2015-June/msg01052.html Changelog: Modified patches 1,2 per review comments. -- 1.8.3.1 -- Prerna Saxena Linux Technology Centre, IBM Systems and Technology Lab, Bangalore, India -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] Storage : Fix cloning of raw, sparse volumes
On Tuesday 23 June 2015 06:29 PM, Ján Tomko wrote:
> On Mon, Jun 22, 2015 at 05:09:18PM +0530, Prerna Saxena wrote:
>> From: Prerna Saxena
>> Date: Mon, 22 Jun 2015 02:54:32 -0500
>>
>> When virsh vol-clone is attempted on a raw file where capacity > allocation,
>> the resulting cloned volume has a size that matches the virtual-size of
>> the parent; in place of matching its actual, disk size.
>> This patch fixes the cloned disk to have same _allocated_size_ as
>> the parent file from which it was cloned.
>>
>> Ref: http://www.redhat.com/archives/libvir-list/2015-May/msg00050.html
>>
>> Also fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1130739
>>
>> Signed-off-by: Prerna Saxena
>> ---
>> src/storage/storage_backend.c | 10 +-
>> src/storage/storage_driver.c | 5 -
>> 2 files changed, 5 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
>> index ce59f63..c99b718 100644
>> --- a/src/storage/storage_backend.c
>> +++ b/src/storage/storage_backend.c
>> @@ -342,7 +342,7 @@ virStorageBackendCreateBlockFrom(virConnectPtr conn
>> ATTRIBUTE_UNUSED,
>> goto cleanup;
>> }
>>
>> -remain = vol->target.allocation;
>> +remain = vol->target.capacity;
>>
>> if (inputvol) {
>> int res = virStorageBackendCopyToFD(vol, inputvol,
>> @@ -397,7 +397,7 @@ createRawFile(int fd, virStorageVolDefPtr vol,
>>virStorageVolDefPtr inputvol,
>>bool reflink_copy)
>> {
>> -bool need_alloc = true;
>> +bool need_alloc = !(inputvol && (inputvol->target.capacity >
>> inputvol->target.allocation));
> Comparing 'inputvol->target.capacity > vol->target.allocation' would
> allow creating a sparse volume from a non-sparse one and vice-versa
> by specifying the correct allocation.
Ok, I was not sure if libvirt wanted to do that.
If the parent volume is a sparse volume, I'd expect the cloned volume to be
sparse too. Likewise, for a non-sparse parent, the cloned volume should also be
non-sparse. Should that not be something we
honour in libvirt, when we clone ?
>> int ret = 0;
>> unsigned long long remain;
>>
>> @@ -420,7 +420,7 @@ createRawFile(int fd, virStorageVolDefPtr vol,
>> * to writing zeroes block by block in case fallocate isn't
>> * available, and since we're going to copy data from another
>> * file it doesn't make sense to write the file twice. */
>> -if (vol->target.allocation) {
>> +if (vol->target.allocation && need_alloc) {
>> if (fallocate(fd, 0, 0, vol->target.allocation) == 0) {
>> need_alloc = false;
>> } else if (errno != ENOSYS && errno != EOPNOTSUPP) {
>> @@ -433,14 +433,14 @@ createRawFile(int fd, virStorageVolDefPtr vol,
>> }
>> #endif
>>
>> -remain = vol->target.allocation;
>> +remain = vol->target.capacity;
>>
>> if (inputvol) {
>> /* allow zero blocks to be skipped if we've requested sparse
>> * allocation (allocation < capacity) or we have already
>> * been able to allocate the required space. */
>> bool want_sparse = !need_alloc ||
>> -(vol->target.allocation < inputvol->target.capacity);
>> +(inputvol->target.allocation < inputvol->target.capacity);
> If allocation < capacity, then need_alloc is already false.
>
I was trying to accomodate the original usecase of need_alloc, but you are
right. This will go away in the next version of this series, which I will post
shortly.
--
Prerna Saxena
Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] Storage: Introduce shadow vol for refresh while the main vol builds.
Hi Jan,
Thanks for the review comments.
On Tuesday 23 June 2015 06:21 PM, Ján Tomko wrote:
> On Mon, Jun 22, 2015 at 05:07:26PM +0530, Prerna Saxena wrote:
>> From: Prerna Saxena
>> Date: Thu, 18 Jun 2015 05:05:09 -0500
>>
>> Libvirt periodically refreshes all volumes in a storage pool, including
>> the volumes being cloned.
>> While cloning a storage volume from parent, we drop pool locks. Subsequent
>> volume refresh sometimes changes allocation for an ongoing copy, and leads
>> to corrupt images.
>> Fix: Introduce a shadow volume that isolates the volume object under refresh
>> from the base which has a copy ongoing.
>>
>> Signed-off-by: Prerna Saxena
>> ---
>> src/storage/storage_driver.c | 15 +--
>> 1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
>> index 57060ab..4980546 100644
>> --- a/src/storage/storage_driver.c
>> +++ b/src/storage/storage_driver.c
>> @@ -1898,7 +1898,7 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
>> {
>> virStoragePoolObjPtr pool, origpool = NULL;
>> virStorageBackendPtr backend;
>> -virStorageVolDefPtr origvol = NULL, newvol = NULL;
>> +virStorageVolDefPtr origvol = NULL, newvol = NULL, shadowvol = NULL;
>> virStorageVolPtr ret = NULL, volobj = NULL;
>> unsigned long long allocation;
>> int buildret;
>> @@ -2010,6 +2010,17 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
>> if (backend->createVol(obj->conn, pool, newvol) < 0)
>> goto cleanup;
>>
>> +/* Make a shallow copy of the 'defined' volume definition, since the
>> + * original allocation value will change as the user polls 'info',
>> + * but we only need the initial requested values
>> + */
>> +if (VIR_ALLOC(shadowvol) < 0) {
>> +newvol = NULL;
> newvol has not been added to pool->volumes.objs yet, so we should free
> it on the cleanup path. shadowvol should also be VIR_FREE'd.
Thanks, I'd missed this. Will be addressed in subsequent patch.
>> +goto cleanup;
>> +}
>> +
>> +memcpy(shadowvol, newvol, sizeof(*newvol));
>> +
>> pool->volumes.objs[pool->volumes.count++] = newvol;
>> volobj = virGetStorageVol(obj->conn, pool->def->name, newvol->name,
>>newvol->key, NULL, NULL);
>> @@ -2029,7 +2040,7 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
>> virStoragePoolObjUnlock(origpool);
>> }
>>
>> -buildret = backend->buildVolFrom(obj->conn, pool, newvol, origvol,
>> flags);
>> +buildret = backend->buildVolFrom(obj->conn, pool, shadowvol, origvol,
>> flags);
>>
> A few lines below, there is one more usage of newvol after the pool has
> been unlocked: newvol->target.allocation
>
> If the parallel volume refresh happened when the volume was not fully
> allocated, this might not contain the intended allocation.
>
> Using shadowvol->target.allocation will behave the same regardless of a
> parallel refresh (even though the buildVolFrom function might not honor
> the allocation exactly).
>
>
Right. The buildVolFrom() call completes an fsync and then returns. In my
experimental runs, a parallel refresh would always happen by the time I got to
this point; so this had missed me. But ofcourse
we can never say that for sure. Will fix in the next patch.
--
Prerna Saxena
Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/2] Storage : Fix cloning of raw, sparse volumes
From: Prerna Saxena
Date: Mon, 22 Jun 2015 02:54:32 -0500
When virsh vol-clone is attempted on a raw file where capacity > allocation,
the resulting cloned volume has a size that matches the virtual-size of
the parent; in place of matching its actual, disk size.
This patch fixes the cloned disk to have same _allocated_size_ as
the parent file from which it was cloned.
Ref: http://www.redhat.com/archives/libvir-list/2015-May/msg00050.html
Also fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1130739
Signed-off-by: Prerna Saxena
---
src/storage/storage_backend.c | 10 +-
src/storage/storage_driver.c | 5 -
2 files changed, 5 insertions(+), 10 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index ce59f63..c99b718 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -342,7 +342,7 @@ virStorageBackendCreateBlockFrom(virConnectPtr conn
ATTRIBUTE_UNUSED,
goto cleanup;
}
-remain = vol->target.allocation;
+remain = vol->target.capacity;
if (inputvol) {
int res = virStorageBackendCopyToFD(vol, inputvol,
@@ -397,7 +397,7 @@ createRawFile(int fd, virStorageVolDefPtr vol,
virStorageVolDefPtr inputvol,
bool reflink_copy)
{
-bool need_alloc = true;
+bool need_alloc = !(inputvol && (inputvol->target.capacity >
inputvol->target.allocation));
int ret = 0;
unsigned long long remain;
@@ -420,7 +420,7 @@ createRawFile(int fd, virStorageVolDefPtr vol,
* to writing zeroes block by block in case fallocate isn't
* available, and since we're going to copy data from another
* file it doesn't make sense to write the file twice. */
-if (vol->target.allocation) {
+if (vol->target.allocation && need_alloc) {
if (fallocate(fd, 0, 0, vol->target.allocation) == 0) {
need_alloc = false;
} else if (errno != ENOSYS && errno != EOPNOTSUPP) {
@@ -433,14 +433,14 @@ createRawFile(int fd, virStorageVolDefPtr vol,
}
#endif
-remain = vol->target.allocation;
+remain = vol->target.capacity;
if (inputvol) {
/* allow zero blocks to be skipped if we've requested sparse
* allocation (allocation < capacity) or we have already
* been able to allocate the required space. */
bool want_sparse = !need_alloc ||
-(vol->target.allocation < inputvol->target.capacity);
+(inputvol->target.allocation < inputvol->target.capacity);
ret = virStorageBackendCopyToFD(vol, inputvol, fd, &remain,
want_sparse, reflink_copy);
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 4980546..c1dfe89 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -1976,11 +1976,6 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
if (newvol->target.capacity < origvol->target.capacity)
newvol->target.capacity = origvol->target.capacity;
-/* Make sure allocation is at least as large as the destination cap,
- * to make absolutely sure we copy all possible contents */
-if (newvol->target.allocation < origvol->target.capacity)
-newvol->target.allocation = origvol->target.capacity;
-
if (!backend->buildVolFrom) {
virReportError(VIR_ERR_NO_SUPPORT,
"%s", _("storage pool does not support"
--
1.8.3.1
--
Prerna Saxena
Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/2] Storage: Introduce shadow vol for refresh while the main vol builds.
From: Prerna Saxena
Date: Thu, 18 Jun 2015 05:05:09 -0500
Libvirt periodically refreshes all volumes in a storage pool, including
the volumes being cloned.
While cloning a storage volume from parent, we drop pool locks. Subsequent
volume refresh sometimes changes allocation for an ongoing copy, and leads
to corrupt images.
Fix: Introduce a shadow volume that isolates the volume object under refresh
from the base which has a copy ongoing.
Signed-off-by: Prerna Saxena
---
src/storage/storage_driver.c | 15 +--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 57060ab..4980546 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -1898,7 +1898,7 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
{
virStoragePoolObjPtr pool, origpool = NULL;
virStorageBackendPtr backend;
-virStorageVolDefPtr origvol = NULL, newvol = NULL;
+virStorageVolDefPtr origvol = NULL, newvol = NULL, shadowvol = NULL;
virStorageVolPtr ret = NULL, volobj = NULL;
unsigned long long allocation;
int buildret;
@@ -2010,6 +2010,17 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
if (backend->createVol(obj->conn, pool, newvol) < 0)
goto cleanup;
+/* Make a shallow copy of the 'defined' volume definition, since the
+ * original allocation value will change as the user polls 'info',
+ * but we only need the initial requested values
+ */
+if (VIR_ALLOC(shadowvol) < 0) {
+newvol = NULL;
+goto cleanup;
+}
+
+memcpy(shadowvol, newvol, sizeof(*newvol));
+
pool->volumes.objs[pool->volumes.count++] = newvol;
volobj = virGetStorageVol(obj->conn, pool->def->name, newvol->name,
newvol->key, NULL, NULL);
@@ -2029,7 +2040,7 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
virStoragePoolObjUnlock(origpool);
}
-buildret = backend->buildVolFrom(obj->conn, pool, newvol, origvol, flags);
+buildret = backend->buildVolFrom(obj->conn, pool, shadowvol, origvol,
flags);
storageDriverLock();
virStoragePoolObjLock(pool);
--
1.8.3.1
--
Prerna Saxena
Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 0/2] Storage fixes for libvirt.
From: Prerna Saxena Date: Mon, 22 Jun 2015 06:12:24 -0500 This is the second version of storage fixes. V1 : http://www.redhat.com/archives/libvir-list/2015-May/msg00050.html Summary: Prerna Saxena (2): Storage: Introduce shadow vol for refresh while the main vol builds. Storage : Fix cloning of raw, sparse volumes src/storage/storage_backend.c | 10 +- src/storage/storage_driver.c | 20 +--- 2 files changed, 18 insertions(+), 12 deletions(-) Changelog: -- 1. Dropped patch 1/2 of v1 as per jtomko's suggestion; and introduced a new patch for shadow-volume based cloning. 2. Reworked patch 2/2 incorporating review comments. -- Regards, -- Prerna Saxena Linux Technology Centre, IBM Systems and Technology Lab, Bangalore, India -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] Storage: Suppress metadata refresh for volumes being built.
On Tuesday 05 May 2015 04:22 PM, Ján Tomko wrote:
> On Tue, May 05, 2015 at 03:24:31PM +0530, Prerna Saxena wrote:
>> On Tuesday 05 May 2015 03:20 PM, Prerna Saxena wrote:
>>> On Tuesday 05 May 2015 01:52 PM, Ján Tomko wrote:
>>>> On Tue, May 05, 2015 at 08:43:21AM +0530, Prerna Saxena wrote:
>>>>> Libvirt periodically calls 'stat' on all volumes in a storage pool,
>>>>> to update fields such as 'target.allocation'.
>>>>>
>>>>> The operation doesnt make sense for a volume which is curently being
>>>>> allocated.
>>>> From the comments in the storage driver, the point of allowing refresh
>>>> for a volume that is currently being allocated is to track the progress
>>>> of the allocation.
>>>>
>>>>> Also, the 'target.allocation' sub-field is taken into account while
>>>>> copying a raw image.
>>>>> To suppress any (potential) corruption, libvirt must not attempt to
>>>>> refresh a volume currently being built.
>>>> What would be the corruption?
>>>>
>>>> We do not allow using a volume that is currently building as a
>>>> source for cloning the volume - storageVolCreateXMLFrom checks for
>>>> origvol->building:
>>>>
>>>> if (origvol->building) {
>>>> virReportError(VIR_ERR_OPERATION_INVALID,
>>>>_("volume '%s' is still being allocated."),
>>>>origvol->name);
>>>> goto cleanup;
>>>> }
>>>>
>>> While running libvirt on PowerPC, I saw an interesting scenario. The
>>> 'target.allocation' field seemed to change for a volume getting allocated,
>>> and this would lead to incomplete copy. This would
>>> happen at random intervals, not deterministically. While looking through
>>> the code, I found this to be the other place in code where the same field
>>> seemed to change without a lock. Hence the patch.
>>>
> Oh, I was thinking of the soure volume for some reason.
>
> We correctly lock the pool before calling refreshVol, so changing the
> object should not be an issue.
> I think the bug is in storageVolCreateXMLFrom - it drops all the locks,
> but expects the allocation not to change.
Yes, and so I sent this patch that blocks a refresh for volumes being created.
> In storageVolCreateXML we work around this by creating a shallow copy of
> the volume.
>
>>> I have sent the second patch which fixes the erring code too :
>>>
>>> -remain = vol->target.allocation;
>>> +remain = inputvol->target.capacity;
>>>
>> More fundamental question -- why do we offload the copying of non-raw images
>> to qemu-img tool, but make libvirt responsible for copying raw volumes ?
>> Would it not be better if libvirt called on 'qemu-img' to copy all types of
>> volumes, including raw ones ?
>>
> This way, libvirt can create raw volumes even without qemu-img
> installed. I don't know if there's any other reason.
>
I'm sorry, libvirt does not copy raw volumes as a 'fallback mechanism'.
Libvirt chooses to copy raw volumes on its own, but calls on qemu-img to copy
all other volume types.
Is it not better to let qemu-img copy all volume types , including raw ones ?
I wanted to check if there are reasons for this decision ? I'll be happy if the
community could throw some light.
Also cc'ing Cole, who had put in some fixes in this area.
Regards,
--
Prerna Saxena
Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] Storage: Suppress metadata refresh for volumes being built.
On Tuesday 05 May 2015 03:20 PM, Prerna Saxena wrote:
> On Tuesday 05 May 2015 01:52 PM, Ján Tomko wrote:
>> On Tue, May 05, 2015 at 08:43:21AM +0530, Prerna Saxena wrote:
>>> Libvirt periodically calls 'stat' on all volumes in a storage pool,
>>> to update fields such as 'target.allocation'.
>>>
>>> The operation doesnt make sense for a volume which is curently being
>>> allocated.
>> From the comments in the storage driver, the point of allowing refresh
>> for a volume that is currently being allocated is to track the progress
>> of the allocation.
>>
>>> Also, the 'target.allocation' sub-field is taken into account while copying
>>> a raw image.
>>> To suppress any (potential) corruption, libvirt must not attempt to refresh
>>> a volume currently being built.
>> What would be the corruption?
>>
>> We do not allow using a volume that is currently building as a
>> source for cloning the volume - storageVolCreateXMLFrom checks for
>> origvol->building:
>>
>> if (origvol->building) {
>> virReportError(VIR_ERR_OPERATION_INVALID,
>>_("volume '%s' is still being allocated."),
>>origvol->name);
>> goto cleanup;
>> }
>>
> While running libvirt on PowerPC, I saw an interesting scenario. The
> 'target.allocation' field seemed to change for a volume getting allocated,
> and this would lead to incomplete copy. This would
> happen at random intervals, not deterministically. While looking through the
> code, I found this to be the other place in code where the same field seemed
> to change without a lock. Hence the patch.
>
> I have sent the second patch which fixes the erring code too :
>
> -remain = vol->target.allocation;
> +remain = inputvol->target.capacity;
>
More fundamental question -- why do we offload the copying of non-raw images to
qemu-img tool, but make libvirt responsible for copying raw volumes ?
Would it not be better if libvirt called on 'qemu-img' to copy all types of
volumes, including raw ones ?
--
Prerna Saxena
Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] Storage: Suppress metadata refresh for volumes being built.
On Tuesday 05 May 2015 01:52 PM, Ján Tomko wrote:
> On Tue, May 05, 2015 at 08:43:21AM +0530, Prerna Saxena wrote:
>> Libvirt periodically calls 'stat' on all volumes in a storage pool,
>> to update fields such as 'target.allocation'.
>>
>> The operation doesnt make sense for a volume which is curently being
>> allocated.
> From the comments in the storage driver, the point of allowing refresh
> for a volume that is currently being allocated is to track the progress
> of the allocation.
>
>> Also, the 'target.allocation' sub-field is taken into account while copying
>> a raw image.
>> To suppress any (potential) corruption, libvirt must not attempt to refresh
>> a volume currently being built.
> What would be the corruption?
>
> We do not allow using a volume that is currently building as a
> source for cloning the volume - storageVolCreateXMLFrom checks for
> origvol->building:
>
> if (origvol->building) {
> virReportError(VIR_ERR_OPERATION_INVALID,
>_("volume '%s' is still being allocated."),
>origvol->name);
> goto cleanup;
> }
>
While running libvirt on PowerPC, I saw an interesting scenario. The
'target.allocation' field seemed to change for a volume getting allocated, and
this would lead to incomplete copy. This would
happen at random intervals, not deterministically. While looking through the
code, I found this to be the other place in code where the same field seemed to
change without a lock. Hence the patch.
I have sent the second patch which fixes the erring code too :
-remain = vol->target.allocation;
+remain = inputvol->target.capacity;
Regards,
--
Prerna Saxena
Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/2] Storage : Fix cloning of raw, sparse volumes.
When virsh vol-clone is attempted on a raw file where capacity > allocation,
the resulting cloned volume has a size that matches the virtual-size of
the parent; in place of matching its actual, disk size.
This patch fixes the cloned disk to have same _allocated_size_ as
the parent file from which it was cloned.
Reference:
https://www.redhat.com/archives/libvir-list/2014-September/msg00064.html
Also fixes : https://bugzilla.redhat.com/show_bug.cgi?id=1130739
Signed-off-by: Prerna Saxena
---
src/storage/storage_backend.c | 2 +-
src/storage/storage_driver.c | 5 -
2 files changed, 1 insertion(+), 6 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 355fc7f..1a7c0cc 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -429,7 +429,7 @@ createRawFile(int fd, virStorageVolDefPtr vol,
}
#endif
-remain = vol->target.allocation;
+remain = inputvol->target.capacity;
if (inputvol) {
/* allow zero blocks to be skipped if we've requested sparse
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index ac4a74a..c511035 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -1990,11 +1990,6 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
if (newvol->target.capacity < origvol->target.capacity)
newvol->target.capacity = origvol->target.capacity;
-/* Make sure allocation is at least as large as the destination cap,
- * to make absolutely sure we copy all possible contents */
-if (newvol->target.allocation < origvol->target.capacity)
-newvol->target.allocation = origvol->target.capacity;
-
if (!backend->buildVolFrom) {
virReportError(VIR_ERR_NO_SUPPORT,
"%s", _("storage pool does not support"
--
1.8.3.1
--
Prerna Saxena
Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/2] Storage: Suppress metadata refresh for volumes being built.
Libvirt periodically calls 'stat' on all volumes in a storage pool,
to update fields such as 'target.allocation'.
The operation doesnt make sense for a volume which is curently being allocated.
Also, the 'target.allocation' sub-field is taken into account while copying a
raw image. To suppress any (potential) corruption, libvirt must not attempt to
refresh a volume currently being built.
Signed-off-by: Prerna Saxena
---
src/storage/storage_backend.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 289f454..355fc7f 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -1576,6 +1576,9 @@ virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol,
{
int ret;
+if (vol->building)
+return 0;
+
if ((ret = virStorageBackendUpdateVolTargetInfo(&vol->target,
withBlockVolFormat,
openflags)) < 0)
--
1.8.3.1
--
Prerna Saxena
Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/2] Storage : Fixes for cloning raw volumes
From: Prerna Saxena Date: Mon, 4 May 2015 12:00:46 -0700 This series has some long-overdue fixes for copying of raw storage volumes with libvirt. Prerna Saxena (2): Storage : Suppress metadata refresh for volumes being built. Storage : Fix cloning of raw, sparse volumes. src/storage/storage_backend.c | 5 - src/storage/storage_driver.c | 5 - 2 files changed, 4 insertions(+), 6 deletions(-) -- 1.8.3.1 -- Prerna Saxena Linux Technology Centre, IBM Systems and Technology Lab, Bangalore, India -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] schema: Allow multiple machines for sparc VMs
On Monday 13 April 2015 07:50 PM, Daniel P. Berrange wrote: > On Mon, Apr 13, 2015 at 04:14:53PM +0200, Martin Kletzander wrote: >> Use the same pattern as there is for x86 machines. >> >> Signed-off-by: Martin Kletzander >> --- >> docs/schemas/domaincommon.rng | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng >> index 03fd541..80b30df 100644 >> --- a/docs/schemas/domaincommon.rng >> +++ b/docs/schemas/domaincommon.rng >> @@ -384,7 +384,9 @@ >> >> >> >> - sun4m >> + >> +[a-zA-Z0-9_\.\-]+ >> + >> >> >> > I think you could probably simplify this all much more. All these > architecture specific blocks of machine type names should just be > deleted and so this: > > > > > > > > > > > > > > > > hvm > > > > Would simplify to just > > > > > > > i686 > others... > > > > > > > [a-zA-Z0-9_\.\-]+ > > > > > Hi Martin, Daniel, I have not been able to try this patch, it fails with this error : error: internal error: Unable to parse RNG /test-libvirt/share/libvirt/schemas/domain.rng: Reference osexe has no matching definition Internal found no define for ref osexe However, had some concerns purely by looking at this patch. This change is very x86-centric, it does not respect other architectures. I think the rationale for simplifying domaincommon.rng would have been to group all types that obey this pattern string: [a-zA-Z0-9_\.\-]+ However, this regex does not conform to machine types for _all_ architectures. As an example, see this : s390 s390x s390 s390-virtio s390-ccw s390-ccw-virtio The s390 arch only allows four machine names : "s390", "s390-virtio", "s390-ccw", "s390-ccw-virtio". With the patch you suggest, even a string such as "abcdefg" will become a legitimate machine type for s390x, which seems like an odd thing. Likewise, ppc64[le] architecture allows only strings such as pseries, pseries-2.1, pseries-2.2 .. This patch will allow any random machine name, which seems somewhat odd to me. Regards, -- Prerna Saxena Linux Technology Centre, IBM Systems and Technology Lab, Bangalore, India -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] PowerPC: Replace hardcoded values of 'pseries-2*' machine in schema
From: Prerna Saxena Date: Mon, 6 Apr 2015 22:56:25 -0500 Qemu-system-ppc64 has introduced new variants of pseries machine type, such as 'pseries', 'pseries-2.1'..'pseries-2.3'. Replace the hardcoded values in the schema with a regex that captures this. Signed-off-by: Prerna Saxena --- docs/schemas/domaincommon.rng | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 7406d92..a192637 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -421,9 +421,9 @@ -pseries -pseries-2.1 -pseries-2.2 + + (pseries)(\-2\.[1-9])? + -- 1.8.3.1 -- Prerna Saxena Linux Technology Centre, IBM Systems and Technology Lab, Bangalore, India -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2.5 00/10] Add support for memory hotplug
Hi Peter, While playing around with memory hotplug implementation, I found that the guest XML isnt updated after a successful hotplug operation : [root@kop2 test-libvirt]# ./bin/virsh attach-device rhel71-be /home/prerna/mem-hp.xml Device attached successfully [root@kop2 test-libvirt]# ./bin/virsh qemu-monitor-command rhel71-be --hmp "info memdev" memory backend: 0 size: 1073741824 merge: true dump: true prealloc: false policy: default host nodes: [root@kop2 test-libvirt]# ./bin/virsh edit rhel71-be < Does not reflect an updated memory value or a separate dimm device > I have observed this on PowerKVM. Wondering what is a good place to observe the hot-added memory : Should this reflect as an updated value for current memory in domain XML, or should this show up as a memory device of type: 4096 1-3 524287 1 I would be happy to work on a patch to fix this. Pls provide me your thoughts. Regards, -- Prerna Saxena Linux Technology Centre, IBM Systems and Technology Lab, Bangalore, India -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] PowerPC : Do not allow an empty model spec for 'host-model'
On Friday 20 March 2015 01:51 PM, Ján Tomko wrote: > On Mon, Mar 16, 2015 at 04:56:49PM +0530, Prerna Saxena wrote: >> [PATCH] PowerPC : Do not allow an empty model spec for 'host-model' >> >> On PowerPC, a guest VM having CPU mode as 'host-model' >> represents a 'compat' mode VM. This cannot have a NULL >> CPU model. > I thought the compat= mode was only used when mode == HOST_MODEL > and a model is specified. And HOST_MODEL with no model behaves like on > x86_64 - copies the features from the host capabilities. > > Was this functionality broken by commit addce06 or did it never produce > useful results? > > Jan > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list Hi Jan, This commit does not break anything. It addresses a few corner cases not completely addressed by commits addce06 & 5e4f49ab8aa2. PowerPC pseries KVM is a paravirtualized platform, wherein there are only 2 allowed vcpu configurations of running a guest : 1) Native mode, where the guest sees the same vcpu model as the host -- this is reflected in libvirt by "host-passthrough" mode; 2) Compat mode, where the physical processor itself runs in binary compatibility with an older cpu model. This is marked in libvirt by "host-model" mode, which takes on an additional argument -- the guest CPU model which needs to be run. This was introduced by commit addce06. PowerKVM, being a paravirt platform, does not emulate a guest vcpu based on features copied from host. This behaviour is unlike x86 KVM. Hence , the host-model mode on PowerKVM needs to error out in case the model which needs to be run in binary compatibility is not specified by user. The reason for this commit was a bug seen even after 5e4f49ab8aa2. A null cpu model XML was causing an incorrect "best-fit" model (just like x86) to be passed to the VM after a save/restore. Hence the need for this check. Regards, -- Prerna Saxena Linux Technology Centre, IBM Systems and Technology Lab, Bangalore, India -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] PowerPC : Do not allow an empty model spec for 'host-model'
[PATCH] PowerPC : Do not allow an empty model spec for 'host-model'
On PowerPC, a guest VM having CPU mode as 'host-model'
represents a 'compat' mode VM. This cannot have a NULL
CPU model.
This commit forbids such a guest definition.
Signed-off-by: Prerna Saxena
---
src/cpu/cpu_powerpc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/cpu/cpu_powerpc.c b/src/cpu/cpu_powerpc.c
index c77374c..62ad92b 100644
--- a/src/cpu/cpu_powerpc.c
+++ b/src/cpu/cpu_powerpc.c
@@ -549,12 +549,12 @@ ppcUpdate(virCPUDefPtr guest,
const virCPUDef *host)
{
switch ((virCPUMode) guest->mode) {
-case VIR_CPU_MODE_HOST_MODEL:
case VIR_CPU_MODE_HOST_PASSTHROUGH:
guest->match = VIR_CPU_MATCH_EXACT;
virCPUDefFreeModel(guest);
return virCPUDefCopyModel(guest, host, true);
+case VIR_CPU_MODE_HOST_MODEL:
case VIR_CPU_MODE_CUSTOM:
return 0;
--
1.8.3.1
--
Prerna Saxena
Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] Tests : Add test for 'ppc64le' architecture.
On Wednesday 04 March 2015 09:28 PM, Ján Tomko wrote: > On Thu, Feb 26, 2015 at 10:45:54PM +0530, Prerna Saxena wrote: >> Tests : Add test for 'ppc64le' architecture. >> >> Signed-off-by: Prerna Saxena >> --- >> .../qemuxml2argv-pseries-cpu-le.args | 7 + >> .../qemuxml2argv-pseries-cpu-le.xml| 17 >> tests/qemuxml2argvtest.c | 2 ++ >> tests/testutilsqemu.c | 30 >> ++ >> 4 files changed, 56 insertions(+) >> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-cpu-le.args >> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-cpu-le.xml >> >> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-cpu-le.args >> b/tests/qemuxml2argvdata/qemuxml2argv-pseries-cpu-le.args >> new file mode 100644 >> index 000..f4f8d5e >> --- /dev/null >> +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-cpu-le.args >> @@ -0,0 +1,7 @@ >> +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test \ >> +QEMU_AUDIO_DRV=none /usr/bin/qemu-system-ppc64 -S -M pseries \ >> +-m 512 -smp 1 -nographic -nodefconfig -nodefaults \ >> +-chardev socket,id=charmonitor,path=/tmp/test-monitor,server,nowait \ >> +-mon chardev=charmonitor,id=monitor,mode=readline -no-acpi -boot c -usb \ >> +-chardev pty,id=charserial0 \ >> +-device spapr-vty,chardev=charserial0,reg=0x3000 > I don't see any reference to little endian on the command line. > > If the domain can only use the same endianness as the host system, do we > need to check if the guest arch matches the host arch somewhere? > > Yes, SLOF in qemu-system-ppc64 can correctly initialize KVM based on the endianness discovered for the guest, so the command line stays the same for both ppc64 & ppc64le guests. However, we need libvirt to recognize ppc64le as a valid arch, since this is used by management utilities that depend on libvirt -- a classic example being virt-install. Regards, -- Prerna Saxena Linux Technology Centre, IBM Systems and Technology Lab, Bangalore, India -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/2] PowerPC : Miscellaneous fixes for 'ppc64le' architecture.
On Tuesday 03 March 2015 03:29 PM, Michal Privoznik wrote: > On 26.02.2015 18:09, Prerna Saxena wrote: >> >From a28ef5a3e7b9cb023948cf97d9f472bb3a1e06d3 Mon Sep 17 00:00:00 2001 >> From: Prerna Saxena >> Date: Thu, 26 Feb 2015 22:31:05 +0530 >> >> This series adds few miscellaneous fixes for PowerPC 64-bit Little >> Endian Architecture. >> >> Changelog : >> == >> v1 of Patch 1/2 already posted and acked : >> https://www.redhat.com/archives/libvir-list/2015-February/msg00486.html >> Patch 2/2 adds a testcase to supplement patch 1. >> >> Prerna Saxena (2): >> PowerPC: Augment XML schema to include 'ppc64le' arch and newer >> pseries-2.* machine types. >> Tests : Add test for 'ppc64le' architecture. >> >> docs/schemas/basictypes.rng| 1 + >> docs/schemas/domaincommon.rng | 7 - >> .../qemuxml2argv-pseries-cpu-le.args | 7 + >> .../qemuxml2argv-pseries-cpu-le.xml| 17 >> tests/qemuxml2argvtest.c | 2 ++ >> tests/testutilsqemu.c | 30 >> ++ >> 6 files changed, 63 insertions(+), 1 deletion(-) >> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-cpu-le.args >> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-cpu-le.xml >> > I've tweaked the commit messages a bit, ACKed and pushed. > > Michal > Thank you :-) -- Prerna Saxena Linux Technology Centre, IBM Systems and Technology Lab, Bangalore, India -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] Fwd: [PATCH 0/2] PowerPC : Miscellaneous fixes for 'ppc64le' architecture.
Ping !` Can you pls let me know if this suffices ? Regards, Prerna Forwarded Message Subject:[libvirt] [PATCH 0/2] PowerPC : Miscellaneous fixes for 'ppc64le' architecture. Date: Thu, 26 Feb 2015 22:39:20 +0530 From: Prerna Saxena To: libvirt mailing list CC: Ján Tomko >From a28ef5a3e7b9cb023948cf97d9f472bb3a1e06d3 Mon Sep 17 00:00:00 2001 From: Prerna Saxena Date: Thu, 26 Feb 2015 22:31:05 +0530 This series adds few miscellaneous fixes for PowerPC 64-bit Little Endian Architecture. Changelog : == v1 of Patch 1/2 already posted and acked : https://www.redhat.com/archives/libvir-list/2015-February/msg00486.html Patch 2/2 adds a testcase to supplement patch 1. Prerna Saxena (2): PowerPC: Augment XML schema to include 'ppc64le' arch and newer pseries-2.* machine types. Tests : Add test for 'ppc64le' architecture. docs/schemas/basictypes.rng| 1 + docs/schemas/domaincommon.rng | 7 - .../qemuxml2argv-pseries-cpu-le.args | 7 + .../qemuxml2argv-pseries-cpu-le.xml| 17 tests/qemuxml2argvtest.c | 2 ++ tests/testutilsqemu.c | 30 ++ 6 files changed, 63 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-cpu-le.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-cpu-le.xml -- 1.9.3 -- Prerna Saxena Linux Technology Centre, IBM Systems and Technology Lab, Bangalore, India -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/2] PowerPC : Miscellaneous fixes for 'ppc64le' architecture.
>From a28ef5a3e7b9cb023948cf97d9f472bb3a1e06d3 Mon Sep 17 00:00:00 2001 From: Prerna Saxena Date: Thu, 26 Feb 2015 22:31:05 +0530 This series adds few miscellaneous fixes for PowerPC 64-bit Little Endian Architecture. Changelog : == v1 of Patch 1/2 already posted and acked : https://www.redhat.com/archives/libvir-list/2015-February/msg00486.html Patch 2/2 adds a testcase to supplement patch 1. Prerna Saxena (2): PowerPC: Augment XML schema to include 'ppc64le' arch and newer pseries-2.* machine types. Tests : Add test for 'ppc64le' architecture. docs/schemas/basictypes.rng| 1 + docs/schemas/domaincommon.rng | 7 - .../qemuxml2argv-pseries-cpu-le.args | 7 + .../qemuxml2argv-pseries-cpu-le.xml| 17 tests/qemuxml2argvtest.c | 2 ++ tests/testutilsqemu.c | 30 ++ 6 files changed, 63 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-cpu-le.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-cpu-le.xml -- 1.9.3 -- Prerna Saxena Linux Technology Centre, IBM Systems and Technology Lab, Bangalore, India -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/2] Tests : Add test for 'ppc64le' architecture.
Tests : Add test for 'ppc64le' architecture.
Signed-off-by: Prerna Saxena
---
.../qemuxml2argv-pseries-cpu-le.args | 7 +
.../qemuxml2argv-pseries-cpu-le.xml| 17
tests/qemuxml2argvtest.c | 2 ++
tests/testutilsqemu.c | 30 ++
4 files changed, 56 insertions(+)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-cpu-le.args
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-cpu-le.xml
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-cpu-le.args
b/tests/qemuxml2argvdata/qemuxml2argv-pseries-cpu-le.args
new file mode 100644
index 000..f4f8d5e
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-cpu-le.args
@@ -0,0 +1,7 @@
+LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test \
+QEMU_AUDIO_DRV=none /usr/bin/qemu-system-ppc64 -S -M pseries \
+-m 512 -smp 1 -nographic -nodefconfig -nodefaults \
+-chardev socket,id=charmonitor,path=/tmp/test-monitor,server,nowait \
+-mon chardev=charmonitor,id=monitor,mode=readline -no-acpi -boot c -usb \
+-chardev pty,id=charserial0 \
+-device spapr-vty,chardev=charserial0,reg=0x3000
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-cpu-le.xml
b/tests/qemuxml2argvdata/qemuxml2argv-pseries-cpu-le.xml
new file mode 100644
index 000..b791a73
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-cpu-le.xml
@@ -0,0 +1,17 @@
+
+ QEMUGuest1
+ 1ccfd97d-5eb4-478a-bbe6-88d254c16db7
+ 524288
+ 1
+
+hvm
+
+
+
+/usr/bin/qemu-system-ppc64
+
+
+
+
+
+
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 39ed66b..982019e 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1341,6 +1341,8 @@ mymain(void)
QEMU_CAPS_NODEFCONFIG);
DO_TEST("pseries-cpu-compat", QEMU_CAPS_KVM, QEMU_CAPS_CPU_HOST,
QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG);
+DO_TEST("pseries-cpu-le", QEMU_CAPS_KVM, QEMU_CAPS_CPU_HOST,
+QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG);
DO_TEST("disk-ide-drive-split",
QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG,
QEMU_CAPS_IDE_CD);
diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c
index 7b26e50..1fd7c96 100644
--- a/tests/testutilsqemu.c
+++ b/tests/testutilsqemu.c
@@ -86,6 +86,33 @@ static int testQemuAddPPC64Guest(virCapsPtr caps)
return -1;
}
+static int testQemuAddPPC64LEGuest(virCapsPtr caps)
+{
+static const char *machine[] = { "pseries" };
+virCapsGuestMachinePtr *machines = NULL;
+virCapsGuestPtr guest;
+
+machines = virCapabilitiesAllocMachines(machine, 1);
+if (!machines)
+goto error;
+
+guest = virCapabilitiesAddGuest(caps, "hvm", VIR_ARCH_PPC64LE,
+"/usr/bin/qemu-system-ppc64", NULL,
+ 1, machines);
+if (!guest)
+goto error;
+
+if (!virCapabilitiesAddGuestDomain(guest, "qemu", NULL, NULL, 0, NULL))
+goto error;
+
+return 0;
+
+ error:
+/* No way to free a guest? */
+virCapabilitiesFreeMachines(machines, 1);
+return -1;
+}
+
static int testQemuAddPPCGuest(virCapsPtr caps)
{
static const char *machine[] = { "g3beige",
@@ -332,6 +359,9 @@ virCapsPtr testQemuCapsInit(void)
if (testQemuAddPPC64Guest(caps))
goto cleanup;
+if (testQemuAddPPC64LEGuest(caps))
+goto cleanup;
+
if (testQemuAddPPCGuest(caps))
goto cleanup;
--
1.9.3
--
Prerna Saxena
Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/2] PowerPC: Augment XML schema to include 'ppc64le' arch ; newer pseries-2.* machine types.
>From 7128e773058751e4d1024ef7d8e4ad286c93ba55 Mon Sep 17 00:00:00 2001 From: Prerna Saxena Date: Thu, 26 Feb 2015 08:10:58 -0600 Subject: [PATCH 1/2] PowerPC: Augment XML schema to include 'ppc64le' arch and newer pseries-2.* machine types. Acked-by: Ján Tomko Signed-off-by: Prerna Saxena --- docs/schemas/basictypes.rng | 1 + docs/schemas/domaincommon.rng | 7 ++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng index 2bc9c1b..3e1841c 100644 --- a/docs/schemas/basictypes.rng +++ b/docs/schemas/basictypes.rng @@ -333,6 +333,7 @@ parisc64 ppc ppc64 + ppc64le ppcemb s390 s390x diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 27a24b4..8613125 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -412,13 +412,18 @@ - ppc64 + +ppc64 +ppc64le + pseries +pseries-2.1 +pseries-2.2 -- 1.9.3 -- Prerna Saxena Linux Technology Centre, IBM Systems and Technology Lab, Bangalore, India -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Patch 0/4] PowerPC fixes for libvirt
On Tuesday 17 February 2015 06:33 PM, Ján Tomko wrote: > On Sun, Feb 15, 2015 at 09:45:25AM +0530, Prerna Saxena wrote: >> This patch set addresses some miscellaneous fixes for libvirt on PowerPC. >> >> Details: >> Patch 1/4 : This adds 'qemu-system-ppc64' as the default emulator for ppc64 >> & ppc64le guests. >> Patch 2/4 : Fixes a small error where not specifying a CPU model leads to a >> NULL compat specification. > I have pushed the first two patches. > >> Patch 3/4 : This introduces 'ppc64le' and newer pseries machine types to >> domain schema. >> Patch 4/4 : Forbids floppy devices in domain XML. > These would be better if they included some test cases (so would 2/4, > but adding a different host cpu to the test suite is not that trivial). > > Jan Thanks for pushing patches 1,2. I will add testcases and resend patch #3. As discussed with you, I will rework Patch #4 in line with https://www.redhat.com/archives/libvir-list/2015-February/msg00410.html Regards, -- Prerna Saxena Linux Technology Centre, IBM Systems and Technology Lab, Bangalore, India -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 4/4] PowerPC : Do not allow floppy disks in domain XML.
Subject: [PATCH 4/4] PowerPC : Do not allow floppy disks in domain XML.
PowerKVM does not support floppy disks. Ensure that libvirt honours this.
Fixes : https://bugzilla.redhat.com/show_bug.cgi?id=1180486
Signed-off-by: Prerna Saxena
---
src/conf/domain_conf.c | 8
1 file changed, 8 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 6a57d80..df3a768 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -13892,6 +13892,14 @@ virDomainDefParseXML(xmlDocPtr xml,
if (!disk)
goto error;
+if (ARCH_IS_PPC64(def->os.arch) &&
+STREQLEN(def->os.machine, "pseries", 7) &&
+disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Pseries machine does not support floppy device"));
+goto error;
+}
+
virDomainDiskInsertPreAlloced(def, disk);
}
VIR_FREE(nodes);
--
1.9.3
--
Prerna Saxena
Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/4] PowerPC: Augment XML schema to include 'ppc64le' arch and pseries* machine types.
PowerPC: Augment XML schema to include 'ppc64le' arch and newer pseries-2.* machine types. Signed-off-by: Prerna Saxena --- docs/schemas/basictypes.rng | 1 + docs/schemas/domaincommon.rng | 7 ++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng index 2bc9c1b..3e1841c 100644 --- a/docs/schemas/basictypes.rng +++ b/docs/schemas/basictypes.rng @@ -333,6 +333,7 @@ parisc64 ppc ppc64 + ppc64le ppcemb s390 s390x diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index d467dce..3a91365 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -412,13 +412,18 @@ - ppc64 + +ppc64 +ppc64le + pseries +pseries-2.1 +pseries-2.2 -- 1.9.3 -- Prerna Saxena Linux Technology Centre, IBM Systems and Technology Lab, Bangalore, India -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/4] PowerPC : Forbid NULL CPU model with 'host-model' mode.
PowerPC : Forbid NULL CPU model with 'host-model' mode in qemu command line.
This ensures that an XML such as following:
...
...
will not generate a '-cpu host,compat=(null)' command line with
qemu-system-ppc64.
Signed-off-by: Prerna Saxena
---
src/qemu/qemu_command.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 9c25788..317bb19 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -6685,7 +6685,8 @@ qemuBuildCpuModelArgStr(virQEMUDriverPtr driver,
virBufferAddLit(buf, "host");
if (ARCH_IS_PPC64(def->os.arch) &&
-cpu->mode == VIR_CPU_MODE_HOST_MODEL) {
+cpu->mode == VIR_CPU_MODE_HOST_MODEL &&
+def->cpu->model != NULL) {
virBufferAsprintf(buf, ",compat=%s", def->cpu->model);
} else {
featCpu = cpu;
--
1.9.3
--
Prerna Saxena
Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/4] PowerPC : Make 'qemu-system-ppc64' the default emulator on ppc64[le].
PowerPC : Explicitly associate 'qemu-system-ppc64' as the
default emulator for all 64-bit PowerPC guests ( both Big & Little Endian )
Signed-off-by: Prerna Saxena
---
src/qemu/qemu_capabilities.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index a04095e..4596f6c 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -673,9 +673,14 @@ virQEMUCapsFindBinaryForArch(virArch hostarch,
virArch guestarch)
{
char *ret;
-const char *archstr = virQEMUCapsArchToString(guestarch);
+const char *archstr;
char *binary;
+if (ARCH_IS_PPC64(guestarch))
+archstr = virQEMUCapsArchToString(VIR_ARCH_PPC64);
+else
+archstr = virQEMUCapsArchToString(guestarch);
+
if (virAsprintf(&binary, "qemu-system-%s", archstr) < 0)
return NULL;
--
1.9.3
--
Prerna Saxena
Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [Patch 0/4] PowerPC fixes for libvirt
This patch set addresses some miscellaneous fixes for libvirt on PowerPC. Details: Patch 1/4 : This adds 'qemu-system-ppc64' as the default emulator for ppc64 & ppc64le guests. Patch 2/4 : Fixes a small error where not specifying a CPU model leads to a NULL compat specification. Patch 3/4 : This introduces 'ppc64le' and newer pseries machine types to domain schema. Patch 4/4 : Forbids floppy devices in domain XML. Regards, -- Prerna Saxena Linux Technology Centre, IBM Systems and Technology Lab, Bangalore, India -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [RFC] [Patch] Make hugepage testcase arch-agnostic
Hi,
I have attached this patch as a response to a recent failure observed on
PowerPC architecture by commit
311b4a67.
This patch introduces a check for dynamically obtaining system page size for
test hugepages-pages6 under 'qemuxml2argv' suite. ( See patch for more verbose
problem description)
This patch is not the most perfect implementation -- it fails syntax check; and
has a Makefile-driven cleanup pending. I will be happy to deck it up and send
it if the community concurs with this
approach.
We could also implement this via a shell script ( just like
'virt-test-aa-helper') but I couldnt find an easy way to determine host page
size.
Awaiting community responses,
Prerna
>From 8a64d4d22e2e65158d3caa45b615ca9a263f841f Mon Sep 17 00:00:00 2001
From: Prerna Saxena
Date: Mon, 2 Feb 2015 10:48:48 +0530
Subject: [PATCH] Commit 311b4a67 introduces a test for normal-page backed
guest XML. However, it hardcodes the page size to 4 KB which is only valid
for Intel Make check consequently fails on PowerPC where page size is 64KB
This makes the hugepages-pages6 test more modular, and enables the page size
to be picked up at runtime.
---
.../qemuxml2argv-hugepages-pages6.template | 32 ++
tests/qemuxml2argvtest.c | 24 +++-
2 files changed, 55 insertions(+), 1 deletion(-)
create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages6.template
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages6.template
b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages6.template
new file mode 100644
index 000..8b9b995
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages6.template
@@ -0,0 +1,32 @@
+
+ SomeDummyHugepagesGuest
+ ef1bdff4-27f3-4e85-a807-5fb4d58463cc
+ 1048576
+ 1048576
+
+
+
+
+
+ 2
+
+hvm
+
+
+
+ destroy
+ restart
+ destroy
+
+/usr/bin/qemu
+
+
+
+
+
+
+
+
+
+
+
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 89afa81..a16d937 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -479,6 +479,11 @@ mymain(void)
{
int ret = 0;
bool skipLegacyCPUs = false;
+const long system_page_size = sysconf(_SC_PAGESIZE) / 1024;
+int fd_in;
+FILE *f_out;
+char *template, *xml = NULL;
+char buf[1000];
abs_top_srcdir = getenv("abs_top_srcdir");
if (!abs_top_srcdir)
@@ -702,7 +707,24 @@ mymain(void)
DO_TEST_FAILURE("hugepages-pages4", QEMU_CAPS_MEM_PATH,
QEMU_CAPS_OBJECT_MEMORY_RAM, QEMU_CAPS_OBJECT_MEMORY_FILE);
DO_TEST("hugepages-pages5", QEMU_CAPS_MEM_PATH);
-DO_TEST("hugepages-pages6", NONE);
+
+if (virAsprintf(&template, "%s/qemuxml2argvdata/qemuxml2argv-%s.template",
+abs_srcdir, "hugepages-pages6") < 0 ||
+virAsprintf(&xml, "%s/qemuxml2argvdata/qemuxml2argv-%s.xml",
+abs_srcdir, "hugepages-pages6") < 0)
+return EXIT_FAILURE;
+fd_in = open(template, O_RDONLY);
+f_out = fopen(xml, "w");
+
+if ( fd_in != -1 && f_out != NULL ) {
+if(read(fd_in, &buf, sizeof(buf))) {
+fprintf(f_out, buf,system_page_size);
+fclose(f_out);
+close(fd_in);
+DO_TEST("hugepages-pages6", NONE);
+}
+}
+
DO_TEST("nosharepages", QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_MEM_MERGE);
DO_TEST("disk-cdrom", NONE);
DO_TEST("disk-cdrom-network-http", QEMU_CAPS_KVM, QEMU_CAPS_DEVICE,
--
1.9.3
--
Prerna Saxena
Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
