Re: [libvirt] [RFC PATCH 0/3] RFC for X86 RDT Cache Monitoring Technology (CMT) support
Please see my inline reply. > -Original Message- > From: Martin Kletzander [mailto:mklet...@redhat.com] > Sent: Thursday, June 14, 2018 3:54 PM > To: Wang, Huaqiang > Cc: libvir-list@redhat.com; Feng, Shaohe ; Niu, Bing > ; Ding, Jian-feng ; Zang, Rui > > Subject: Re: [libvirt] [RFC PATCH 0/3] RFC for X86 RDT Cache Monitoring > Technology (CMT) support > > On Tue, Jun 12, 2018 at 10:11:30AM +, Wang, Huaqiang wrote: > >Hi Martin, > > > >Thanks for your comments, please see my update inline below. > > > >> -Original Message- > >> From: Martin Kletzander [mailto:mklet...@redhat.com] > >> Sent: Monday, June 11, 2018 4:30 PM > >> To: Wang, Huaqiang > >> Cc: libvir-list@redhat.com; Feng, Shaohe ; > >> Niu, Bing ; Ding, Jian-feng > >> ; Zang, Rui > >> Subject: Re: [libvirt] [RFC PATCH 0/3] RFC for X86 RDT Cache > >> Monitoring Technology (CMT) support > >> > >> [It would be nice if you wrapped the long lines] > >I'll pay attention to these long lines. Thanks for advices. > > No need to, most email clients can do that automatically. Doing stuff like > this > manually is very unproductive :). > > >> > >> On Fri, Jun 08, 2018 at 05:02:16PM +0800, Wang Huaqiang wrote: > >> >This is an RFC request for supporting CPU Cache Monitoring > >> >Technology (CMT) > >> feature in libvirt. Since MBM is also another feature which is very > >> close to CMT, for simplicity we only discuss CMT here. MBM is the > >> followup that will be implemented after CMT. > >> >About CMT please refer to Intel x86 SDM section 17.18 of volume 3 > >> (link:https://software.intel.com/en-us/articles/intel-sdm). > >> > > >> > >> Can you elaborate on how is this different to the CMT perf event that > >> is already in libvirt and can be monitored through domstats API? > > > >Due to kernel interface removal of the perf events 'cmt,mbmt,mbml', the > >libvirt will no longer work with latest kernel. Please examine following > >link for > details. > >https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git > >/commit/?id=c39a0e2c8850f08249383f2425dbd8dbe4baad69, > > > >This serials is trying to provide the similar functions of this missing > >part for reporting cmt, mbmt and mbml information. First we only focus on > >cmt. > >Comparing with 'CMT perf event already in libvirt', I am trying to > >implement almost the same output as 'perf.cmt' in the output message of > >'domstats', but with another name , such as 'resctrl.cmt' or 'rdt.cmt' (or > >some > others). > >Another difference is that the underlying implementation is done > >through the kernel resctrl fs. > > > >This serials also attempts to provide a command interface for enabling > >and disabling cmt feature in scope of whole domain as original perf > >event based cmt could be controlled, enabled or disabled, through specifying > >'- > -enable cmt' or '--disable cmt' > >while invoking command 'virsh perf '. > >Our version is like 'virsh resctrl --enable' with a difference > >of no suffix of 'cmt'. The 'cmt' is omitted because the CMT and MBM > >function are both enabled whenever a valid resctrl fs sub-folder > >created, there is no way to disable one while enable another one, such as > enabling CMT while disabling MBML at the same time. > > > >This serials is trying to stick to interfaces exposed by perf event > >based CMT/MBM and provide an interface substitution for perf event > >based CMB/MBM, such as the perf based CMT only provides the cache > >occupancy information for whole domain only. We are also in thinking > >providing the capability to provide the cache occupancy information > >based on vcpus groups which may be specified in XML file. > >For example, if we have following configuration: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >The 'domstats' will output following information regarding cmt > > [root@dl-c200 libvirt]# virsh domstats vm1 --resctrl > > Domain: 'vm1' > > rdt.cmt.total=645562 > > rdt.cmt.vcpu0=104331 > > rdt.cmt.vcpu1_2=203200 > > rdt.cmt.vcpu3=340129 > > > > beware as 1-4 is something else than 1,4 so you need to differentiate that. > Or > to make it easier to parse for consumers of that API just list each vcpu on > its > own line (but then you need to say which are counted together). Or group them: > > rdt.cmt.total=645562 > rdt.cmt.group0.value=104331 > rdt.cmt.group0.vcpus=0 > rdt.cmt.group0.value=203200 > rdt.cmt.group0.vcpus=1-2 > rdt.cmt.group0.value=340129 > rdt.cmt.group0.vcpus=3 > > Honestly, I don't care that much how it is going to look, but it needs to be > easy > to parse and understand. > Honestly, I don't care that much how it is going to look, but it needs to be easy to parse and understand. Your arrangement by separating group vcpus and group resource value is much
[libvirt] [PATCH v1.5 4/8] backup: Document new XML for backups
Prepare for new checkpoint and backup APIs by describing the XML that will represent a checkpoint and backup. The checkpoint XML is modeled heavily after virDomainSnapshotPtr, since both represent a point in time of the guest (however, a snapshot exists with the intent to roll back to that point, while a checkpoint exists to facilitate later incremental backups). Meanwhile, the backup XML has enough information to represent both push model (the hypervisor writes the backup file to a location of the user's choice) and the pull model (the hypervisor needs local temporary storage, and also creates an NBD server that the user can use to read the backup via a third-party client).. But while a snapshot exists with the intent of rolling back to that state, a checkpoint instead makes it possible to create an incremental backup at a later time. Add testsuite coverage for some minimal uses of both XML. Ultimately, I'd love for push model backups to target a network driver rather than just a local file or block device; but doing that got hairy (while uses as the description of a host or network resource, I picked as the description of a push model backup target [defaults to qcow2 but can also be raw or any other format], and as the description of a pull model backup scratch space [must be qcow2]). The ideal refactoring would be a way to parameterize RNG to accept ... so that the name of the subelement can be for domain, or or as needed for backups. Future patches may improve this area of code. Signed-off-by: Eric Blake --- An updated version of the XML descriptions, while I still work on piecing together the pieces needed to demo the XML in action. Missing changes that were pointed out by John, but also supplies the XML that was previously a black box. docs/docs.html.in| 3 +- docs/domainstatecapture.html.in | 4 +- docs/format.html.in | 1 + docs/formatcheckpoint.html.in| 273 +++ docs/index.html.in | 3 +- docs/schemas/domainbackup.rng| 180 ++ docs/schemas/domaincheckpoint.rng| 89 + libvirt.spec.in | 2 + mingw-libvirt.spec.in| 4 + tests/domainbackupxml2xmlin/backup-pull.xml | 9 + tests/domainbackupxml2xmlin/backup-push.xml | 9 + tests/domainbackupxml2xmlin/empty.xml| 1 + tests/domainbackupxml2xmlout/backup-pull.xml | 9 + tests/domainbackupxml2xmlout/backup-push.xml | 9 + tests/domainbackupxml2xmlout/empty.xml | 7 + tests/domaincheckpointxml2xmlin/empty.xml| 1 + tests/domaincheckpointxml2xmlin/sample.xml | 7 + tests/domaincheckpointxml2xmlout/empty.xml | 10 + tests/domaincheckpointxml2xmlout/sample.xml | 16 ++ tests/virschematest.c| 4 + 20 files changed, 637 insertions(+), 4 deletions(-) create mode 100644 docs/formatcheckpoint.html.in create mode 100644 docs/schemas/domainbackup.rng create mode 100644 docs/schemas/domaincheckpoint.rng create mode 100644 tests/domainbackupxml2xmlin/backup-pull.xml create mode 100644 tests/domainbackupxml2xmlin/backup-push.xml create mode 100644 tests/domainbackupxml2xmlin/empty.xml create mode 100644 tests/domainbackupxml2xmlout/backup-pull.xml create mode 100644 tests/domainbackupxml2xmlout/backup-push.xml create mode 100644 tests/domainbackupxml2xmlout/empty.xml create mode 100644 tests/domaincheckpointxml2xmlin/empty.xml create mode 100644 tests/domaincheckpointxml2xmlin/sample.xml create mode 100644 tests/domaincheckpointxml2xmlout/empty.xml create mode 100644 tests/domaincheckpointxml2xmlout/sample.xml diff --git a/docs/docs.html.in b/docs/docs.html.in index 4c46b74980..4914e7dbed 100644 --- a/docs/docs.html.in +++ b/docs/docs.html.in @@ -79,7 +79,8 @@ domain capabilities, node devices, secrets, - snapshots + snapshots, + backups and checkpoints URI format The URI formats used for connecting to libvirt diff --git a/docs/domainstatecapture.html.in b/docs/domainstatecapture.html.in index 00ab7e8ee1..4de93c87c8 100644 --- a/docs/domainstatecapture.html.in +++ b/docs/domainstatecapture.html.in @@ -154,9 +154,9 @@ time as a new backup, so that the next incremental backup can refer to the incremental state since the checkpoint created during the current backup. Guest state is then actually -captured using virDomainBackupBegin(). +this command. Examples diff --git a/docs/format.html.in b/docs/format.html.in index 22b23e3fc7..8c4e15e079 100644 --- a/docs/format.html.in +++ b/docs/format.html.in @@ -24,6 +24,7 @@ Node devices Secrets Snapshots + Backups and checkpoints Command line validation diff --git a/docs/formatcheckpoint.html.in
[libvirt] [PATCH] util: fix mount issue by moving NULL value to "none" in syscall.
After running libvirt daemon with valgrind tools, some errors are
appearing when you try to start a domain. One example:
==18012== Syscall param mount(type) points to unaddressable byte(s)
==18012==at 0x6FEE3CA: mount (syscall-template.S:78)
==18012==by 0x531344D: virFileMoveMount (virfile.c:3828)
==18012==by 0x27FE7675: qemuDomainBuildNamespace (qemu_domain.c:11501)
==18012==by 0x2800C44E: qemuProcessHook (qemu_process.c:2870)
==18012==by 0x52F7E1D: virExec (vircommand.c:726)
==18012==by 0x52F7E1D: virCommandRunAsync (vircommand.c:2477)
==18012==by 0x52F4EDD: virCommandRun (vircommand.c:2309)
==18012==by 0x2800A731: qemuProcessLaunch (qemu_process.c:6235)
==18012==by 0x2800D6B4: qemuProcessStart (qemu_process.c:6569)
==18012==by 0x28074876: qemuDomainObjStart (qemu_driver.c:7314)
==18012==by 0x280522EB: qemuDomainCreateWithFlags (qemu_driver.c:7367)
==18012==by 0x55484BF: virDomainCreate (libvirt-domain.c:6531)
==18012==by 0x12CDBD: remoteDispatchDomainCreate
(remote_daemon_dispatch_stubs.h:4350)
==18012==by 0x12CDBD: remoteDispatchDomainCreateHelper
(remote_daemon_dispatch_stubs.h:4326)
==18012== Address 0x0 is not stack'd, malloc'd or (recently) free'd
Some documentation recommends to use "none" when you don't have a
filesystem type to use. Specially, for bind and move actions.
Signed-off-by: Julio Faracco
---
src/util/virfile.c| 2 +-
src/util/virprocess.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c
index 9296ccbe2a..378d03ecf0 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -3825,7 +3825,7 @@ virFileMoveMount(const char *src,
{
const unsigned long mount_flags = MS_MOVE;
-if (mount(src, dst, NULL, mount_flags, NULL) < 0) {
+if (mount(src, dst, "none", mount_flags, NULL) < 0) {
virReportSystemError(errno,
_("Unable to move %s mount to %s"),
src, dst);
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index 1f3a27..f92b0dce37 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -1181,7 +1181,7 @@ virProcessSetupPrivateMountNS(void)
goto cleanup;
}
-if (mount("", "/", NULL, MS_SLAVE|MS_REC, NULL) < 0) {
+if (mount("", "/", "none", MS_SLAVE|MS_REC, NULL) < 0) {
virReportSystemError(errno, "%s",
_("Failed to switch root mount into slave mode"));
goto cleanup;
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/8] backup: Introduce virDomainCheckpoint APIs
On 06/25/2018 06:16 PM, John Ferlan wrote:
On 06/13/2018 12:42 PM, Eric Blake wrote:
Introduce a bunch of new public APIs related to backup checkpoints.
Checkpoints are modeled heavily after virDomainSnapshotPtr (both
represent a point in time of the guest), although a snapshot exists
with the intent of rolling back to that state, while a checkpoint
exists to make it possible to create an incremental backup at a later
time.
Signed-off-by: Eric Blake
---
docs/Makefile.am| 3 +
docs/apibuild.py| 2 +
docs/docs.html.in | 1 +
include/libvirt/libvirt-domain-checkpoint.h | 147 ++
include/libvirt/libvirt.h | 5 +-
libvirt.spec.in | 1 +
mingw-libvirt.spec.in | 2 +
po/POTFILES | 1 +
src/Makefile.am | 2 +
src/driver-hypervisor.h | 60 ++-
src/libvirt-domain-checkpoint.c | 708
src/libvirt_public.syms | 16 +
12 files changed, 944 insertions(+), 4 deletions(-)
create mode 100644 include/libvirt/libvirt-domain-checkpoint.h
create mode 100644 src/libvirt-domain-checkpoint.c
In a word... Overwhelming!
Yeah, it's been a lot of code on my end, and more still to come.
I have concerns related to committing the API before everyone is sure
about the underlying hypervisor code. No sense in baking in an API only
to find out later needs/issues. It seems we
Incomplete sentence? But it definitely explains why I want a working
demo of the API in use, even if targeting what is currently experimental
qemu commands, to show that the API does what we want.
I see checkpoint code borrows the domain connection - is that similar to
snapshots?
Yes, I was very heavily borrowing the code for snapshots, as both items
are sub-objects that are related to a domain at a point in time.
I won't go too in depth here - mostly just scan to look for obvious issues.
+++ b/include/libvirt/libvirt-domain-checkpoint.h
@@ -0,0 +1,147 @@
+/*
+ * libvirt-domain-checkpoint.h
+ * Summary: APIs for management of domain checkpoints
+ * Description: Provides APIs for the management of domain checkpoints
+ * Author: Eric Blake
+ *
+ * Copyright (C) 2006-2018 Red Hat, Inc.
Since it's created in 2018 - shouldn't it just list that? Not my area of
expertise by any stretch of the imagination though.
Since I copied-and-pasted from snapshots, I like to keep the full range
of years from my template code; I could use just 2018 by calling it new
code, but I don't see it being much of an issue either way (no one will
use this header in isolation, so whether it has the project's full
copyright years, or just this file's earliest year of existence, doesn't
really matter when git history will paint a full picture).
+/**
+ * virDomainCheckpointListFlags:
+ *
+ * Flags valid for virDomainListCheckpoints() and
+ * virDomainCheckpointListChildren(). Note that the interpretation of
+ * flag (1<<0) depends on which function it is passed to; but serves
+ * to toggle the per-call default of whether the listing is shallow or
+ * recursive. Remaining bits come in groups; if all bits from a group
+ * are 0, then that group is not used to filter results. */
^^
There's an extra space here
Yeah, I tend to do two spaces after full stop (old-school typewriting
convention). I can make it a point to use just one when revising this
patch, although I don't think it makes too much difference either way.
+typedef enum {
+VIR_DOMAIN_CHECKPOINT_LIST_ROOTS = (1 << 0), /* Filter by checkpoints
+ with no parents, when
+ listing a domain */
+VIR_DOMAIN_CHECKPOINT_LIST_DESCENDANTS = (1 << 0), /* List all descendants,
+ not just children,
when
+ listing a checkpoint
*/
There's two "1 << 0" entries - ironically the doc page render lists
these in the opposite order. Still I see this is essentially a copy of
the SnapshotListFlags. So do we really need to keep that when there's
only 2 API's?
That's the same way snapshots did it, and yes, two separate names makes
the most sense for both consistency and usage. That is, you call either:
virDomainListCheckpoints(,0) (list all checkpoints, by recursing)
virDomainListCheckpoints(,_LIST_ROOTS) (list only roots, by not recursing)
virDomainCheckpointListChildren(,0) (list only direct children, by not
recursing)
virDomainCheckpointListChildren(,_LIST_DESCENDENTS) (list all
generations, by recursing)
but it was easier to declare one set of flags than two separate enums
Re: [libvirt] [PATCH] appdev-guide/Connections: Remove duplicated entries
On Mon, Jun 25, 2018 at 05:13:26PM +0200, Erik Skultety wrote: > On Fri, Jun 22, 2018 at 05:44:18PM -0300, Marcos Paulo de Souza wrote: > > On Mon, Jun 25, 2018 at 01:35:33PM +0200, Erik Skultety wrote: > > > On Sat, Jun 23, 2018 at 11:32:55AM -0300, Marcos Paulo de Souza wrote: > > > > Both /hosts/cpu/arch and /hosts/cpu/features appear twice in the > > > > documentation. > > > > > > > > Signed-off-by: Marcos Paulo de Souza > > > > --- > > > > > > > > This is my first submission to libvirt, so let me know if I forgot > > > > something. > > > > > > Which repo is this patch against? > > > > This patch is againt the appdev-guide located here: > > https://libvirt.org/git/?p=libvirt-appdev-guide.git;a=summary > > > > The repository seems to be the source of the documenation located in > > libvirt.org: > > https://libvirt.org/devguide.html > > > > Is there other place to send patches for appdev-guide? > > No, but usually we change patch subject prefix to PATCH-project so that it's > obvious which project you're sending the patch against, because from the > commit > subject itself it's not apparent at first sight, especially when noone has > touched that git for over 6 years. The document itself was never even finished > and regarding the publican we basically it by moving the static content to the > main repo. Anyhow, I'm fairly sceptical anyone ever used the development guide > solely because of how much it's missing, so I don't know how much sense it > makes > to push the patch and whether we keep building the repo so that the change > would actually be propagated, even though it's correct, but Dan will know the > answer for sure, so I'm CCing him. OK, fair enough. As I saw a commit in the last months, I tought it would be good to improve it. But OK, I will try to read the docs from the main repo, thanks for pointing it out. Maybe it would be nice to state this in the libvirt.org. Thanks, > > Erik -- Thanks, Marcos -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/8] backup: Introduce virDomainCheckpoint APIs
On 06/13/2018 12:42 PM, Eric Blake wrote:
> Introduce a bunch of new public APIs related to backup checkpoints.
> Checkpoints are modeled heavily after virDomainSnapshotPtr (both
> represent a point in time of the guest), although a snapshot exists
> with the intent of rolling back to that state, while a checkpoint
> exists to make it possible to create an incremental backup at a later
> time.
>
> Signed-off-by: Eric Blake
> ---
> docs/Makefile.am| 3 +
> docs/apibuild.py| 2 +
> docs/docs.html.in | 1 +
> include/libvirt/libvirt-domain-checkpoint.h | 147 ++
> include/libvirt/libvirt.h | 5 +-
> libvirt.spec.in | 1 +
> mingw-libvirt.spec.in | 2 +
> po/POTFILES | 1 +
> src/Makefile.am | 2 +
> src/driver-hypervisor.h | 60 ++-
> src/libvirt-domain-checkpoint.c | 708
>
> src/libvirt_public.syms | 16 +
> 12 files changed, 944 insertions(+), 4 deletions(-)
> create mode 100644 include/libvirt/libvirt-domain-checkpoint.h
> create mode 100644 src/libvirt-domain-checkpoint.c
>
In a word... Overwhelming!
I have concerns related to committing the API before everyone is sure
about the underlying hypervisor code. No sense in baking in an API only
to find out later needs/issues. It seems we
I see checkpoint code borrows the domain connection - is that similar to
snapshots?
I won't go too in depth here - mostly just scan to look for obvious issues.
> diff --git a/docs/Makefile.am b/docs/Makefile.am
> index 9620587a77..0df8ebbd64 100644
> --- a/docs/Makefile.am
> +++ b/docs/Makefile.am
> @@ -25,6 +25,7 @@ apihtml = \
> apihtml_generated = \
>html/libvirt-libvirt-common.html \
>html/libvirt-libvirt-domain.html \
> + html/libvirt-libvirt-domain-checkpoint.html \
>html/libvirt-libvirt-domain-snapshot.html \
>html/libvirt-libvirt-event.html \
>html/libvirt-libvirt-host.html \
> @@ -318,6 +319,7 @@ $(python_generated_files): $(APIBUILD_STAMP)
> $(APIBUILD_STAMP): $(srcdir)/apibuild.py \
> $(top_srcdir)/include/libvirt/libvirt.h \
> $(top_srcdir)/include/libvirt/libvirt-common.h.in \
> + $(top_srcdir)/include/libvirt/libvirt-domain-checkpoint.h \
> $(top_srcdir)/include/libvirt/libvirt-domain-snapshot.h \
> $(top_srcdir)/include/libvirt/libvirt-domain.h \
> $(top_srcdir)/include/libvirt/libvirt-event.h \
> @@ -334,6 +336,7 @@ $(APIBUILD_STAMP): $(srcdir)/apibuild.py \
> $(top_srcdir)/include/libvirt/libvirt-admin.h \
> $(top_srcdir)/include/libvirt/virterror.h \
> $(top_srcdir)/src/libvirt.c \
> + $(top_srcdir)/src/libvirt-domain-checkpoint.c \
> $(top_srcdir)/src/libvirt-domain-snapshot.c \
> $(top_srcdir)/src/libvirt-domain.c \
> $(top_srcdir)/src/libvirt-host.c \
> diff --git a/docs/apibuild.py b/docs/apibuild.py
> index 5e218a9ad0..471547cea7 100755
> --- a/docs/apibuild.py
> +++ b/docs/apibuild.py
> @@ -26,6 +26,7 @@ debugsym = None
> included_files = {
>"libvirt-common.h": "header with general libvirt API definitions",
>"libvirt-domain.h": "header with general libvirt API definitions",
> + "libvirt-domain-checkpoint.h": "header with general libvirt API
> definitions",
>"libvirt-domain-snapshot.h": "header with general libvirt API definitions",
>"libvirt-event.h": "header with general libvirt API definitions",
>"libvirt-host.h": "header with general libvirt API definitions",
> @@ -39,6 +40,7 @@ included_files = {
>"virterror.h": "header with error specific API definitions",
>"libvirt.c": "Main interfaces for the libvirt library",
>"libvirt-domain.c": "Domain interfaces for the libvirt library",
> + "libvirt-domain-checkpoint.c": "Domain checkpoint interfaces for the
> libvirt library",
>"libvirt-domain-snapshot.c": "Domain snapshot interfaces for the libvirt
> library",
>"libvirt-host.c": "Host interfaces for the libvirt library",
>"libvirt-interface.c": "Interface interfaces for the libvirt library",
> diff --git a/docs/docs.html.in b/docs/docs.html.in
> index 11dfd27ba6..63dbdf7755 100644
> --- a/docs/docs.html.in
> +++ b/docs/docs.html.in
> @@ -97,6 +97,7 @@
> Reference manual for the C public API, split in
>common,
>domain,
> + domain
> checkpoint,
>domain
> snapshot,
>error,
>event,
> diff --git a/include/libvirt/libvirt-domain-checkpoint.h
> b/include/libvirt/libvirt-domain-checkpoint.h
> new file mode 100644
> index 00..4a7dc73089
> --- /dev/null
> +++ b/include/libvirt/libvirt-domain-checkpoint.h
> @@ -0,0 +1,147 @@
> +/*
> + *
Re: [libvirt] [PATCH 1/8] snapshots: Avoid term 'checkpoint' for full system snapshot
On 06/22/2018 04:16 PM, John Ferlan wrote: On 06/13/2018 12:42 PM, Eric Blake wrote: Upcoming patches plan to introduce virDomainCheckpointPtr as a new object for use in incremental backups, along with documentation how incremental backups differ from snapshots. But first, we need to rename any existing mention of a 'system checkpoint' to instead be a 'full system state snapshot', so that we aren't overloading the term checkpoint. Signed-off-by: Eric Blake --- Bikeshed suggestions on what to name the new object for use in backups is welcome, if we would rather keep the term 'checkpoint' for a disk+memory snapshot. --- "Naming is hard" and opinions can vary greatly - be careful for what you ask in case you receive something not wanted ;-). I haven't followed the discussions thus far all that closely, but I'll give this a go anyway since it's languishing and saying nothing is akin to implicitly agreeing everything is fine. Fair warning, I'm not all that familiar with snapshot algorithms having largely tried to ignore it since others (Eric and Peter) have far more in depth knowledge. In any case, another option for the proposed "checkpoint" could be a "snapshot reference". One can start or end a reference period and then set or clear a reference point. What I'm not clear on yet is whether the intention is to have this checkpoint (and backup) be integrated in any way to the existing snapshot algorithms. I guess part of me thinks that if I take a full system snapshot, then any backup/checkpoint data should be included so that if/when I go back to that point in time I can start from whence I left as it relates to my backup. Kind of a superset and/or integrated model rather than something bolted onto the side to resolve a specific need. That's a tough call. My current design has incremental backups completely separate from the existing checkpoint code for several reasons: - the snapshot code is already confusing with lots of flags (internal/external, disk/memory, etc) - snapshots can be reverted to (well, in theory - we STILL can't revert to an external snapshot cleanly, even though the design supports it) - incremental backups are not direct revert points so, rather than bolt something on to the existing design, I went with a new concept. As you found later in the series, I then tried to provide a good summary page describing the different pieces, and what tradeoffs are involved in order to know which approach will work for a given need. I suppose a reservation I have about separate virDomainCheckpoint* and virDomainBackup* API's is understanding the relationship between the two naming spaces. IIUC though a Checkpoint would be reference point in time within a Backup period. A sequence of snapshots are different points in time you can revert to. A sequence of checkpoints are different points in time you can use as the reference point for starting an incremental backup. So if we don't like the term 'checkpoint', maybe virDomainBlockBackupReference would work. But it is longer, and would make for some mouthful API names. Also, you commented elsewhere that 'virDomainBackupBegin' misses out on the fact that under the hood, it is a block operation (only disk state); would 'virDomainBlockBackupBegin' be any better? There are fewer APIs with the term 'Backup' than with 'Checkpoint', if we do want go with that particular rename. I do have more comments in patch2, but I want to make them coherent before posting. Still I wanted to be sure you got at least "some" feedback for this and well of course an opinion on checkpoint ;-) docs/formatsnapshot.html.in | 14 +++--- include/libvirt/libvirt-domain-snapshot.h | 2 +- src/conf/snapshot_conf.c | 2 +- src/libvirt-domain-snapshot.c | 4 ++-- src/qemu/qemu_driver.c| 12 ++-- tools/virsh-snapshot.c| 2 +- tools/virsh.pod | 14 +++--- 7 files changed, 25 insertions(+), 25 deletions(-) diff --git a/docs/formatsnapshot.html.in b/docs/formatsnapshot.html.in index fbbecfd242..f2e51df5ab 100644 --- a/docs/formatsnapshot.html.in +++ b/docs/formatsnapshot.html.in @@ -33,7 +33,7 @@ resume in a consistent state; but if the disks are modified externally in the meantime, this is likely to lead to data corruption. - system checkpoint + full system state Is "state" superfluous in this context? IOW: Everywhere that "full system state" exists, it seems "full system" could be used. Other synonyms that came up are complete, entire, integrated, or thorough (hah!). But I think "Full System" conveys enough meaning even though it could convey more meaning than intended. Okay, I can live with shortening the replacement to 'full system'. Don't know if it will happen in the v2 series that I hope to post later tonight, or if it would be done on top (my immediate
Re: [libvirt] [PATCH 4/8] backup: Document new XML for backups
On 06/13/2018 12:42 PM, Eric Blake wrote: > Prepare for new checkpoint and backup APIs by describing the XML > that will represent a checkpoint. This is modeled heavily after > the XML for virDomainSnapshotPtr, since both represent a point in > time of the guest. But while a snapshot exists with the intent > of rolling back to that state, a checkpoint instead makes it > possible to create an incremental backup at a later time. > > Add testsuite coverage of a minimal use of the XML. > > Signed-off-by: Eric Blake > --- > docs/docs.html.in | 3 +- > docs/domainstatecapture.html.in| 4 +- > docs/formatcheckpoint.html.in | 273 > + > docs/schemas/domaincheckpoint.rng | 89 ++ > libvirt.spec.in| 1 + > mingw-libvirt.spec.in | 2 + > tests/domaincheckpointxml2xmlin/empty.xml | 1 + > tests/domaincheckpointxml2xmlout/empty.xml | 10 ++ > tests/virschematest.c | 2 + > 9 files changed, 382 insertions(+), 3 deletions(-) > create mode 100644 docs/formatcheckpoint.html.in > create mode 100644 docs/schemas/domaincheckpoint.rng > create mode 100644 tests/domaincheckpointxml2xmlin/empty.xml > create mode 100644 tests/domaincheckpointxml2xmlout/empty.xml > > diff --git a/docs/docs.html.in b/docs/docs.html.in > index 4c46b74980..11dfd27ba6 100644 > --- a/docs/docs.html.in > +++ b/docs/docs.html.in > @@ -79,7 +79,8 @@ >domain capabilities, >node devices, >secrets, > - snapshots > + snapshots, > + checkpoints > > URI format > The URI formats used for connecting to libvirt Add a link in the format.html.in and index.html.in pages too. > diff --git a/docs/domainstatecapture.html.in b/docs/domainstatecapture.html.in > index 00ab7e8ee1..4de93c87c8 100644 > --- a/docs/domainstatecapture.html.in > +++ b/docs/domainstatecapture.html.in > @@ -154,9 +154,9 @@ > time as a new backup, so that the next incremental backup can > refer to the incremental state since the checkpoint created > during the current backup. Guest state is then actually > -captured using virDomainBackupBegin(). > +this command. > > > Examples > diff --git a/docs/formatcheckpoint.html.in b/docs/formatcheckpoint.html.in > new file mode 100644 > index 00..34507a9f68 > --- /dev/null > +++ b/docs/formatcheckpoint.html.in > @@ -0,0 +1,273 @@ > + > + > +http://www.w3.org/1999/xhtml;> > + > +Checkpoint and Backup XML format > + > + > + > +Checkpoint XML > + > + > + Domain disk backups, including incremental backups, are one form' > + of domain state capture. > + IMO: Strange opening line for something describing checkpoints. As I've read further, the fact that checkpoint and backup is only supported for qcow2 domain disks I would think needs to be up at the top here - front an center. No sense reading any further for raw disks. Yet another patch 2 "factor" related to choosing a backup plan. What to do if you have raw devices and how those should be handled. This would seem to preclude LUKS encrypted devices, true? What about disks with various levels of backingStore logic? I can only imagine some of the depth issues causing problems with that logic would be applicable here too with the hierarchical approach. > + > + Libvirt is able to facilitate incremental backups by tracking > + disk checkpoints, or points in time against which it is easy to s/, or/ or/ > + compute which portion of the disk has changed. Given a full > + backup (a backup created from the creation of the disk to a > + given point in time, coupled with the creation of a disk s/time,/time) > + checkpoint at that time), and an incremental backup (a backup s/time),/time, > + created from just the dirty portion of the disk between the > + first checkpoint and the second backup operation), it is > + possible to do an offline reconstruction of the state of the > + disk at the time of the second backup, without having to copy as s/without,/without/ > + much data as a second full backup would require. Most disk > + checkpoints are created in concert with a backup, s/backup,/backup/ > + via virDomainBackupBegin(); however, libvirt also > + exposes enough support to create disk checkpoints independently > + from a backup operation, s/operation,/operation/ > + via virDomainCheckpointCreateXML(). > + NB: virDomainBackupBegin doesn't exist yet. Still a few patches away. > + > + Attributes of libvirt checkpoints are stored as child elements of > + the domaincheckpoint element. At checkpoint creation > + time, normally only the name, description, > + and disks elements are settable; the rest of the s/; the/. The
[libvirt] [PATCH python] libvirtaio: Fix compat with python 3.7
In python 3.7, async is now a keyword, so this throws a syntax error: File "/usr/lib64/python3.7/site-packages/libvirtaio.py", line 49 from asyncio import async as ensure_future ^ SyntaxError: invalid syntax Switch to getattr trickery to accomplish the same goal Signed-off-by: Cole Robinson --- libvirtaio.py | 10 ++ 1 file changed, 6 insertions(+), 4 deletions(-) If we don't care about python3 < 3.4.4 compat, we can just do from asyncio import ensure_future diff --git a/libvirtaio.py b/libvirtaio.py index 1c432dd..9d517e6 100644 --- a/libvirtaio.py +++ b/libvirtaio.py @@ -43,10 +43,12 @@ import warnings import libvirt -try: -from asyncio import ensure_future -except ImportError: -from asyncio import async as ensure_future +# python < 3.4.4: we want 'async' +# python >= 3.4.4 < 3.7, we want 'ensure_future' +# python >= 3.7, 'async' is a reserved keyword, so we can't import it +ensure_future = getattr(asyncio, "ensure_future", None) +if not ensure_future: +ensure_future = getattr(asyncio, "async") class Callback(object): -- 2.17.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] docs: schema: Add missing to devices
On 06/22/2018 07:24 AM, Han Han wrote: > For input,hub,redirdev devices, their sub-elements should be interleaved. > > input device: interleave for , , > hub device: interleave for , > redirdev device: interleave for , , , > > Signed-off-by: Han Han > --- > docs/schemas/domaincommon.rng | 60 +++ > 1 file changed, 33 insertions(+), 27 deletions(-) > > diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng > index 4a454dddb4..d262eb2b1b 100644 > --- a/docs/schemas/domaincommon.rng > +++ b/docs/schemas/domaincommon.rng > @@ -4270,11 +4270,19 @@ > > > > - > - > - > - > - > + > + > + > + > + > + > + > + > + > + > + > + > + > > > This is not that simple. In between these two hunks there's another possible element defined for this kind of device: What we need is to put the whole body into . Fixed, ACKed and pushed. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Fix memory leak in qemuDomainBlockJobSetSpeed()
On 06/25/2018 04:15 PM, w00251574 wrote: > Subject: [PATCH] qemu: Fix memory leak in qemuDomainBlockJobSetSpeed() > > fix 'device' leak in qemuDomainBlockJobSetSpeed > > Signed-off-by: Jie Wang > --- > src/qemu/qemu_driver.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index 921aafcd79..b45c26eb3b 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -17395,6 +17395,7 @@ qemuDomainBlockJobSetSpeed(virDomainPtr dom, > qemuDomainObjEndJob(driver, vm); > > cleanup: > +VIR_FREE(device); > virDomainObjEndAPI(); > > return ret; > Almost. @device is type of 'const char *' and that needs to change (one should never free a const string). Secondly, with this change there are multiple ways to get to @cleanup label without @device being set to anything (and thus we might end up freeing random pointer). Fixed and pushed. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: monitor: Fix memory leak in qemuMonitorJSONNBDServerStart()
On 06/25/2018 03:48 PM, w00251574 wrote: > Subject: [PATCH] qemu: monitor: Fix memory leak in > qemuMonitorJSONNBDServerStart() > > Exiting early through the return path did result in 'port_str' > being leaked. > > Signed-off-by: Jie Wang > --- > src/qemu/qemu_monitor_json.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) ACKed and pushed. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] spec: Move SASL configuration file from -libs to -daemon
On 06/21/2018 04:21 PM, Andrea Bolognani wrote: > SASL authentication is configured server-side, so the sample > configuration file should be shipped along with the daemon > rather than with the libraries. > > Signed-off-by: Andrea Bolognani > --- > libvirt.spec.in | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) ACK Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 6/7] qemu: Format HPT maxpagesize on the command line
This makes the feature fully functional.
https://bugzilla.redhat.com/show_bug.cgi?id=1571078
Signed-off-by: Andrea Bolognani
---
src/qemu/qemu_command.c | 12
tests/qemuxml2argvdata/pseries-features.args | 3 ++-
2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 5cd6d44a88..748b596f63 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7265,6 +7265,18 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
virBufferAsprintf(, ",resize-hpt=%s", str);
}
+
+if (def->hpt_maxpagesize > 0) {
+if (!virQEMUCapsGet(qemuCaps,
QEMU_CAPS_MACHINE_PSERIES_CAP_HPT_MAX_PAGE_SIZE)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Configuring the page size for HPT guests "
+ "is not supported by this QEMU binary"));
+goto cleanup;
+}
+
+virBufferAsprintf(, ",cap-hpt-max-page-size=%lluk",
+ def->hpt_maxpagesize);
+}
}
if (cpu && cpu->model &&
diff --git a/tests/qemuxml2argvdata/pseries-features.args
b/tests/qemuxml2argvdata/pseries-features.args
index f5c1959cca..12c14715c6 100644
--- a/tests/qemuxml2argvdata/pseries-features.args
+++ b/tests/qemuxml2argvdata/pseries-features.args
@@ -7,7 +7,8 @@ QEMU_AUDIO_DRV=none \
/usr/bin/qemu-system-ppc64 \
-name guest \
-S \
--machine pseries,accel=tcg,usb=off,dump-guest-core=off,resize-hpt=required \
+-machine pseries,accel=tcg,usb=off,dump-guest-core=off,resize-hpt=required,\
+cap-hpt-max-page-size=1048576k \
-m 512 \
-smp 1,sockets=1,cores=1,threads=1 \
-uuid 1ccfd97d-5eb4-478a-bbe6-88d254c16db7 \
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 5/7] conf: Parse and format HPT maxpagesize
Signed-off-by: Andrea Bolognani
---
docs/formatdomain.html.in | 11 --
docs/schemas/domaincommon.rng | 21 +++
src/conf/domain_conf.c| 36 ---
src/conf/domain_conf.h| 1 +
tests/qemuxml2argvdata/pseries-features.xml | 4 ++-
tests/qemuxml2argvtest.c | 1 +
tests/qemuxml2xmloutdata/pseries-features.xml | 4 ++-
tests/qemuxml2xmltest.c | 1 +
8 files changed, 63 insertions(+), 16 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 89672a0486..1cf92cd755 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -1922,7 +1922,9 @@
pvspinlock state='on'/
gic version='2'/
ioapic driver='qemu'/
- hpt resizing='required'/
+ hpt resizing='required'
+maxpagesize unit='MiB'16/maxpagesize
+ /hpt
vmcoreinfo state='on'/
smm state='on'
tseg unit='MiB'48/tseg
@@ -2149,7 +2151,12 @@
support; and required, which prevents the guest from
starting unless both the guest and the host support HPT resizing. If
the attribute is not defined, the hypervisor default will be used.
- Since 3.10.0 (QEMU/KVM only)
+ Since 3.10.0 (QEMU/KVM only).
+
+ The optional maxpagesize subelement can be used to
+ limit the usable page size for HPT guests. Common values are 64 KiB,
+ 16 MiB and 16 GiB; when not specified, the hypervisor default will
+ be used. Since 4.5.0 (QEMU/KVM only).
vmcoreinfo
Enable QEMU vmcoreinfo device to let the guest kernel save debug
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 4a454dddb4..0a661cf1e7 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -5121,13 +5121,20 @@
-
-
- enabled
- disabled
- required
-
-
+
+
+
+enabled
+disabled
+required
+
+
+
+
+
+
+
+
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 3f7b0d1bfe..d8cb7f37f3 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -19807,8 +19807,24 @@ virDomainDefParseXML(xmlDocPtr xml,
VIR_FREE(tmp);
}
-if (def->hpt_resizing != VIR_DOMAIN_HPT_RESIZING_NONE)
+if (virDomainParseScaledValue("./features/hpt/maxpagesize",
+ NULL,
+ ctxt,
+ >hpt_maxpagesize,
+ 1024,
+ ULLONG_MAX,
+ false) < 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ "%s",
+ _("Unable to parse HPT maxpagesize setting"));
+goto error;
+}
+def->hpt_maxpagesize = VIR_DIV_UP(def->hpt_maxpagesize, 1024);
+
+if (def->hpt_resizing != VIR_DOMAIN_HPT_RESIZING_NONE ||
+def->hpt_maxpagesize > 0) {
def->features[val] = VIR_TRISTATE_SWITCH_ON;
+}
break;
/* coverity[dead_error_begin] */
@@ -21987,15 +22003,18 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr
src,
case VIR_DOMAIN_FEATURE_HPT:
if (src->features[i] != dst->features[i] ||
-src->hpt_resizing != dst->hpt_resizing) {
+src->hpt_resizing != dst->hpt_resizing ||
+src->hpt_maxpagesize != dst->hpt_maxpagesize) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("State of feature '%s' differs: "
- "source: '%s,%s=%s', destination:
'%s,%s=%s'"),
+ "source: '%s,%s=%s,%s=%llu', destination:
'%s,%s=%s,%s=%llu'"),
featureName,
virTristateSwitchTypeToString(src->features[i]),
"resizing",
virDomainHPTResizingTypeToString(src->hpt_resizing),
+ "maxpagesize", src->hpt_maxpagesize,
virTristateSwitchTypeToString(dst->features[i]),
- "resizing",
virDomainHPTResizingTypeToString(dst->hpt_resizing));
+ "resizing",
virDomainHPTResizingTypeToString(dst->hpt_resizing),
+ "maxpagesize", dst->hpt_maxpagesize);
return false;
}
break;
@@ -27778,15 +27797,22 @@ virDomainDefFormatInternal(virDomainDefPtr def,
break;
[libvirt] [PATCH 7/7] news: Update for HPT maxpagesize feature
Signed-off-by: Andrea Bolognani --- docs/news.xml | 11 +++ 1 file changed, 11 insertions(+) diff --git a/docs/news.xml b/docs/news.xml index f843648c75..a32d2b88a5 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -94,6 +94,17 @@ independent calls run at the same time. + + + qemu: Allow configuring the page size for HPT pSeries guests + + + For HPT pSeries guests, the size of the host pages used to back guest + memory and the usable guest page sizes are connected; the new setting + can be used to request that a certain page size is available in the + guest. + + -- 2.17.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/7] conf: Reintroduce virDomainDef::hpt_resizing
We're going to introduce a second HPT-related setting soon,
at which point using a single location to store everything is
no longer going to cut it.
This mostly, but not completely, reverts 3dd1eb3b2650.
Signed-off-by: Andrea Bolognani
---
src/conf/domain_conf.c | 21 ++---
src/conf/domain_conf.h | 1 +
src/qemu/qemu_command.c | 7 ---
src/qemu/qemu_domain.c | 2 +-
4 files changed, 20 insertions(+), 11 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index f0b604e125..069ff8c2f8 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -19803,9 +19803,12 @@ virDomainDefParseXML(xmlDocPtr xml,
tmp);
goto error;
}
-def->features[val] = value;
+def->hpt_resizing = (virDomainHPTResizing) value;
VIR_FREE(tmp);
}
+
+if (def->hpt_resizing != VIR_DOMAIN_HPT_RESIZING_NONE)
+def->features[val] = VIR_TRISTATE_SWITCH_ON;
break;
/* coverity[dead_error_begin] */
@@ -21983,13 +21986,16 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr
src,
break;
case VIR_DOMAIN_FEATURE_HPT:
-if (src->features[i] != dst->features[i]) {
+if (src->features[i] != dst->features[i] ||
+src->hpt_resizing != dst->hpt_resizing) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("State of feature '%s' differs: "
- "source: '%s=%s', destination: '%s=%s'"),
+ "source: '%s,%s=%s', destination:
'%s,%s=%s'"),
featureName,
- "resizing",
virDomainHPTResizingTypeToString(src->features[i]),
- "resizing",
virDomainHPTResizingTypeToString(dst->features[i]));
+ virTristateSwitchTypeToString(src->features[i]),
+ "resizing",
virDomainHPTResizingTypeToString(src->hpt_resizing),
+ virTristateSwitchTypeToString(dst->features[i]),
+ "resizing",
virDomainHPTResizingTypeToString(dst->hpt_resizing));
return false;
}
break;
@@ -27767,11 +27773,12 @@ virDomainDefFormatInternal(virDomainDefPtr def,
break;
case VIR_DOMAIN_FEATURE_HPT:
-if (def->features[i] == VIR_DOMAIN_HPT_RESIZING_NONE)
+if (def->features[i] != VIR_TRISTATE_SWITCH_ON ||
+def->hpt_resizing == VIR_DOMAIN_HPT_RESIZING_NONE)
break;
virBufferAsprintf(buf, "\n",
-
virDomainHPTResizingTypeToString(def->features[i]));
+
virDomainHPTResizingTypeToString(def->hpt_resizing));
break;
/* coverity[dead_error_begin] */
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 8cefef535a..39fa2bc35a 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2438,6 +2438,7 @@ struct _virDomainDef {
int kvm_features[VIR_DOMAIN_KVM_LAST];
unsigned int hyperv_spinlocks;
virGICVersion gic_version;
+virDomainHPTResizing hpt_resizing;
char *hyperv_vendor_id;
int apic_eoi;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 446df3e1d8..081314ed0a 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7244,17 +7244,18 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
}
}
-if (def->features[VIR_DOMAIN_FEATURE_HPT] != VIR_DOMAIN_HPT_RESIZING_NONE)
{
+if (def->features[VIR_DOMAIN_FEATURE_HPT] == VIR_TRISTATE_SWITCH_ON) {
const char *str;
-if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_PSERIES_RESIZE_HPT)) {
+if (def->hpt_resizing != VIR_DOMAIN_HPT_RESIZING_NONE &&
+!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_PSERIES_RESIZE_HPT)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("HTP resizing is not supported by this "
"QEMU binary"));
goto cleanup;
}
-str =
virDomainHPTResizingTypeToString(def->features[VIR_DOMAIN_FEATURE_HPT]);
+str = virDomainHPTResizingTypeToString(def->hpt_resizing);
if (!str) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("Invalid setting for HPT resizing"));
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 95c3e3a8aa..6d203e1f2e 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3812,7 +3812,7 @@ qemuDomainDefValidateFeatures(const virDomainDef *def,
break;
case VIR_DOMAIN_FEATURE_HPT:
-if (def->features[i] !=
[libvirt] [PATCH 4/7] conf: Tweak HPT feature parsing and formatting
This doesn't seem very useful at the moment, but it will make
sense once we introduce another HPT-related setting.
The output XML is decoupled from the input XML in preparation
of future changes as well; while doing so, we can shave a few
lines off the latter.
This commit is best viewed with 'git show -w'.
Signed-off-by: Andrea Bolognani
---
src/conf/domain_conf.c| 19 ---
src/qemu/qemu_command.c | 32 ++-
tests/qemuxml2argvdata/pseries-features.xml | 14 ++--
tests/qemuxml2xmloutdata/pseries-features.xml | 29 -
4 files changed, 62 insertions(+), 32 deletions(-)
mode change 12 => 100644 tests/qemuxml2xmloutdata/pseries-features.xml
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 069ff8c2f8..3f7b0d1bfe 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -27258,6 +27258,7 @@ virDomainDefFormatInternal(virDomainDefPtr def,
const char *type = NULL;
int n;
size_t i;
+virBuffer attributeBuf = VIR_BUFFER_INITIALIZER;
virBuffer childrenBuf = VIR_BUFFER_INITIALIZER;
char *netprefix = NULL;
@@ -27773,12 +27774,21 @@ virDomainDefFormatInternal(virDomainDefPtr def,
break;
case VIR_DOMAIN_FEATURE_HPT:
-if (def->features[i] != VIR_TRISTATE_SWITCH_ON ||
-def->hpt_resizing == VIR_DOMAIN_HPT_RESIZING_NONE)
+if (def->features[i] != VIR_TRISTATE_SWITCH_ON)
break;
-virBufferAsprintf(buf, "\n",
-
virDomainHPTResizingTypeToString(def->hpt_resizing));
+virBufferFreeAndReset();
+
+if (def->hpt_resizing != VIR_DOMAIN_HPT_RESIZING_NONE) {
+virBufferAsprintf(,
+ " resizing='%s'",
+
virDomainHPTResizingTypeToString(def->hpt_resizing));
+}
+
+if (virXMLFormatElement(buf, "hpt",
+, NULL) < 0) {
+goto error;
+}
break;
/* coverity[dead_error_begin] */
@@ -28040,6 +28050,7 @@ virDomainDefFormatInternal(virDomainDefPtr def,
error:
virBufferFreeAndReset(buf);
virBufferFreeAndReset();
+virBufferFreeAndReset();
return -1;
}
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 081314ed0a..5cd6d44a88 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7245,24 +7245,26 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
}
if (def->features[VIR_DOMAIN_FEATURE_HPT] == VIR_TRISTATE_SWITCH_ON) {
-const char *str;
-if (def->hpt_resizing != VIR_DOMAIN_HPT_RESIZING_NONE &&
-!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_PSERIES_RESIZE_HPT)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("HTP resizing is not supported by this "
- "QEMU binary"));
-goto cleanup;
-}
+if (def->hpt_resizing != VIR_DOMAIN_HPT_RESIZING_NONE) {
+const char *str;
-str = virDomainHPTResizingTypeToString(def->hpt_resizing);
-if (!str) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("Invalid setting for HPT resizing"));
-goto cleanup;
-}
+if (!virQEMUCapsGet(qemuCaps,
QEMU_CAPS_MACHINE_PSERIES_RESIZE_HPT)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("HTP resizing is not supported by this "
+ "QEMU binary"));
+goto cleanup;
+}
+
+str = virDomainHPTResizingTypeToString(def->hpt_resizing);
+if (!str) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Invalid setting for HPT resizing"));
+goto cleanup;
+}
-virBufferAsprintf(, ",resize-hpt=%s", str);
+virBufferAsprintf(, ",resize-hpt=%s", str);
+}
}
if (cpu && cpu->model &&
diff --git a/tests/qemuxml2argvdata/pseries-features.xml
b/tests/qemuxml2argvdata/pseries-features.xml
index 5dd0dbd0be..5ef1a744c8 100644
--- a/tests/qemuxml2argvdata/pseries-features.xml
+++ b/tests/qemuxml2argvdata/pseries-features.xml
@@ -2,27 +2,17 @@
guest
1ccfd97d-5eb4-478a-bbe6-88d254c16db7
524288
- 524288
1
hvm
-
-
- destroy
- restart
- destroy
/usr/bin/qemu-system-ppc64
-
-
-
-
-
+
+
-
diff --git a/tests/qemuxml2xmloutdata/pseries-features.xml
b/tests/qemuxml2xmloutdata/pseries-features.xml
deleted file mode 12
index 1b01dbace6..00
--- a/tests/qemuxml2xmloutdata/pseries-features.xml
+++
[libvirt] [PATCH 0/7] qemu: Support page size tuning for pSeries guests
This applies cleanly on top of a0d6894af1b1.
Patch 1/7 is too big to go through the list; it can be fetched,
along with the rest of the series, from [1].
Patch 2/7 conflicts with patch 1/3 from [2], but making this one
depend on it didn't feel right; whichever of the two series gets
reviewed and merged first, I'll probably post a rebase of the
other one immediately afterwards.
Changes from [RFC]:
* the QEMU interface has changed based on feedback, which means
some of the code is no longer useful and has been dropped;
* switched to virXMLFormatElement(), as suggested during review;
* added documentation and release notes entry.
[1] https://github.com/andreabolognani/libvirt/tree/pseries-hpt-maxpagesize
[2] https://www.redhat.com/archives/libvir-list/2018-June/msg01374.html
[RFC] https://www.redhat.com/archives/libvir-list/2018-May/msg01710.html
Andrea Bolognani (7):
tests: Add replies for QEMU 3.0.0 on ppc64
qemu: Add capability for the HPT maxpagesize feature
conf: Reintroduce virDomainDef::hpt_resizing
conf: Tweak HPT feature parsing and formatting
conf: Parse and format HPT maxpagesize
qemu: Format HPT maxpagesize on the command line
news: Update for HPT maxpagesize feature
docs/formatdomain.html.in | 11 +-
docs/news.xml | 11 +
docs/schemas/domaincommon.rng | 21 +-
src/conf/domain_conf.c| 60 +-
src/conf/domain_conf.h|2 +
src/qemu/qemu_capabilities.c |8 +
src/qemu/qemu_capabilities.h |1 +
src/qemu/qemu_command.c | 43 +-
src/qemu/qemu_domain.c|2 +-
.../caps_2.12.0.aarch64.replies | 48 +-
.../caps_2.12.0.aarch64.xml |2 +-
.../caps_2.12.0.ppc64.replies | 197 +-
.../caps_2.12.0.ppc64.xml |2 +-
.../caps_2.12.0.s390x.replies | 52 +-
.../caps_2.12.0.s390x.xml |2 +-
.../caps_2.12.0.x86_64.replies| 64 +-
.../caps_2.12.0.x86_64.xml|2 +-
...ppc64.replies => caps_3.0.0.ppc64.replies} | 5268 ++---
..._2.12.0.ppc64.xml => caps_3.0.0.ppc64.xml} | 19 +-
tests/qemucapabilitiestest.c |1 +
tests/qemuxml2argvdata/pseries-features.args |3 +-
tests/qemuxml2argvdata/pseries-features.xml | 18 +-
tests/qemuxml2argvtest.c |1 +
tests/qemuxml2xmloutdata/pseries-features.xml | 31 +-
tests/qemuxml2xmltest.c |1 +
25 files changed, 3624 insertions(+), 2246 deletions(-)
copy tests/qemucapabilitiesdata/{caps_2.12.0.ppc64.replies =>
caps_3.0.0.ppc64.replies} (92%)
copy tests/qemucapabilitiesdata/{caps_2.12.0.ppc64.xml =>
caps_3.0.0.ppc64.xml} (99%)
mode change 12 => 100644 tests/qemuxml2xmloutdata/pseries-features.xml
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/7] qemu: Add capability for the HPT maxpagesize feature
Signed-off-by: Andrea Bolognani
---
src/qemu/qemu_capabilities.c | 8 +
src/qemu/qemu_capabilities.h | 1 +
.../caps_2.12.0.aarch64.replies | 48 ++--
.../caps_2.12.0.aarch64.xml | 2 +-
.../caps_2.12.0.ppc64.replies | 197 +++--
.../caps_2.12.0.ppc64.xml | 2 +-
.../caps_2.12.0.s390x.replies | 52 +++--
.../caps_2.12.0.s390x.xml | 2 +-
.../caps_2.12.0.x86_64.replies| 64 --
.../caps_2.12.0.x86_64.xml| 2 +-
.../caps_3.0.0.ppc64.replies | 207 --
.../qemucapabilitiesdata/caps_3.0.0.ppc64.xml | 3 +-
12 files changed, 497 insertions(+), 91 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 419208ad5c..37c8fbe3d3 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -500,6 +500,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
/* 310 */
"sev-guest",
+ "machine.pseries.cap-hpt-max-page-size",
);
@@ -1428,10 +1429,17 @@ static struct virQEMUCapsStringFlags
virQEMUCapsObjectPropsMemoryBackendFile[] =
{ "discard-data", QEMU_CAPS_OBJECT_MEMORY_FILE_DISCARD },
};
+static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsSPAPRMachine[] = {
+{ "cap-hpt-max-page-size", QEMU_CAPS_MACHINE_PSERIES_CAP_HPT_MAX_PAGE_SIZE
},
+};
+
static virQEMUCapsObjectTypeProps virQEMUCapsObjectProps[] = {
{ "memory-backend-file", virQEMUCapsObjectPropsMemoryBackendFile,
ARRAY_CARDINALITY(virQEMUCapsObjectPropsMemoryBackendFile),
QEMU_CAPS_OBJECT_MEMORY_FILE },
+{ "spapr-machine", virQEMUCapsObjectPropsSPAPRMachine,
+ ARRAY_CARDINALITY(virQEMUCapsObjectPropsSPAPRMachine),
+ -1 },
};
static void
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 3519a194e9..df9bf49abb 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -484,6 +484,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for
syntax-check */
/* 310 */
QEMU_CAPS_SEV_GUEST, /* -object sev-guest,... */
+QEMU_CAPS_MACHINE_PSERIES_CAP_HPT_MAX_PAGE_SIZE, /* -machine
pseries.cap-hpt-max-page-size */
QEMU_CAPS_LAST /* this must always be the last item */
} virQEMUCapsFlags;
diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.replies
b/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.replies
index e0b4f6da38..9a8e54c63d 100644
--- a/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.replies
+++ b/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.replies
@@ -5582,10 +5582,26 @@
}
{
- "execute": "query-machines",
+ "execute": "qom-list-properties",
+ "arguments": {
+"typename": "spapr-machine"
+ },
"id": "libvirt-37"
}
+{
+ "id": "libvirt-37",
+ "error": {
+"class": "DeviceNotFound",
+"desc": "Class 'spapr-machine' not found"
+ }
+}
+
+{
+ "execute": "query-machines",
+ "id": "libvirt-38"
+}
+
{
"return": [
{
@@ -5880,12 +5896,12 @@
"cpu-max": 1
}
],
- "id": "libvirt-37"
+ "id": "libvirt-38"
}
{
"execute": "query-cpu-definitions",
- "id": "libvirt-38"
+ "id": "libvirt-39"
}
{
@@ -6061,35 +6077,35 @@
"static": false
}
],
- "id": "libvirt-38"
+ "id": "libvirt-39"
}
{
"execute": "query-tpm-models",
- "id": "libvirt-39"
+ "id": "libvirt-40"
}
{
"return": [
],
- "id": "libvirt-39"
+ "id": "libvirt-40"
}
{
"execute": "query-tpm-types",
- "id": "libvirt-40"
+ "id": "libvirt-41"
}
{
"return": [
"emulator"
],
- "id": "libvirt-40"
+ "id": "libvirt-41"
}
{
"execute": "query-command-line-options",
- "id": "libvirt-41"
+ "id": "libvirt-42"
}
{
@@ -7250,12 +7266,12 @@
"option": "drive"
}
],
- "id": "libvirt-41"
+ "id": "libvirt-42"
}
{
"execute": "query-migrate-capabilities",
- "id": "libvirt-42"
+ "id": "libvirt-43"
}
{
@@ -7317,12 +7333,12 @@
"capability": "dirty-bitmaps"
}
],
- "id": "libvirt-42"
+ "id": "libvirt-43"
}
{
"execute": "query-qmp-schema",
- "id": "libvirt-43"
+ "id": "libvirt-44"
}
{
@@ -18690,12 +18706,12 @@
"meta-type": "object"
}
],
- "id": "libvirt-43"
+ "id": "libvirt-44"
}
{
"execute": "query-gic-capabilities",
- "id": "libvirt-44"
+ "id": "libvirt-45"
}
{
@@ -18711,7 +18727,7 @@
"kernel": false
}
],
- "id": "libvirt-44"
+ "id": "libvirt-45"
}
{
diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml
b/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml
index 5ed0290397..ecc029f403 100644
--- a/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml
+++ b/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml
@@ -171,7 +171,7 @@
2011090
0
- 347313
+ 347550
v2.12.0-rc0
aarch64
diff --git
[libvirt] [PATCH 1/7] tests: Add replies for QEMU 3.0.0 on ppc64
Signed-off-by: Andrea Bolognani
---
.../caps_3.0.0.ppc64.replies | 23673
.../qemucapabilitiesdata/caps_3.0.0.ppc64.xml | 1074 +
tests/qemucapabilitiestest.c | 1 +
3 files changed, 24748 insertions(+)
create mode 100644 tests/qemucapabilitiesdata/caps_3.0.0.ppc64.replies
create mode 100644 tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml
diff --git a/tests/qemucapabilitiesdata/caps_3.0.0.ppc64.replies
b/tests/qemucapabilitiesdata/caps_3.0.0.ppc64.replies
new file mode 100644
index 00..6aa99733ee
--- /dev/null
+++ b/tests/qemucapabilitiesdata/caps_3.0.0.ppc64.replies
@@ -0,0 +1,23673 @@
+{
+ "execute": "qmp_capabilities",
+ "id": "libvirt-1"
+}
+
+{
+ "return": {
+ },
+ "id": "libvirt-1"
+}
+
+{
+ "execute": "query-version",
+ "id": "libvirt-2"
+}
+
+{
+ "return": {
+"qemu": {
+ "micro": 50,
+ "minor": 12,
+ "major": 2
+},
+"package": "v2.12.0-1689-g518d23a"
+ },
+ "id": "libvirt-2"
+}
[...]
diff --git a/tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml
b/tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml
new file mode 100644
index 00..c98db19815
--- /dev/null
+++ b/tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml
@@ -0,0 +1,1074 @@
+
+ 0
+ 0
+ 0
+
[...]
+
diff --git a/tests/qemucapabilitiestest.c b/tests/qemucapabilitiestest.c
index 633389f263..43f2bcfbc8 100644
--- a/tests/qemucapabilitiestest.c
+++ b/tests/qemucapabilitiestest.c
@@ -182,6 +182,7 @@ mymain(void)
DO_TEST("ppc64", "caps_2.9.0");
DO_TEST("ppc64", "caps_2.10.0");
DO_TEST("ppc64", "caps_2.12.0");
+DO_TEST("ppc64", "caps_3.0.0");
DO_TEST("s390x", "caps_2.7.0");
DO_TEST("s390x", "caps_2.8.0");
DO_TEST("s390x", "caps_2.9.0");
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] appdev-guide/Connections: Remove duplicated entries
On Fri, Jun 22, 2018 at 05:44:18PM -0300, Marcos Paulo de Souza wrote: > On Mon, Jun 25, 2018 at 01:35:33PM +0200, Erik Skultety wrote: > > On Sat, Jun 23, 2018 at 11:32:55AM -0300, Marcos Paulo de Souza wrote: > > > Both /hosts/cpu/arch and /hosts/cpu/features appear twice in the > > > documentation. > > > > > > Signed-off-by: Marcos Paulo de Souza > > > --- > > > > > > This is my first submission to libvirt, so let me know if I forgot > > > something. > > > > Which repo is this patch against? > > This patch is againt the appdev-guide located here: > https://libvirt.org/git/?p=libvirt-appdev-guide.git;a=summary > > The repository seems to be the source of the documenation located in > libvirt.org: > https://libvirt.org/devguide.html > > Is there other place to send patches for appdev-guide? No, but usually we change patch subject prefix to PATCH-project so that it's obvious which project you're sending the patch against, because from the commit subject itself it's not apparent at first sight, especially when noone has touched that git for over 6 years. The document itself was never even finished and regarding the publican we basically it by moving the static content to the main repo. Anyhow, I'm fairly sceptical anyone ever used the development guide solely because of how much it's missing, so I don't know how much sense it makes to push the patch and whether we keep building the repo so that the change would actually be propagated, even though it's correct, but Dan will know the answer for sure, so I'm CCing him. Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] appdev-guide/Connections: Remove duplicated entries
On Mon, Jun 25, 2018 at 01:35:33PM +0200, Erik Skultety wrote: > On Sat, Jun 23, 2018 at 11:32:55AM -0300, Marcos Paulo de Souza wrote: > > Both /hosts/cpu/arch and /hosts/cpu/features appear twice in the > > documentation. > > > > Signed-off-by: Marcos Paulo de Souza > > --- > > > > This is my first submission to libvirt, so let me know if I forgot > > something. > > Which repo is this patch against? This patch is againt the appdev-guide located here: https://libvirt.org/git/?p=libvirt-appdev-guide.git;a=summary The repository seems to be the source of the documenation located in libvirt.org: https://libvirt.org/devguide.html Is there other place to send patches for appdev-guide? > > Erik -- Thanks, Marcos -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1 18/21] util: identity: use VIR_AUTOFREE instead of VIR_FREE for scalar types
On Fri, Jun 08, 2018 at 01:04:40AM +0530, Sukrit Bhatnagar wrote:
> By making use of the GCC's __attribute__((cleanup)) handled by VIR_AUTOFREE
> macro, majority of the VIR_FREE calls can be dropped, which in turn leads to
> getting rid of most of our cleanup sections.
You're missing the SoB line here. Make sure to add that to comply with the
Developer Certificate of Origin, for more info, see
https://libvirt.org/governance.html#contributors
> ---
> src/util/viridentity.c | 54
> --
> 1 file changed, 22 insertions(+), 32 deletions(-)
>
> diff --git a/src/util/viridentity.c b/src/util/viridentity.c
> index 2f4307b..2060dd7 100644
> --- a/src/util/viridentity.c
> +++ b/src/util/viridentity.c
> @@ -133,8 +133,8 @@ int virIdentitySetCurrent(virIdentityPtr ident)
> */
> virIdentityPtr virIdentityGetSystem(void)
> {
> -char *username = NULL;
> -char *groupname = NULL;
> +VIR_AUTOFREE(char *) username = NULL;
> +VIR_AUTOFREE(char *) groupname = NULL;
> unsigned long long startTime;
> virIdentityPtr ret = NULL;
> #if WITH_SELINUX
> @@ -154,14 +154,14 @@ virIdentityPtr virIdentityGetSystem(void)
> goto error;
>
> if (!(username = virGetUserName(geteuid(
> -goto cleanup;
> +return ret;
> if (virIdentitySetUNIXUserName(ret, username) < 0)
> goto error;
> if (virIdentitySetUNIXUserID(ret, getuid()) < 0)
> goto error;
>
> if (!(groupname = virGetGroupName(getegid(
> -goto cleanup;
> +return ret;
> if (virIdentitySetUNIXGroupName(ret, groupname) < 0)
> goto error;
> if (virIdentitySetUNIXGroupID(ret, getgid()) < 0)
> @@ -172,7 +172,7 @@ virIdentityPtr virIdentityGetSystem(void)
> if (getcon() < 0) {
> virReportSystemError(errno, "%s",
> _("Unable to lookup SELinux process
> context"));
> -goto cleanup;
> +return ret;
> }
> if (virIdentitySetSELinuxContext(ret, con) < 0) {
> freecon(con);
> @@ -182,15 +182,9 @@ virIdentityPtr virIdentityGetSystem(void)
> }
> #endif
>
> - cleanup:
> -VIR_FREE(username);
> -VIR_FREE(groupname);
> -return ret;
> -
> error:
> virObjectUnref(ret);
> -ret = NULL;
> -goto cleanup;
> +return NULL;
you're returning NULL on success too, which causes the test suite to fail, make
sure that you run make check -j X && make syntax-check -j X after every patch
in the series.
X - number of logical CPUs + 1
I briefly went through the other VIR_AUTOFREE patches and didn't spot any
problems, I'll have a better look once you post another version of this series
due to the points I raised.
Regards,
Erik
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemu: Fix memory leak in qemuDomainBlockJobSetSpeed()
Subject: [PATCH] qemu: Fix memory leak in qemuDomainBlockJobSetSpeed() fix 'device' leak in qemuDomainBlockJobSetSpeed Signed-off-by: Jie Wang --- src/qemu/qemu_driver.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 921aafcd79..b45c26eb3b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17395,6 +17395,7 @@ qemuDomainBlockJobSetSpeed(virDomainPtr dom, qemuDomainObjEndJob(driver, vm); cleanup: +VIR_FREE(device); virDomainObjEndAPI(); return ret; -- 2.15.0.windows.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/8] backup: Introduce virDomainCheckpointPtr
On 06/13/2018 12:42 PM, Eric Blake wrote:
> Prepare for introducing a bunch of new public APIs related to
> backup checkpoints by first introducing a new internal type
> and errors associated with that type. Checkpoints are modeled
> heavily after virDomainSnapshotPtr (both represent a point in
> time of the guest), although a snapshot exists with the intent
> of rolling back to that state, while a checkpoint exists to
> make it possible to create an incremental backup at a later
> time.
>
> Signed-off-by: Eric Blake
> ---
> include/libvirt/libvirt-domain-snapshot.h | 8 ++--
> include/libvirt/libvirt.h | 2 +
> include/libvirt/virterror.h | 5 ++-
> src/datatypes.c | 62
> ++-
> src/datatypes.h | 31 +++-
> src/libvirt_private.syms | 2 +
> src/util/virerror.c | 15 +++-
> 7 files changed, 118 insertions(+), 7 deletions(-)
>
> diff --git a/include/libvirt/libvirt-domain-snapshot.h
> b/include/libvirt/libvirt-domain-snapshot.h
> index e5a893a767..ff1e890cfc 100644
> --- a/include/libvirt/libvirt-domain-snapshot.h
> +++ b/include/libvirt/libvirt-domain-snapshot.h
> @@ -31,15 +31,17 @@
> /**
> * virDomainSnapshot:
> *
> - * a virDomainSnapshot is a private structure representing a snapshot of
> - * a domain.
> + * A virDomainSnapshot is a private structure representing a snapshot of
> + * a domain. A snapshot captures the state of the domain at a point in
> + * time, with the intent that the guest can be reverted back to that
> + * state at a later time.
> */
> typedef struct _virDomainSnapshot virDomainSnapshot;
>
> /**
> * virDomainSnapshotPtr:
> *
> - * a virDomainSnapshotPtr is pointer to a virDomainSnapshot private
> structure,
> + * A virDomainSnapshotPtr is pointer to a virDomainSnapshot private
> structure,
> * and is the type used to reference a domain snapshot in the API.
> */
> typedef virDomainSnapshot *virDomainSnapshotPtr;
The above hunk is separable (and push-able) as it's own patch, so you
can consider it :
Reviewed-by: John Ferlan
Naming scheme aside, the rest had one minor nit:
[...]
> diff --git a/src/util/virerror.c b/src/util/virerror.c
> index 93632dbdf7..1e6fd77abf 100644
> --- a/src/util/virerror.c
> +++ b/src/util/virerror.c
> @@ -1,7 +1,7 @@
> /*
> * virerror.c: error handling and reporting code for libvirt
> *
> - * Copyright (C) 2006, 2008-2016 Red Hat, Inc.
> + * Copyright (C) 2006, 2008-2018 Red Hat, Inc.
> *
> * This library is free software; you can redistribute it and/or
> * modify it under the terms of the GNU Lesser General Public
> @@ -140,6 +140,7 @@ VIR_ENUM_IMPL(virErrorDomain, VIR_ERR_DOMAIN_LAST,
>"Perf", /* 65 */
>"Libssh transport layer",
>"Resource control",
> + "Domain Checkpoint",
> )
>
>
> @@ -1494,6 +1495,18 @@ virErrorMsg(virErrorNumber error, const char *info)
> else
> errmsg = _("device not found: %s");
> break;
> +case VIR_ERR_INVALID_DOMAIN_CHECKPOINT:
> +if (info == NULL)
> +errmsg = _("Invalid checkpoint");
> +else
> +errmsg = _("Invalid checkpoint: %s");
> +break;
> +case VIR_ERR_NO_DOMAIN_CHECKPOINT:
> +if (info == NULL)
> +errmsg = _("Domain snapshot not found");
> +else
> +errmsg = _("Domain snapshot not found: %s");
checkpoint
> +break;
> }
> return errmsg;
> }
>
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/8] backup: Document nuances between different state capture APIs
On 06/13/2018 12:42 PM, Eric Blake wrote: > Upcoming patches will add support for incremental backups via > a new API; but first, we need a landing page that gives an > overview of capturing various pieces of guest state, and which > APIs are best suited to which tasks. > > Signed-off-by: Eric Blake > --- > docs/docs.html.in | 5 ++ > docs/domainstatecapture.html.in | 190 > > docs/formatsnapshot.html.in | 2 + > 3 files changed, 197 insertions(+) > create mode 100644 docs/domainstatecapture.html.in > This got a lot messier than originally intended. As noted in my response for .1 - I haven't really followed the discussions thus far - so take it with that viewpoint - someone from outside the current discussion trying to make sense of what this topic is all about. > diff --git a/docs/docs.html.in b/docs/docs.html.in > index 40e0e3b82e..4c46b74980 100644 > --- a/docs/docs.html.in > +++ b/docs/docs.html.in > @@ -120,6 +120,11 @@ > > Secure usage > Secure usage of the libvirt APIs > + > +Domain state > +capture > +Comparison between different methods of capturing domain > + state > > > > diff --git a/docs/domainstatecapture.html.in b/docs/domainstatecapture.html.in > new file mode 100644 > index 00..00ab7e8ee1 > --- /dev/null > +++ b/docs/domainstatecapture.html.in > @@ -0,0 +1,190 @@ > + > + > +http://www.w3.org/1999/xhtml;> > + > + > +Domain state capture using Libvirt > + > + > + > + > + This page compares the different means for capturing state > + related to a domain managed by libvirt, in order to aid > + application developers to choose which operations best suit > + their needs. I would alter the sentence at the comma... IOW: In order to aid ... their needs, this page compares ... by libvirt. Then rather than discussing this below - I think we really need to state right at the top the following: The information here is primarily geared towards capturing the state of the active domain. Capturing state of an inactive domain essentially only requires the contents of the guest disks and then restoring that state is merely a fresh boot with the disks restored to that state. There are aspects of the subsequent functionality that cover the inactive state collection, but it's not the primary focus. > + > + > +State capture trade-offs > + > +One of the features made possible with virtual machines is live > + migration, or transferring all state related to the guest from > + one host to another, with minimal interruption to the guest's to me the commas are unnecessary. > + activity. A clever observer will then note that if all state is s/activity./activity. In this case, state includes domain memory including the current instruction stream and domain storage, whether that is local virtual disks which are not present on a target host or networked storage being updated by the local hypervisor. A clever... [BTW: In rereading my response - I almost want to add - "As it relates to domain checkpoints and backups, state only includes disk state change.". However, I'm not sure if that ties in yet or not. I think it only matters for the two new API's being discussed.] > + available for live migration, there is nothing stopping a user , then there is... > + from saving that state at a given point of time, to be able to s/,/ in order/ > + later rewind guest execution back to the state it previously > + had. There are several different libvirt APIs associated with [BTW: The following includes something else I pulled up from the list below...] s/had. /had. The astute reader will also realize that state capture at any level requires that the data must be stored and managed by some mechanism. This processing may be to a single file or some set of chained files. This is the inflection point between where Libvirt would (could, should?) integrate with third party tools that are built around managing the volume of data possibly generated by multiple domains with multiple disks. This leaves the task of synchronizing the capture algorithms to Libvirt in order to be able to work seamlessly with the underlying hypervisor. There are several libvirt APIs associated with ... (different is superfluous) > + capturing the state of a guest, such that the captured state can s/, such that the captured state/which > + later be used to rewind that guest to the conditions it was in > + earlier. But since there are multiple APIs, it is best to > + understand the tradeoffs and differences between them, in order> + > to choose the best API for a given task. s/But since ... given task./The following is a list of trade-offs and differences between the various facets that affect capturing domain state for active domains:/ > + > + > + > + Timing "Data
Re: [libvirt] [dbus PATCH] connect: fix deregistering of NodeDevice events
On Mon, Jun 25, 2018 at 09:30:49AM -0400, Anya Harter wrote:
> Signed-off-by: Anya Harter
> ---
> src/connect.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/connect.c b/src/connect.c
> index 09e5628..32dda14 100644
> --- a/src/connect.c
> +++ b/src/connect.c
> @@ -68,7 +68,7 @@ virtDBusConnectClose(virtDBusConnect *connect,
> for (gint i = 0; i < VIR_NODE_DEVICE_EVENT_ID_LAST; i++) {
> if (connect->nodeDevCallbackIds[i] >= 0) {
> if (deregisterEvents) {
> -virConnectNetworkEventDeregisterAny(connect->connection,
> +virConnectNodeDeviceEventDeregisterAny(connect->connection,
>
> connect->nodeDevCallbackIds[i]);
Indentation is off here. Otherwise looks good.
With this adjusted.
Reviewed-by: Katerina Koukiou
> }
> connect->nodeDevCallbackIds[i] = -1;
> --
> 2.17.1
>
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemu: monitor: Fix memory leak in qemuMonitorJSONNBDServerStart()
Subject: [PATCH] qemu: monitor: Fix memory leak in
qemuMonitorJSONNBDServerStart()
Exiting early through the return path did result in 'port_str'
being leaked.
Signed-off-by: Jie Wang
---
src/qemu/qemu_monitor_json.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index aa89ea7056..3e90279b71 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -6540,7 +6540,7 @@ qemuMonitorJSONNBDServerStart(qemuMonitorPtr mon,
return ret;
if (!(addr = qemuMonitorJSONBuildInetSocketAddress(host, port_str)))
-return ret;
+goto cleanup;
if (!(cmd = qemuMonitorJSONMakeCommand("nbd-server-start",
"a:addr", ,
--
2.15.0.windows.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [dbus PATCH] connect: fix deregistering of NodeDevice events
Signed-off-by: Anya Harter
---
src/connect.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/connect.c b/src/connect.c
index 09e5628..32dda14 100644
--- a/src/connect.c
+++ b/src/connect.c
@@ -68,7 +68,7 @@ virtDBusConnectClose(virtDBusConnect *connect,
for (gint i = 0; i < VIR_NODE_DEVICE_EVENT_ID_LAST; i++) {
if (connect->nodeDevCallbackIds[i] >= 0) {
if (deregisterEvents) {
-virConnectNetworkEventDeregisterAny(connect->connection,
+virConnectNodeDeviceEventDeregisterAny(connect->connection,
connect->nodeDevCallbackIds[i]);
}
connect->nodeDevCallbackIds[i] = -1;
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PULL 25/26] block: Remove deprecated -drive option serial
On Mon, Jun 25, 2018 at 13:41:06 +0200, Kevin Wolf wrote: > Am 25.06.2018 um 11:53 hat Daniel P. Berrangé geschrieben: > > On Fri, Jun 22, 2018 at 03:31:46PM +0100, Daniel P. Berrangé wrote: > > > On Fri, Jun 22, 2018 at 04:25:13PM +0200, Kevin Wolf wrote: > > > > Am 22.06.2018 um 15:36 hat Christian Borntraeger geschrieben: [...] > > Thanks! > > I'll look into werror/rerror support for usb-storage. It shouldn't be > too hard, though it's strictly speaking a separate problem related to > using -blockdev rather than option deprecation. > > If Peter wants to wait for QEMU support before converting werror/rerror Definitely. I don't want to keep around yet another hack that will satisfy one specific case and then add another capability for it. We should then gate the moving of the feature based on the presence of werror for usb-storage. > to -device, maybe it would make sense to split your patch for v2 so that > geometry and serial can get fixed right away? Yes this can be done right away. signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PULL 25/26] block: Remove deprecated -drive option serial
Am 25.06.2018 um 11:53 hat Daniel P. Berrangé geschrieben: > On Fri, Jun 22, 2018 at 03:31:46PM +0100, Daniel P. Berrangé wrote: > > On Fri, Jun 22, 2018 at 04:25:13PM +0200, Kevin Wolf wrote: > > > Am 22.06.2018 um 15:36 hat Christian Borntraeger geschrieben: > > > > > > > > > > > > On 06/22/2018 02:55 PM, Kevin Wolf wrote: > > > > > Am 22.06.2018 um 13:38 hat Christian Borntraeger geschrieben: > > > > >> > > > > >> On 06/15/2018 04:21 PM, Kevin Wolf wrote: > > > > >>> The -drive option serial was deprecated in QEMU 2.10. It's time to > > > > >>> remove it. > > > > >>> > > > > >>> Tests need to be updated to set the serial number with -global > > > > >>> instead > > > > >>> of using the -drive option. > > > > >> > > > > >> libvirt 4.5 still creates those (at least on s390x) > > > > >> > > > > >> > > > > >>> > > >> iothread='1'/> > > > > >> > > > > >> > > > > >> skel > > > > >> > > > > >> > > > > >> > > > > >> > > > > >> > > > > >> -> > > > > >> [...] > > > > >> -drive > > > > >> file=/var/lib/libvirt/qemu/image.zhyp137,format=qcow2,if=none,id=drive-virtio-disk0,serial=skel,cache=none,aio=native > > > > >> -device > > > > >> virtio-blk-ccw,iothread=iothread1,scsi=off,devno=fe.0.,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1,write-cache=on > > > > >> > > > > >> [...] > > > > >> > > > > >> 2018-06-22T11:25:20.946024Z qemu-system-s390x: -drive > > > > >> file=/var/lib/libvirt/qemu/image.zhyp137,format=qcow2,if=none,id=drive-virtio-disk0,serial=skel,cache=none,aio=native: > > > > >> Block format 'qcow2' does not support the option 'serial' > > > > >> 2018-06-22 11:25:21.098+: shutting down, reason=failed > > > > >> > > > > >> So it seems that this breaks s390x. > > > > > > > > To me it seems that this is also broken on x86. > > > > > > > > > > Thanks for bringing this up. libvirt should fix this before QEMU 3.0 > > > > > is > > > > > released. > > > > > > > > I think this is definitely too short notice. We should not break > > > > existing > > > > setups just by insisting that users have to update libvirt when they > > > > update > > > > QEMU. Yes, this might be our policy, but doing so "just because we can" > > > > is certainly a very bad attitude. I see no fundamental technical reason > > > > why > > > > we should not revert this change. > > > > > > This was in fact one release longer than our deprecation policy says. > > > Are we serious about the deprecation policy or aren't we? > > > > > > I might consider reverting a change if it turned out that this requires > > > some massive work in libvirt. But I think this one should be rather easy > > > to fix in libvirt until 3.0 is released. > > > > It is probably even possible for us to fix it in our July 1st > > release > > Fix posted here: > > https://www.redhat.com/archives/libvir-list/2018-June/msg01598.html Thanks! I'll look into werror/rerror support for usb-storage. It shouldn't be too hard, though it's strictly speaking a separate problem related to using -blockdev rather than option deprecation. If Peter wants to wait for QEMU support before converting werror/rerror to -device, maybe it would make sense to split your patch for v2 so that geometry and serial can get fixed right away? Kevin -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] appdev-guide/Connections: Remove duplicated entries
On Sat, Jun 23, 2018 at 11:32:55AM -0300, Marcos Paulo de Souza wrote: > Both /hosts/cpu/arch and /hosts/cpu/features appear twice in the > documentation. > > Signed-off-by: Marcos Paulo de Souza > --- > > This is my first submission to libvirt, so let me know if I forgot something. Which repo is this patch against? Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PULL 25/26] block: Remove deprecated -drive option serial
On Fri, Jun 22, 2018 at 07:54:00PM +0200, Kevin Wolf wrote: > Am 22.06.2018 um 17:40 hat Daniel P. Berrangé geschrieben: > > On Fri, Jun 22, 2018 at 04:25:13PM +0200, Kevin Wolf wrote: > > > This was in fact one release longer than our deprecation policy says. > > > Are we serious about the deprecation policy or aren't we? > > > > > > I might consider reverting a change if it turned out that this requires > > > some massive work in libvirt. But I think this one should be rather easy > > > to fix in libvirt until 3.0 is released. > > > > I've got a patch mostly ready that converts libvirt to setting these things > > on the frontend device, however, I've got some queries... > > > > - usb-storage - doesn't appear to support geometry or werror/rerror > > > >Will we loose functionality by stopping use of -drive for werror > > > >Loosing geometry feels relevant too, unless it was already ignored > >when set opf -drive ? > > You're right, usb-storage doesn't allow using -device to specify these, > and it does use the values from -drive. > > For werror/rerror, we should clearly implement the option forwarding to > scsi-disk so that you can make use of it. This missing werror item looks like a blocker for libvirt to be able to switch to using -blockdev without a regression. > I'm not sure how sane specifying a non-standard CHS geometry for a USB > stick actually is. As an additional difficulty, usb-storage internally > creates a scsi-disk device (not scsi-hd), which is also considered > legacy and doesn't support the geometry options either, so it's not just > simple forwarding. We removed an actual feature there, but that feature > was probably never intended nor used. > > If someone comes up with a compelling reason why they really need to > configure the CHS geometry of their USB sticks, I guess we can do it. My > real USB sticks I tested don't even support MODE_PAGE_HD_GEOMETRY > (though they have MODE_PAGE_FLEXIBLE_DISK_GEOMETRY). I don't think libvirt has a compelling reason. The ability to set CHS on usb-storage is just something we got for free because it used the same -drive syntax as other disk device frontends. It would be nice to avoid the regression but I doubt people will actually notice in practice as usb-storage users are few & far between, and even fewer of those will care about CHS. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: format serial, geometry and error policy on frontend disk device
On Mon, Jun 25, 2018 at 11:37:07 +0100, Daniel Berrange wrote: > On Mon, Jun 25, 2018 at 12:29:48PM +0200, Peter Krempa wrote: > > On Mon, Jun 25, 2018 at 10:50:32 +0100, Daniel Berrange wrote: > > > Currently we format the serial, geometry and error policy on the -drive > > > backend argument. > > > > > > QEMU added the ability to set serial and geometry on the frontend in > > > the 1.2 release, and ability to set error policy in the 2.7 release. > > > usb-storage is an oddity, however, as it lacks error policy support > > > right now. > > > > > > Setting any of these on -drive has been deprecated and support for this > > > will be removed very soon. > > > > > > Note that some disk buses (sd) still don't support -device. Although > > > QEMU allowed these properties to be set on -drive for if=sd, they > > > have been ignored so we don't loose anything by removing this. > > > > > > Signed-off-by: Daniel P. Berrangé > > > > @@ -2125,6 +2141,11 @@ qemuBuildDriveDevStr(const virDomainDef *def, > > > if (qemuBuildDriveDevCacheStr(disk, , qemuCaps) < 0) > > > goto error; > > > > > > +qemuBuildDiskFrontendAttributes(disk, ); > > > +if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DISK_ERROR) && > > > +disk->bus != VIR_DOMAIN_DISK_BUS_USB) > > > > Why is USB special here? Switching to -blockdev will require doing this > > also for the USB disk, so I'd rather have a single case here. > > We have no choice - QEMU didn't add it > > $ for i in virtio-blk ide-hd scsi-hd usb-storage ; do > ./x86_64-softmmu/qemu-system-x86_64 -device $i,? 2>&1 | grep werror ; done > virtio-blk-pci.werror=BlockdevOnError (Error handling policy, > report/ignore/enospc/stop/auto) > ide-hd.werror=BlockdevOnError (Error handling policy, > report/ignore/enospc/stop/auto) > scsi-hd.werror=BlockdevOnError (Error handling policy, > report/ignore/enospc/stop/auto) Hmm, thats awesome. If they intend to support it for the usb-storage device they should add it prior to us converting it to the new place. That will also block enabling of -blockdev signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: format serial, geometry and error policy on frontend disk device
On Mon, Jun 25, 2018 at 12:29:48PM +0200, Peter Krempa wrote: > On Mon, Jun 25, 2018 at 10:50:32 +0100, Daniel Berrange wrote: > > Currently we format the serial, geometry and error policy on the -drive > > backend argument. > > > > QEMU added the ability to set serial and geometry on the frontend in > > the 1.2 release, and ability to set error policy in the 2.7 release. > > usb-storage is an oddity, however, as it lacks error policy support > > right now. > > > > Setting any of these on -drive has been deprecated and support for this > > will be removed very soon. > > > > Note that some disk buses (sd) still don't support -device. Although > > QEMU allowed these properties to be set on -drive for if=sd, they > > have been ignored so we don't loose anything by removing this. > > > > Signed-off-by: Daniel P. Berrangé > > @@ -2125,6 +2141,11 @@ qemuBuildDriveDevStr(const virDomainDef *def, > > if (qemuBuildDriveDevCacheStr(disk, , qemuCaps) < 0) > > goto error; > > > > +qemuBuildDiskFrontendAttributes(disk, ); > > +if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DISK_ERROR) && > > +disk->bus != VIR_DOMAIN_DISK_BUS_USB) > > Why is USB special here? Switching to -blockdev will require doing this > also for the USB disk, so I'd rather have a single case here. We have no choice - QEMU didn't add it $ for i in virtio-blk ide-hd scsi-hd usb-storage ; do ./x86_64-softmmu/qemu-system-x86_64 -device $i,? 2>&1 | grep werror ; done virtio-blk-pci.werror=BlockdevOnError (Error handling policy, report/ignore/enospc/stop/auto) ide-hd.werror=BlockdevOnError (Error handling policy, report/ignore/enospc/stop/auto) scsi-hd.werror=BlockdevOnError (Error handling policy, report/ignore/enospc/stop/auto) Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PULL 25/26] block: Remove deprecated -drive option serial
On 25 June 2018 at 11:31, Peter Krempa wrote: > On Mon, Jun 25, 2018 at 11:01:46 +0100, Peter Maydell wrote: >> On 22 June 2018 at 16:40, Daniel P. Berrangé wrote: >> > - SD card - requires -drive with if=sd, no -device support AFAICT >> >> Hmm? You can use -device if you want: >> -drive if=none,id=mydrive,... -device sd-card,...,drive=mydrive >> (the sd-card device then plugs into the sd controller device >> via the sd-bus bus). >> >> Most command lines floating around on the web use if=sd, though >> (which doesn't require explicit creation of the sd-card device.) > > When I've spoke with Markus during the KVM forum he found out that some > machine types still don't support that, so that why libvirt didn't > bother converting it yet. Yes, it's a bit of a stalled-in-progress transition... thanks -- PMM -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PULL 25/26] block: Remove deprecated -drive option serial
On Mon, Jun 25, 2018 at 11:01:46 +0100, Peter Maydell wrote: > On 22 June 2018 at 16:40, Daniel P. Berrangé wrote: > > - SD card - requires -drive with if=sd, no -device support AFAICT > > Hmm? You can use -device if you want: > -drive if=none,id=mydrive,... -device sd-card,...,drive=mydrive > (the sd-card device then plugs into the sd controller device > via the sd-bus bus). > > Most command lines floating around on the web use if=sd, though > (which doesn't require explicit creation of the sd-card device.) When I've spoke with Markus during the KVM forum he found out that some machine types still don't support that, so that why libvirt didn't bother converting it yet. signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: format serial, geometry and error policy on frontend disk device
On Mon, Jun 25, 2018 at 10:50:32 +0100, Daniel Berrange wrote:
> Currently we format the serial, geometry and error policy on the -drive
> backend argument.
>
> QEMU added the ability to set serial and geometry on the frontend in
> the 1.2 release, and ability to set error policy in the 2.7 release.
> usb-storage is an oddity, however, as it lacks error policy support
> right now.
>
> Setting any of these on -drive has been deprecated and support for this
> will be removed very soon.
>
> Note that some disk buses (sd) still don't support -device. Although
> QEMU allowed these properties to be set on -drive for if=sd, they
> have been ignored so we don't loose anything by removing this.
>
> Signed-off-by: Daniel P. Berrangé
> ---
> src/qemu/qemu_capabilities.c | 2 ++
> src/qemu/qemu_capabilities.h | 1 +
> src/qemu/qemu_command.c | 31 ---
> .../caps_2.10.0.aarch64.xml | 1 +
> .../caps_2.10.0.ppc64.xml | 1 +
> .../caps_2.10.0.s390x.xml | 1 +
> .../caps_2.10.0.x86_64.xml| 1 +
> .../caps_2.11.0.s390x.xml | 1 +
> .../caps_2.12.0.aarch64.xml | 1 +
> .../caps_2.12.0.ppc64.xml | 1 +
> .../caps_2.12.0.s390x.xml | 1 +
> .../caps_2.12.0.x86_64.xml| 1 +
> .../qemucapabilitiesdata/caps_2.7.0.s390x.xml | 1 +
> .../caps_2.7.0.x86_64.xml | 1 +
> .../qemucapabilitiesdata/caps_2.8.0.s390x.xml | 1 +
> .../caps_2.8.0.x86_64.xml | 1 +
> .../qemucapabilitiesdata/caps_2.9.0.ppc64.xml | 1 +
> .../qemucapabilitiesdata/caps_2.9.0.s390x.xml | 1 +
> .../caps_2.9.0.x86_64.xml | 1 +
> .../disk-drive-network-tlsx509.args | 12 +++
> .../disk-drive-network-vxhs.args | 4 +--
> tests/qemuxml2argvdata/disk-drive-shared.args | 5 +--
> tests/qemuxml2argvdata/disk-geometry.args | 6 ++--
> tests/qemuxml2argvdata/disk-ide-wwn.args | 5 ++-
> .../qemuxml2argvdata/disk-scsi-disk-wwn.args | 4 +--
> tests/qemuxml2argvdata/disk-serial.args | 10 +++---
> tests/qemuxml2argvdata/disk-serial.xml| 1 -
> tests/qemuxml2argvtest.c | 1 +
> tests/qemuxml2xmloutdata/disk-serial.xml | 1 -
> 29 files changed, 69 insertions(+), 30 deletions(-)
>
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 419208ad5c..b9a6c2d612 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -500,6 +500,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
>
>/* 310 */
>"sev-guest",
> + "disk-error",
> );
>
>
> @@ -1162,6 +1163,7 @@ static struct virQEMUCapsStringFlags
> virQEMUCapsDevicePropsVirtioBlk[] = {
> { "iommu_platform", QEMU_CAPS_VIRTIO_PCI_IOMMU_PLATFORM },
> { "ats", QEMU_CAPS_VIRTIO_PCI_ATS },
> { "write-cache", QEMU_CAPS_DISK_WRITE_CACHE },
> +{ "werror", QEMU_CAPS_DISK_ERROR },
> };
Please split-off capability changes into a separate commit as it's
usual.
>
> static struct virQEMUCapsStringFlags virQEMUCapsDevicePropsVirtioNet[] = {
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index 3519a194e9..1245fb46cf 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -484,6 +484,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for
> syntax-check */
>
> /* 310 */
> QEMU_CAPS_SEV_GUEST, /* -object sev-guest,... */
> +QEMU_CAPS_DISK_ERROR, /* -device $DISK,werror=,rerror=X */
>
> QEMU_CAPS_LAST /* this must always be the last item */
> } virQEMUCapsFlags;
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 446df3e1d8..e97158fe64 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1621,8 +1621,6 @@ qemuBuildDiskFrontendAttributes(virDomainDiskDefPtr
> disk,
> virBufferAddLit(buf, ",serial=");
> virBufferEscape(buf, '\\', " ", "%s", disk->serial);
> }
> -
> -qemuBuildDiskFrontendAttributeErrorPolicy(disk, buf);
> }
>
>
> @@ -1662,11 +1660,29 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
> virBufferAsprintf(, "if=%s",
>virDomainDiskQEMUBusTypeToString(disk->bus));
> virBufferAsprintf(, ",index=%d", idx);
> +
> +if (disk->serial) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Serial property not supported for drive bus
> '%s'"),
> + virDomainDiskBusTypeToString(disk->bus));
> +goto error;
> +}
> +if (disk->geometry.cylinders > 0 &&
> +disk->geometry.heads > 0 &&
> +disk->geometry.sectors > 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +
Re: [libvirt] [PATCH] formatdomain.html.in: Amend the 'random' RNG backend section
On Mon, Jun 25, 2018 at 11:10:29AM +0200, Kashyap Chamarthy wrote: > On Mon, Jun 25, 2018 at 10:57:31AM +0200, Erik Skultety wrote: > > On Fri, Jun 22, 2018 at 12:09:39PM +0200, Kashyap Chamarthy wrote: > > > Since libvirt 1.3.4, any RNG source is accepted for the 'random' > > > backend. However, '/dev/urandom' is the _recommended_ source of > > > entropy. > > > > > > > s/\n// > > > > > Mention it so in the docs. > > > > "Therefore we should mention that in the docs." > > > > With that > > Reviewed-by: Erik Skultety > > Whoever is pushing the ptach can amend that I hope. > > Thanks! Ah, I didn't realize you don't have commit permissions, I'll take care of that. Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PULL 25/26] block: Remove deprecated -drive option serial
On 22 June 2018 at 16:40, Daniel P. Berrangé wrote: > - SD card - requires -drive with if=sd, no -device support AFAICT Hmm? You can use -device if you want: -drive if=none,id=mydrive,... -device sd-card,...,drive=mydrive (the sd-card device then plugs into the sd controller device via the sd-bus bus). Most command lines floating around on the web use if=sd, though (which doesn't require explicit creation of the sd-card device.) I think one or two controllers might not yet have been converted to use sd-bus. thanks -- PMM -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PULL 25/26] block: Remove deprecated -drive option serial
On Fri, Jun 22, 2018 at 03:31:46PM +0100, Daniel P. Berrangé wrote: > On Fri, Jun 22, 2018 at 04:25:13PM +0200, Kevin Wolf wrote: > > Am 22.06.2018 um 15:36 hat Christian Borntraeger geschrieben: > > > > > > > > > On 06/22/2018 02:55 PM, Kevin Wolf wrote: > > > > Am 22.06.2018 um 13:38 hat Christian Borntraeger geschrieben: > > > >> > > > >> On 06/15/2018 04:21 PM, Kevin Wolf wrote: > > > >>> The -drive option serial was deprecated in QEMU 2.10. It's time to > > > >>> remove it. > > > >>> > > > >>> Tests need to be updated to set the serial number with -global instead > > > >>> of using the -drive option. > > > >> > > > >> libvirt 4.5 still creates those (at least on s390x) > > > >> > > > >> > > > >>> > >> iothread='1'/> > > > >> > > > >> > > > >> skel > > > >> > > > >> > > > >> > > > >> > > > >> > > > >> -> > > > >> [...] > > > >> -drive > > > >> file=/var/lib/libvirt/qemu/image.zhyp137,format=qcow2,if=none,id=drive-virtio-disk0,serial=skel,cache=none,aio=native > > > >> -device > > > >> virtio-blk-ccw,iothread=iothread1,scsi=off,devno=fe.0.,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1,write-cache=on > > > >> > > > >> [...] > > > >> > > > >> 2018-06-22T11:25:20.946024Z qemu-system-s390x: -drive > > > >> file=/var/lib/libvirt/qemu/image.zhyp137,format=qcow2,if=none,id=drive-virtio-disk0,serial=skel,cache=none,aio=native: > > > >> Block format 'qcow2' does not support the option 'serial' > > > >> 2018-06-22 11:25:21.098+: shutting down, reason=failed > > > >> > > > >> So it seems that this breaks s390x. > > > > > > To me it seems that this is also broken on x86. > > > > > > > > Thanks for bringing this up. libvirt should fix this before QEMU 3.0 is > > > > released. > > > > > > I think this is definitely too short notice. We should not break existing > > > setups just by insisting that users have to update libvirt when they > > > update > > > QEMU. Yes, this might be our policy, but doing so "just because we can" > > > is certainly a very bad attitude. I see no fundamental technical reason > > > why > > > we should not revert this change. > > > > This was in fact one release longer than our deprecation policy says. > > Are we serious about the deprecation policy or aren't we? > > > > I might consider reverting a change if it turned out that this requires > > some massive work in libvirt. But I think this one should be rather easy > > to fix in libvirt until 3.0 is released. > > It is probably even possible for us to fix it in our July 1st > release Fix posted here: https://www.redhat.com/archives/libvir-list/2018-June/msg01598.html Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemu: format serial, geometry and error policy on frontend disk device
Currently we format the serial, geometry and error policy on the -drive
backend argument.
QEMU added the ability to set serial and geometry on the frontend in
the 1.2 release, and ability to set error policy in the 2.7 release.
usb-storage is an oddity, however, as it lacks error policy support
right now.
Setting any of these on -drive has been deprecated and support for this
will be removed very soon.
Note that some disk buses (sd) still don't support -device. Although
QEMU allowed these properties to be set on -drive for if=sd, they
have been ignored so we don't loose anything by removing this.
Signed-off-by: Daniel P. Berrangé
---
src/qemu/qemu_capabilities.c | 2 ++
src/qemu/qemu_capabilities.h | 1 +
src/qemu/qemu_command.c | 31 ---
.../caps_2.10.0.aarch64.xml | 1 +
.../caps_2.10.0.ppc64.xml | 1 +
.../caps_2.10.0.s390x.xml | 1 +
.../caps_2.10.0.x86_64.xml| 1 +
.../caps_2.11.0.s390x.xml | 1 +
.../caps_2.12.0.aarch64.xml | 1 +
.../caps_2.12.0.ppc64.xml | 1 +
.../caps_2.12.0.s390x.xml | 1 +
.../caps_2.12.0.x86_64.xml| 1 +
.../qemucapabilitiesdata/caps_2.7.0.s390x.xml | 1 +
.../caps_2.7.0.x86_64.xml | 1 +
.../qemucapabilitiesdata/caps_2.8.0.s390x.xml | 1 +
.../caps_2.8.0.x86_64.xml | 1 +
.../qemucapabilitiesdata/caps_2.9.0.ppc64.xml | 1 +
.../qemucapabilitiesdata/caps_2.9.0.s390x.xml | 1 +
.../caps_2.9.0.x86_64.xml | 1 +
.../disk-drive-network-tlsx509.args | 12 +++
.../disk-drive-network-vxhs.args | 4 +--
tests/qemuxml2argvdata/disk-drive-shared.args | 5 +--
tests/qemuxml2argvdata/disk-geometry.args | 6 ++--
tests/qemuxml2argvdata/disk-ide-wwn.args | 5 ++-
.../qemuxml2argvdata/disk-scsi-disk-wwn.args | 4 +--
tests/qemuxml2argvdata/disk-serial.args | 10 +++---
tests/qemuxml2argvdata/disk-serial.xml| 1 -
tests/qemuxml2argvtest.c | 1 +
tests/qemuxml2xmloutdata/disk-serial.xml | 1 -
29 files changed, 69 insertions(+), 30 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 419208ad5c..b9a6c2d612 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -500,6 +500,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
/* 310 */
"sev-guest",
+ "disk-error",
);
@@ -1162,6 +1163,7 @@ static struct virQEMUCapsStringFlags
virQEMUCapsDevicePropsVirtioBlk[] = {
{ "iommu_platform", QEMU_CAPS_VIRTIO_PCI_IOMMU_PLATFORM },
{ "ats", QEMU_CAPS_VIRTIO_PCI_ATS },
{ "write-cache", QEMU_CAPS_DISK_WRITE_CACHE },
+{ "werror", QEMU_CAPS_DISK_ERROR },
};
static struct virQEMUCapsStringFlags virQEMUCapsDevicePropsVirtioNet[] = {
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 3519a194e9..1245fb46cf 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -484,6 +484,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for
syntax-check */
/* 310 */
QEMU_CAPS_SEV_GUEST, /* -object sev-guest,... */
+QEMU_CAPS_DISK_ERROR, /* -device $DISK,werror=,rerror=X */
QEMU_CAPS_LAST /* this must always be the last item */
} virQEMUCapsFlags;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 446df3e1d8..e97158fe64 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1621,8 +1621,6 @@ qemuBuildDiskFrontendAttributes(virDomainDiskDefPtr disk,
virBufferAddLit(buf, ",serial=");
virBufferEscape(buf, '\\', " ", "%s", disk->serial);
}
-
-qemuBuildDiskFrontendAttributeErrorPolicy(disk, buf);
}
@@ -1662,11 +1660,29 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
virBufferAsprintf(, "if=%s",
virDomainDiskQEMUBusTypeToString(disk->bus));
virBufferAsprintf(, ",index=%d", idx);
+
+if (disk->serial) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Serial property not supported for drive bus
'%s'"),
+ virDomainDiskBusTypeToString(disk->bus));
+goto error;
+}
+if (disk->geometry.cylinders > 0 &&
+disk->geometry.heads > 0 &&
+disk->geometry.sectors > 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Geometry not supported for drive bus '%s'"),
+ virDomainDiskBusTypeToString(disk->bus));
+goto error;
+}
}
-/* Format attributes for the drive itself (not the storage backing it)
which
- * we've formatted historically with -drive */
-qemuBuildDiskFrontendAttributes(disk, );
+/*
Re: [libvirt] [PATCH] formatdomain.html.in: Amend the 'random' RNG backend section
On Mon, Jun 25, 2018 at 10:57:31AM +0200, Erik Skultety wrote: > On Fri, Jun 22, 2018 at 12:09:39PM +0200, Kashyap Chamarthy wrote: > > Since libvirt 1.3.4, any RNG source is accepted for the 'random' > > backend. However, '/dev/urandom' is the _recommended_ source of > > entropy. > > > > s/\n// > > > Mention it so in the docs. > > "Therefore we should mention that in the docs." > > With that > Reviewed-by: Erik Skultety Whoever is pushing the ptach can amend that I hope. Thanks! -- /kashyap -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PULL 25/26] block: Remove deprecated -drive option serial
On Mon, Jun 25, 2018 at 10:23:03AM +0200, Thomas Huth wrote: > On 25.06.2018 09:16, Peter Krempa wrote: > > On Fri, Jun 22, 2018 at 14:55:02 +0200, Kevin Wolf wrote: > >> Am 22.06.2018 um 13:38 hat Christian Borntraeger geschrieben: > >>> > >>> On 06/15/2018 04:21 PM, Kevin Wolf wrote: > The -drive option serial was deprecated in QEMU 2.10. It's time to > remove it. > > Tests need to be updated to set the serial number with -global instead > of using the -drive option. > [...] > >>> So it seems that this breaks s390x. > >> > >> Thanks for bringing this up. libvirt should fix this before QEMU 3.0 is > >> released. > > > > So, I already extracted the code that formats the attributes which > > should be actually done for the frontend few months ago, but did not > > swithc to it since I did not want to check when everything was added. > > > > The fix will be rather simple if we are certain that the disk serial, > > geometry and error policies were supported in qemu 1.5. In that case we > > can switch unconditionally to the new format. > > The CHS device properties are available since QEMU v1.2: > - IDE: https://git.qemu.org/?p=qemu.git;a=commitdiff;h=ba801960 > - SCSI: https://git.qemu.org/?p=qemu.git;a=commitdiff;h=d252df48 > - virtio-blk: https://git.qemu.org/?p=qemu.git;a=commitdiff;h=e63e7fde2 > > The "serial" property and the PCI "addr" property are also available > since 2012 at least, I didn't fully track their origins, but QEMU v1.2 > already supported them. > > So yes, all these properties should be available in QEMU 1.5 already. The werror/rerror attributes were not added until 2.7.0, so we still need a capabilities check for them Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] formatdomain.html.in: Amend the 'random' RNG backend section
On Fri, Jun 22, 2018 at 12:09:39PM +0200, Kashyap Chamarthy wrote: > Since libvirt 1.3.4, any RNG source is accepted for the 'random' > backend. However, '/dev/urandom' is the _recommended_ source of > entropy. > s/\n// > Mention it so in the docs. "Therefore we should mention that in the docs." With that Reviewed-by: Erik Skultety -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [GSoC] Code design for scalar and external types
On Fri, Jun 22, 2018 at 04:43:35AM +0530, Sukrit Bhatnagar wrote:
> On Mon, 18 Jun 2018 at 17:20, Andrea Bolognani wrote:
> >
> > On Mon, 2018-06-18 at 11:52 +0100, Daniel P. Berrangé wrote:
> > > On Mon, Jun 18, 2018 at 12:24:39PM +0200, Pavel Hrdina wrote:
> > > > # define VIR_DEFINE_AUTOCLEAR_FUNC(type, func) \
> > > > static inline void VIR_AUTOCLEAR_FUNC_NAME(type)(type *_ptr) \
> > > > { \
> > > > (func)(_ptr); \
> > > > }
> > >
> > > For anything where we define the impl of (func) these two macros
> > > should never be used IMHO. We should just change the signature of
> > > the real functions so that accept a double pointer straight away,
> > > and thus not require the wrapper function. Yes, it this will
> > > require updating existing code, but such a design change is
> > > beneficial even without the cleanup macros, as it prevents the
> > > possbility of double frees. I'd suggest we just do this kind of
> > > change straightaway
> >
> > Agreed that we want to fix our own free functions.
> >
> > > > # define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type
> > >
> > > I don't see the point in passing "type" in here we could simplify
> > > this:
> > >
> > > #define VIR_AUTOFREE __attribute__((cleanup(virFree))
> > >
> > > VIR_AUTOFREE char *foo = NULL;
> >
> > Passing the type was suggested earlier to make usage consistent
> > across all cases, eg.
> >
> > VIR_AUTOFREE(char *) str = NULL;
> > VIR_AUTOPTR(virDomain) dom = NULL;
> >
> > and IIRC we ended up agreeing that it was a good idea overall.
> >
> > > > # define VIR_AUTOPTR(type) \
> > > > __attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type
> > > > VIR_AUTOPTR_TYPE_NAME(type)
> > > >
> > > > The usage would not require our internal vir*Ptr types and would
> > > > be easy to use with external types as well:
> > > >
> > > > VIR_DEFINE_AUTOPTR_FUNC(virBitmap, virBitmapFree);
> > > >
> > > > VIR_AUTOPTR(virBitmap) map = NULL;
> > > >
> > > > VIR_DEFINE_AUTOPTR_FUNC(LIBSSH2_CHANNEL, libssh2_channel_free);
> > >
> > > Contrary to my general point above about VIR_DEFINE_AUTOPTR_FUNC,
> > > this is a reasonable usage, because we don't control the signature
> > > of the libssh2_channel_free function.
> >
> > This is why I suggested we might want to have two sets of
> > macros, one for types we defined ourselves and one for types
> > we bring in from external libraries.
> >
> > > > VIR_AUTOPTR(LIBSSH2_CHANNEL) channel = NULL;
> > >
> > > With my example above
> > >
> > >#define VIR_AUTOFREE_LIBSSH_CHANNEL VIR_AUTOFREE_FUNC(LIBSSH2_CHANNEL)
> > >
> > >VIR_AUTOFREE_LIBSSH_CHANNEL LIBSSH_CHANNEL *chan = NULL;
> >
> > This makes the declaration needlessly verbose, since you're
> > repeating the type name twice; Pavel's approach would avoid
> > that.
>
> So, have we reached a decision about the design?
>
> Fixing the existing vir*Free functions to take double pointers seems good to
> me,
> as many have mentioned earlier. It will solve multiple problems at once.
> I have almost 7 weeks left of GSoC, and I think it is totally doable.
>
> On the other hand, making cleanup macros use double pointers also seems
> great! But, aren't we borrowing too much design from GLib?
>
> Anyway, both approaches take care of the double free problem.
>
> TL;DR
> I prefer cleanup macros which define functions with double pointers
> over changing
> existing vir*Free functions. It just looks more natural.
It does, but it doesn't solve all the problems, mainly those Dan has in mind.
With attribute cleanup, it's applied on variables that go out of scope, but
that's not what you want if you want to keep the memory for while, imagine the
following:
A->B
where B:
B ( dst, ...)
{
VIR_AUTOPTR(tmp);
VIR_ALLOC(tmp)
...
*dst = tmp;
tmp = NULL;
return;
}
when A is finished with dst, it will need to free it manually --> this could
lead to a double free if A doesn't clear it afterwards, either in A or some
within some other consumer of dst.
So in the long term, we'll probably want to change the signatures, but as I
said, I see the proposed macros as an improvement worth being a stepping stone
towards changing the signatures of free functions. The ultimate goal is still
to gradually rewrite libvirt into some other memory-safe language.
Erik
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PULL 25/26] block: Remove deprecated -drive option serial
On 25.06.2018 09:16, Peter Krempa wrote: > On Fri, Jun 22, 2018 at 14:55:02 +0200, Kevin Wolf wrote: >> Am 22.06.2018 um 13:38 hat Christian Borntraeger geschrieben: >>> >>> On 06/15/2018 04:21 PM, Kevin Wolf wrote: The -drive option serial was deprecated in QEMU 2.10. It's time to remove it. Tests need to be updated to set the serial number with -global instead of using the -drive option. [...] >>> So it seems that this breaks s390x. >> >> Thanks for bringing this up. libvirt should fix this before QEMU 3.0 is >> released. > > So, I already extracted the code that formats the attributes which > should be actually done for the frontend few months ago, but did not > swithc to it since I did not want to check when everything was added. > > The fix will be rather simple if we are certain that the disk serial, > geometry and error policies were supported in qemu 1.5. In that case we > can switch unconditionally to the new format. The CHS device properties are available since QEMU v1.2: - IDE: https://git.qemu.org/?p=qemu.git;a=commitdiff;h=ba801960 - SCSI: https://git.qemu.org/?p=qemu.git;a=commitdiff;h=d252df48 - virtio-blk: https://git.qemu.org/?p=qemu.git;a=commitdiff;h=e63e7fde2 The "serial" property and the PCI "addr" property are also available since 2012 at least, I didn't fully track their origins, but QEMU v1.2 already supported them. So yes, all these properties should be available in QEMU 1.5 already. Thomas signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [GSoC] Code design for scalar and external types
On Mon, Jun 18, 2018 at 11:52:04AM +0100, Daniel P. Berrangé wrote:
> On Mon, Jun 18, 2018 at 12:24:39PM +0200, Pavel Hrdina wrote:
> > Hi,
> >
> > It took me longer to sit down and write this mail but here it goes.
> >
> > There was a lot of ideas about the macro design and it's easy to
> > get lost in all the designs.
> >
> > So we agreed on this form:
> >
> >
> > # define VIR_AUTOPTR_FUNC_NAME(type) virAutoPtr##type
> > # define VIR_AUTOCLEAR_FUNC_NAME(type) virAutoClear##type
> >
> > # define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
> > static inline void VIR_AUTOPTR_FUNC_NAME(type)(type *_ptr) \
> > { \
> > (func)(*_ptr); \
> > }
> >
> > # define VIR_DEFINE_AUTOCLEAR_FUNC(type, func) \
> > static inline void VIR_AUTOCLEAR_FUNC_NAME(type)(type *_ptr) \
> > { \
> > (func)(_ptr); \
> > }
>
> For anything where we define the impl of (func) these two macros
> should never be used IMHO. We should just change the signature of
> the real functions so that accept a double pointer straight away,
> and thus not require the wrapper function. Yes, it this will
> require updating existing code, but such a design change is
> beneficial even without the cleanup macros, as it prevents the
> possbility of double frees. I'd suggest we just do this kind of
> change straightaway
>From security POV, I agree that the free functions should accept a double
pointer. However, in terms of GSoC and the time schedule, I think that having
the macros above is a nice stepping stone towards to long term goal to convert
our free functions to accept double pointers, besides, as you pointed out
below, the macros (at least the AUTOPTR_FUNC) makes sense with external types
and I like that we'd have a almost universal approach here. Even with the
macros, we don't lose anything (because they're straightforward and that's
important), quite the opposite, it's progress compared to the current status
quo.
>
> > # define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type
>
> I don't see the point in passing "type" in here we could simplify
> this:
>
> #define VIR_AUTOFREE __attribute__((cleanup(virFree))
>
> VIR_AUTOFREE char *foo = NULL;
>
> and at the same time fix the char ** problems
>
> #define VIR_AUTOFREE_FUNC(func) __attribute__((cleanup(func))
>
> VIR_AUTOFREE_FUNC(virStringListFree) char *foo = NULL;
>
> or if we want to simplify it
>
> #define VIR_AUTOFREE_STRINGLIST VIR_AUTOFREE_FUNC(virStringListFree)
>
> VIR_AUTOFREE_STRINGLIST char **strs = NULL;
Andrea already pointed it out in his reply, but I don't like this
"simplification" either, the syntax is kinda obfuscating.
>
>
> This also works for external libraries
>
>
>
> > # define VIR_AUTOPTR(type) \
> > __attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type type
> >
> > # define VIR_AUTOCLEAR(type) \
> > __attribute__((cleanup(VIR_AUTOCLEAR_FUNC_NAME(type type
> >
> >
> > but it turned out the it's not sufficient for several reasons:
> >
> > - there is no simple way ho to free list of strings (char **)
> > - this will not work for external types with their own free function
> >
> >
> > In order to solve these two issues there were some ideas.
> >
> >
> > 1. How to handle list of strings
> >
> > We have virStringListFree() function in order to free list of strings,
> > the question is how to use it with these macros:
> >
> > - Create a new typedef for list of strings and use VIR_AUTOPTR:
> >
> > typedef char **virStringList;
> >
> > VIR_DEFINE_AUTOPTR_FUNC(virStringList, virStringListFree);
> >
> > VIR_AUTOPTR(virStringList) list = NULL;
>
> I don't think we should be creating artifical new typedefs just to
> deal with the flawed design of our own autofree macros.
>
> > - Overload VIR_AUTOFREE macro by making it variadic
> >
> > # define _VIR_AUTOFREE_0(type) __attribute__((cleanup(virFree))) type
> > # define _VIR_AUTOFREE_1(type, func) __attribute__((cleanup(func))) type
> >
> > # define _VIR_AUTOFREE_OVERLOADER(_1, _2, NAME, ...) NAME
> > # define VIR_AUTOFREE(...) _VIR_AUTOFREE_OVERLOADER(__VA_ARGS__,
> > _VIR_AUTOFREE_1, _VIR_AUTOFREE_0)(__VA_ARGS__)
>
> This is uneccessarily black magic - just have VIR_AUTOFREE and
> VIR_AUTOFREE_FUNC defined separately.
Yeah, looking back at when I came up with the idea, it is indeed unnecessarily
hacky. Having another macro for this purpose is an option, although Pavel had
some concerns that if we do that, people might misuse it every time they don't
know what the proper usage or approach is in that the type name and the
corresponding free function name should match. To me, that sounds like a
reviewer's job to spot. On the other hand, I think that the virStringList type
would truly be an exception here as it's unlikely that we'd be creating an
integer pointer-based list with everything else already being a compound type.
So, I'm still ambivalent here and I'd be fine with both having either an extra
macro or an extra typedef.
>
> > void
Re: [libvirt] [Qemu-devel] [PULL 25/26] block: Remove deprecated -drive option serial
On 22.06.2018 16:25, Kevin Wolf wrote: > Am 22.06.2018 um 15:36 hat Christian Borntraeger geschrieben: >> >> >> On 06/22/2018 02:55 PM, Kevin Wolf wrote: >>> Am 22.06.2018 um 13:38 hat Christian Borntraeger geschrieben: On 06/15/2018 04:21 PM, Kevin Wolf wrote: > The -drive option serial was deprecated in QEMU 2.10. It's time to > remove it. > > Tests need to be updated to set the serial number with -global instead > of using the -drive option. libvirt 4.5 still creates those (at least on s390x) >>> iothread='1'/> skel -> [...] -drive file=/var/lib/libvirt/qemu/image.zhyp137,format=qcow2,if=none,id=drive-virtio-disk0,serial=skel,cache=none,aio=native -device virtio-blk-ccw,iothread=iothread1,scsi=off,devno=fe.0.,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1,write-cache=on [...] 2018-06-22T11:25:20.946024Z qemu-system-s390x: -drive file=/var/lib/libvirt/qemu/image.zhyp137,format=qcow2,if=none,id=drive-virtio-disk0,serial=skel,cache=none,aio=native: Block format 'qcow2' does not support the option 'serial' 2018-06-22 11:25:21.098+: shutting down, reason=failed So it seems that this breaks s390x. >> >> To me it seems that this is also broken on x86. >>> >>> Thanks for bringing this up. libvirt should fix this before QEMU 3.0 is >>> released. >> >> I think this is definitely too short notice. We should not break existing >> setups just by insisting that users have to update libvirt when they update >> QEMU. Yes, this might be our policy, but doing so "just because we can" >> is certainly a very bad attitude. I see no fundamental technical reason why >> we should not revert this change. > > This was in fact one release longer than our deprecation policy says. Actually, if we assume that the chapter in qemu-doc.texi is the "official" way to deprecate things, these options are only officially deprecated since QEMU v2.12, since we missed to add them to the deprecation chapter earlier: https://git.qemu.org/?p=qemu.git;a=commitdiff;h=c08d46a9 (We've mentioned them in https://wiki.qemu.org/ChangeLog/2.10#Deprecated_options already, though) Thomas -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PULL 25/26] block: Remove deprecated -drive option serial
On Fri, Jun 22, 2018 at 14:55:02 +0200, Kevin Wolf wrote: > Am 22.06.2018 um 13:38 hat Christian Borntraeger geschrieben: > > > > On 06/15/2018 04:21 PM, Kevin Wolf wrote: > > > The -drive option serial was deprecated in QEMU 2.10. It's time to > > > remove it. > > > > > > Tests need to be updated to set the serial number with -global instead > > > of using the -drive option. > > > > libvirt 4.5 still creates those (at least on s390x) > > > > > >> iothread='1'/> > > > > > > skel > > > > > > > > > > > > -> > > [...] > > -drive > > file=/var/lib/libvirt/qemu/image.zhyp137,format=qcow2,if=none,id=drive-virtio-disk0,serial=skel,cache=none,aio=native > > -device > > virtio-blk-ccw,iothread=iothread1,scsi=off,devno=fe.0.,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1,write-cache=on > > > > [...] > > > > 2018-06-22T11:25:20.946024Z qemu-system-s390x: -drive > > file=/var/lib/libvirt/qemu/image.zhyp137,format=qcow2,if=none,id=drive-virtio-disk0,serial=skel,cache=none,aio=native: > > Block format 'qcow2' does not support the option 'serial' > > 2018-06-22 11:25:21.098+: shutting down, reason=failed > > > > So it seems that this breaks s390x. > > Thanks for bringing this up. libvirt should fix this before QEMU 3.0 is > released. So, I already extracted the code that formats the attributes which should be actually done for the frontend few months ago, but did not swithc to it since I did not want to check when everything was added. The fix will be rather simple if we are certain that the disk serial, geometry and error policies were supported in qemu 1.5. In that case we can switch unconditionally to the new format. signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2] qemuDomainObjBeginJobInternal: Report agent job in error message
If a thread is unable to acquire a job (e.g. because of timeout)
an error is reported and the error message contains reference to
the other thread holding the job. Well, the error message should
report agent job too as it is yet another source of possible
failure.
Signed-off-by: Michal Privoznik
---
diff to v1:
- Expand messages to cover all @blocker and @agentBlocker possibilities
(as Jira requested in review)
src/qemu/qemu_domain.c | 46 ++
1 file changed, 38 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 95c3e3a8aa..3410774781 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -6420,6 +6420,7 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
bool async = job == QEMU_JOB_ASYNC;
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
const char *blocker = NULL;
+const char *agentBlocker = NULL;
int ret = -1;
unsigned long long duration = 0;
unsigned long long agentDuration = 0;
@@ -6549,16 +6550,32 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
priv->job.apiFlags,
duration / 1000, agentDuration / 1000, asyncDuration / 1000);
-if (nested || qemuDomainNestedJobAllowed(priv, job))
-blocker = priv->job.ownerAPI;
-else
-blocker = priv->job.asyncOwnerAPI;
+if (job) {
+if (nested || qemuDomainNestedJobAllowed(priv, job))
+blocker = priv->job.ownerAPI;
+else
+blocker = priv->job.asyncOwnerAPI;
+}
+
+if (agentJob)
+agentBlocker = priv->job.agentOwnerAPI;
if (errno == ETIMEDOUT) {
-if (blocker) {
+if (blocker && agentBlocker) {
virReportError(VIR_ERR_OPERATION_TIMEOUT,
- _("cannot acquire state change lock (held by %s)"),
+ _("cannot acquire state change "
+ "lock (held by monitor=%s agent=%s)"),
+ blocker, agentBlocker);
+} else if (blocker) {
+virReportError(VIR_ERR_OPERATION_TIMEOUT,
+ _("cannot acquire state change "
+ "lock (held by monitor=%s)"),
blocker);
+} else if (agentBlocker) {
+virReportError(VIR_ERR_OPERATION_TIMEOUT,
+ _("cannot acquire state change "
+ "lock (held by agent=%s)"),
+ agentBlocker);
} else {
virReportError(VIR_ERR_OPERATION_TIMEOUT, "%s",
_("cannot acquire state change lock"));
@@ -6566,11 +6583,24 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
ret = -2;
} else if (cfg->maxQueuedJobs &&
priv->jobs_queued > cfg->maxQueuedJobs) {
-if (blocker) {
+if (blocker && agentBlocker) {
virReportError(VIR_ERR_OPERATION_FAILED,
- _("cannot acquire state change lock (held by %s) "
+ _("cannot acquire state change "
+ "lock (held by monitor=%s agent=%s) "
+ "due to max_queued limit"),
+ blocker, agentBlocker);
+} else if (blocker) {
+virReportError(VIR_ERR_OPERATION_FAILED,
+ _("cannot acquire state change "
+ "lock (held by monitor=%s) "
"due to max_queued limit"),
blocker);
+} else if (agentBlocker) {
+virReportError(VIR_ERR_OPERATION_FAILED,
+ _("cannot acquire state change "
+ "lock (held by agent=%s) "
+ "due to max_queued limit"),
+ agentBlocker);
} else {
virReportError(VIR_ERR_OPERATION_FAILED, "%s",
_("cannot acquire state change lock "
--
2.16.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
