[libvirt] [PATCH v3] qemu: fix deadlock if create qemuProcessReconnect thread failed
qemuProcessReconnectHelper has hold lock on doms, if create
qemuProcessReconnect thread failed, calls qemuDomainRemoveInactiveJob
will lead to deadlock.
Add a qemuDomainRemoveInactiveJobLocked, modify qemuProcessReconnectHelper
to call it.
Signed-off-by: Wang Yechao
---
v2 patch:
https://www.redhat.com/archives/libvir-list/2018-September/msg00635.html
Changes in v3:
- drop v2 patch, cause it is not good.
- split qemuDomainRemoveInactive into qemuDomainRemoveInactiveCommon
and virDomainObjListRemove.
- create a qemuDomainRemoveInactiveLocked, consists of
qemuDomainRemoveInactiveCommon and virDomainObjListRemoveLocked.
- create a qemuDomainRemoveInactiveJobLocked, which copies
qemuDomainRemoveInactiveJob except calling qemuDomainRemoveInactiveLocked
- Modify qemuProcessReconnectHelper to call qemuDomainRemoveInactiveJobLocked
---
src/qemu/qemu_domain.c | 75 -
src/qemu/qemu_domain.h | 9 ++
src/qemu/qemu_process.c | 2 +-
3 files changed, 71 insertions(+), 15 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 2fd8a2a..ef0d91d 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -8343,24 +8343,13 @@ qemuDomainSnapshotDiscardAllMetadata(virQEMUDriverPtr
driver,
return rem.err;
}
-
-/**
- * qemuDomainRemoveInactive:
- *
- * The caller must hold a lock to the vm.
- */
void
-qemuDomainRemoveInactive(virQEMUDriverPtr driver,
- virDomainObjPtr vm)
+qemuDomainRemoveInactiveCommon(virQEMUDriverPtr driver,
+ virDomainObjPtr vm)
{
char *snapDir;
virQEMUDriverConfigPtr cfg;
-if (vm->persistent) {
-/* Short-circuit, we don't want to remove a persistent domain */
-return;
-}
-
cfg = virQEMUDriverGetConfig(driver);
/* Remove any snapshot metadata prior to removing the domain */
@@ -8379,9 +8368,47 @@ qemuDomainRemoveInactive(virQEMUDriverPtr driver,
}
qemuExtDevicesCleanupHost(driver, vm->def);
+virObjectUnref(cfg);
+}
+
+/**
+ * qemuDomainRemoveInactive:
+ *
+ * The caller must hold a lock to the vm.
+ */
+void
+qemuDomainRemoveInactive(virQEMUDriverPtr driver,
+ virDomainObjPtr vm)
+{
+if (vm->persistent) {
+/* Short-circuit, we don't want to remove a persistent domain */
+return;
+}
+
+qemuDomainRemoveInactiveCommon(driver, vm);
virDomainObjListRemove(driver->domains, vm);
-virObjectUnref(cfg);
+}
+
+/**
+ * qemuDomainRemoveInactiveLocked:
+ *
+ * The caller must hold a lock to the vm ,
+ * and hold a lock to the doms.
+ */
+
+void
+qemuDomainRemoveInactiveLocked(virQEMUDriverPtr driver,
+ virDomainObjPtr vm)
+{
+if (vm->persistent) {
+/* Short-circuit, we don't want to remove a persistent domain */
+return;
+}
+
+qemuDomainRemoveInactiveCommon(driver, vm);
+virDomainObjListRemoveLocked(driver->domains, vm);
+
}
@@ -8407,6 +8434,26 @@ qemuDomainRemoveInactiveJob(virQEMUDriverPtr driver,
qemuDomainObjEndJob(driver, vm);
}
+/**
+ * qemuDomainRemoveInactiveJobLocked:
+ *
+ * Just like qemuDomainRemoveInactiveJob but it hold lock
+ * on 'doms'.
+ */
+
+void
+qemuDomainRemoveInactiveJobLocked(virQEMUDriverPtr driver,
+ virDomainObjPtr vm)
+{
+bool haveJob;
+
+haveJob = qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) >= 0;
+
+qemuDomainRemoveInactiveLocked(driver, vm);
+
+if (haveJob)
+qemuDomainObjEndJob(driver, vm);
+}
void
qemuDomainSetFakeReboot(virQEMUDriverPtr driver,
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 914c9a6..1d80621 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -698,12 +698,21 @@ int qemuDomainSnapshotDiscardAll(void *payload,
int qemuDomainSnapshotDiscardAllMetadata(virQEMUDriverPtr driver,
virDomainObjPtr vm);
+void qemuDomainRemoveInactiveCommon(virQEMUDriverPtr driver,
+virDomainObjPtr vm);
+
void qemuDomainRemoveInactive(virQEMUDriverPtr driver,
virDomainObjPtr vm);
+void qemuDomainRemoveInactiveLocked(virQEMUDriverPtr driver,
+virDomainObjPtr vm);
+
void qemuDomainRemoveInactiveJob(virQEMUDriverPtr driver,
virDomainObjPtr vm);
+void qemuDomainRemoveInactiveJobLocked(virQEMUDriverPtr driver,
+ virDomainObjPtr vm);
+
void qemuDomainSetFakeReboot(virQEMUDriverPtr driver,
virDomainObjPtr vm,
bool value);
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 72a59de..ed24447 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -8025,7 +8025,7 @@ qemuProcessReconnectHelper(virDomainObjPtr obj,
*/
Re: [libvirt] [PATCH 0/5] Cleanup some storage driver paths
ping? Tks, John On 09/12/2018 12:08 PM, John Ferlan wrote: > See the patches for details - everything leads to the last one. > > John Ferlan (5): > storage: Clean up stateFile if refreshPool fails > storage: Clean up storagePoolUpdateStateCallback processing > storage: Create error label path for storagePoolCreateXML > storage: Introduce storagePoolRefreshFailCleanup > storage: Save error during refresh failure processing > > src/storage/storage_driver.c | 73 > 1 file changed, 40 insertions(+), 33 deletions(-) > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt PATCH v6 15/15] xen_common: Change xenParseCharDev to using virConfGetValueStringList
On 09/18/2018 02:48 PM, Fabiano Fidêncio wrote:
> Signed-off-by: Fabiano Fidêncio
> ---
> src/xenconfig/xen_common.c | 23 +++
> 1 file changed, 11 insertions(+), 12 deletions(-)
>
> diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
> index a6e77a9250..786c276c99 100644
> --- a/src/xenconfig/xen_common.c
> +++ b/src/xenconfig/xen_common.c
> @@ -759,9 +759,11 @@ xenParseVfb(virConfPtr conf, virDomainDefPtr def)
> static int
> xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char
> *nativeFormat)
> {
> +char **serials = NULL;
> const char *str;
> virConfValuePtr value = NULL;
> virDomainChrDefPtr chr = NULL;
> +int ret = -1;
>
> if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) {
> if (xenConfigGetString(conf, "parallel", , NULL) < 0)
Heh heh - this is what I noted earlier - @str isn't going to be FREE'd.
> @@ -782,7 +784,8 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def,
> const char *nativeFormat)
>
> /* Try to get the list of values to support multiple serial ports */
> value = virConfGetValue(conf, "serial");
> -if (value && value->type == VIR_CONF_LIST) {
> +if (virConfGetValueStringList(conf, "serial", false, ) == 1)
> {
Well this is similar to the previous two except that you kept @value
here and that line probably should have been deleted as would the value
= value->next below and the variable itself as it's unused... At least
it shouldn't run off the end of the list...
> +char **entries;
> int portnum = -1;
>
> if (STREQ(nativeFormat, XEN_CONFIG_FORMAT_XM)) {
> @@ -791,18 +794,12 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def,
> const char *nativeFormat)
> goto cleanup;
> }
>
> -value = value->list;
> -while (value) {
> -char *port = NULL;
> +for (entries = serials; *entries; entries++) {
> +char *port = *entries;
>
> -if ((value->type != VIR_CONF_STRING) || (value->str == NULL))
> -goto cleanup;
> -port = value->str;
> portnum++;
> -if (STREQ(port, "none")) {
> -value = value->next;
> +if (STREQ(port, "none"))
> continue;
> -}
>
> if (!(chr = xenParseSxprChar(port, NULL)))
> goto cleanup;
> @@ -827,6 +824,7 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def,
> const char *nativeFormat)
> chr->target.port = 0;
> def->serials[0] = chr;
> def->nserials++;
> +chr = NULL;
Hmmm... unrelated but equally bad bug. This would require it's own
separate patch.
John
> }
> }
> } else {
> @@ -840,11 +838,12 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def,
> const char *nativeFormat)
> def->consoles[0]->targetType =
> VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_XEN;
> }
>
> -return 0;
> +ret = 0;
>
> cleanup:
> virDomainChrDefFree(chr);
> -return -1;
> +virStringListFree(serials);
> +return ret;
> }
>
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt PATCH v6 14/15] xen_common: Change xenParseVfbs to using virConfGetValueStringList
On 09/18/2018 02:48 PM, Fabiano Fidêncio wrote:
> Signed-off-by: Fabiano Fidêncio
> ---
> src/xenconfig/xen_common.c | 16
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
> index 9ad081e56b..a6e77a9250 100644
> --- a/src/xenconfig/xen_common.c
> +++ b/src/xenconfig/xen_common.c
> @@ -605,8 +605,10 @@ xenParseCPUFeatures(virConfPtr conf,
> static int
> xenParseVfb(virConfPtr conf, virDomainDefPtr def)
> {
> +int ret = -1;
> int val;
> char *listenAddr = NULL;
> +char **vfbs = NULL;
> int hvm = def->os.type == VIR_DOMAIN_OSTYPE_HVM;
> virConfValuePtr list;
> virDomainGraphicsDefPtr graphics = NULL;
> @@ -664,17 +666,14 @@ xenParseVfb(virConfPtr conf, virDomainDefPtr def)
> }
>
> if (!hvm && def->graphics == NULL) { /* New PV guests use this format */
> -list = virConfGetValue(conf, "vfb");
> -if (list && list->type == VIR_CONF_LIST &&
> -list->list && list->list->type == VIR_CONF_STRING &&
> -list->list->str) {
> +if (virConfGetValueStringList(conf, "vfb", false, ) == 1) {
There needs to be an else that will do some of the similar checking
described in the previous patch...
At the very least if return < 0, then virResetLastError... In fact now
that I see the same pattern, the code described in the last patch should
be a some common local/static function used by the 3 consumers to handle
the case where rc is not 1.
John
> char vfb[MAX_VFB];
> char *key = vfb;
>
> -if (virStrcpyStatic(vfb, list->list->str) < 0) {
> +if (virStrcpyStatic(vfb, *vfbs) < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("VFB %s too big for destination"),
> - list->list->str);
> + *vfbs);
> goto cleanup;
> }
>
> @@ -747,12 +746,13 @@ xenParseVfb(virConfPtr conf, virDomainDefPtr def)
> }
> }
>
> -return 0;
> +ret = 0;
>
> cleanup:
> virDomainGraphicsDefFree(graphics);
> VIR_FREE(listenAddr);
> -return -1;
> +virStringListFree(vfbs);
> +return ret;
> }
>
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt PATCH v6 13/15] xen_common: Change xenParsePCIList to using virConfGetValueStringList
On 09/18/2018 02:48 PM, Fabiano Fidêncio wrote:
> The `if (!list || list->type != VIR_CONF_LIST)` couldn't be written in a
> 100% similar way as `if (virConfGetValueStringList(conf, "pci", false,
> ) <= 0)` leads us to just ignore any error and return 0 in case of
> failure. However, for what's needed in this function, this is the
> closest to the original that we can get and it shouldn't change the
> function behaviour.
>
> Signed-off-by: Fabiano Fidêncio
> ---
> src/xenconfig/xen_common.c | 23 +--
> 1 file changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
> index 09f93ba449..9ad081e56b 100644
> --- a/src/xenconfig/xen_common.c
> +++ b/src/xenconfig/xen_common.c
> @@ -462,27 +462,30 @@ xenParsePCI(char *entry)
> static int
> xenParsePCIList(virConfPtr conf, virDomainDefPtr def)
> {
> -virConfValuePtr list = virConfGetValue(conf, "pci");
> +char **pcis = NULL, **entries;
> +int ret = -1;
>
> -if (!list || list->type != VIR_CONF_LIST)
> +if (virConfGetValueStringList(conf, "pci", false, ) <= 0)
> return 0;
Since this call passes @false, that means virConfGetValueStringList
processing will "fallthrough" the switch for VIR_CONF_STRING and thus
generate an error and return -1. So to that degree OK, I agree this is
no different than the previous code. I'll point out it's not a great
design, but yes, no worse than before.
Still if < 0, a virReportError was generated, thus we need to
virResetLastError before returning 0 indicating we're going to ignore
whether the "pci" value was found and whether it was the right type
(e.g. since @false we only want VIR_CONF_LIST values.
This will though cause us to miss memory allocation errors, but I have a
feeling we'd hit another one real soon.
So, this will be ugly and makes assumptions that may not be true in the
called function forever, but would look something like this:
int rc;
if ((rc = virConfGetValueStringList(...)) <= 0) {
if (rc < 0) {
/* IOW: If called function didn't fail because the
* cval->type switch fell through - since we're passing
* @compatString == false - assumes failures for memory
* allocation and VIR_CONF_LIST traversal failure
* should cause -1 to be returned to the caller with
* the error message set. */
if (virGetLastErrorCode() != VIR_ERR_INTERNAL_ERROR)
return -1;
/* If we did fall through the switch, then ignore and
* clear the last error./
virResetLastError();
}
return 0;
}
See, really ugly and not 100% supportable, but it should cover the
existing cases.
John
>
> -for (list = list->list; list; list = list->next) {
> +for (entries = pcis; *entries; entries++) {
> +char *entry = *entries;
> virDomainHostdevDefPtr hostdev;
>
> -if ((list->type != VIR_CONF_STRING) || (list->str == NULL))
> -continue;
> -
> -if (!(hostdev = xenParsePCI(list->str)))
> -return -1;
> +if (!(hostdev = xenParsePCI(entry)))
> +goto cleanup;
>
> if (VIR_APPEND_ELEMENT(def->hostdevs, def->nhostdevs, hostdev) < 0) {
> virDomainHostdevDefFree(hostdev);
> -return -1;
> +goto cleanup;
> }
> }
>
> -return 0;
> +ret = 0;
> +
> + cleanup:
> +virStringListFree(pcis);
> +return ret;
> }
>
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt PATCH v6 03/15] xen_common: Change xenConfigGetString to using virConfGetValueString
On 09/18/2018 02:48 PM, Fabiano Fidêncio wrote:
> This change actually changes the behaviour of xenConfigGetString() as
> now it returns a newly-allocated string.
>
> Unfortunately, there's not much that can be done in order to avoid that
> and all the needed changes in callers in order to not leak the returned
> value are coming in the following patches.
Having a patch with a known memory leak to be fixed by "n" subsequent
patches is in general not accepted. If you didn't know about them (as is
often the case), then we'd be good.
One thing that you "may" consider (and I wasn't involved in) is using
the VIR_AUTOFREE stuff that automagically cleans up for variables. Talk
with Erik or Pavel, they were highly involved.
Of course that assumes you fix a couple other issues - read on...
>
> Signed-off-by: Fabiano Fidêncio
> ---
> src/xenconfig/xen_common.c | 22 +++---
> 1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
> index 08fbfff44f..c044cb9672 100644
> --- a/src/xenconfig/xen_common.c
> +++ b/src/xenconfig/xen_common.c
> @@ -228,23 +228,23 @@ xenConfigGetString(virConfPtr conf,
> const char **value,
Being able to assign an allocated buffer to *value leaves me wondering
why the compiler isn't throwing up because the "const"-ness...
> const char *def)
> {
> -virConfValuePtr val;
> +char *string = NULL;
> +int rc;
>
> *value = NULL;
> -if (!(val = virConfGetValue(conf, name))) {
> -*value = def;
> +if ((rc = virConfGetValueString(conf, name, )) < 0)
> +return -1;
> +
> +if (rc == 0) {
> +*value = VIR_STRDUP(def);
I don't think you're compiling this code (like me) since VIR_STRDUP is
generally something like:
if (VIR_STRDUP(*value, def) < 0)
return -1;
and I know for sure the compiler would complain as it would if *value is
a "const char **"... As an example this should throw up with for example
if I added a properly formatted VIR_STRDUP to vsh.c:
In file included from vsh.c:60:
vsh.c: In function 'vshCommandOptStringQuiet':
../src/util/virstring.h:167:41: error: passing argument 1 of 'virStrdup'
from incompatible pointer type [-Werror=incompatible-pointer-types]
# define VIR_STRDUP(dst, src) virStrdup(&(dst), src, true, VIR_FROM_THIS, \
^~
vsh.c:1026:9: note: in expansion of macro 'VIR_STRDUP'
if (VIR_STRDUP(*value, "shit this works") < 0)
^~
../src/util/virstring.h:137:22: note: expected 'char **' but argument is
of type 'const char **'
int virStrdup(char **dest, const char *src, bool report, int domcode,
~~~^~~~
Once you get your build working, then I think if you change this *and*
all the callers to use "VIR_AUTOFREE(char *) value = NULL;" (where
@value will be each value that needs to be free'd) and change the
API/prototype to use "char **" instead of "const char **", then I think
all that in one patch will do what you want.
I won't look at patches 4 -> 12 since they're impacted by the above, but
I do note that you missed xenParseCharDev.
John
> return 0;
> }
> > -if (val->type != VIR_CONF_STRING) {
> -virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("config value %s was malformed"), name);
> -return -1;
> -}
> -if (!val->str)
> -*value = def;
> +if (!string)
> +*value = VIR_STRDUP(def);
> else
> -*value = val->str;
> +*value = string;
> +
> return 0;
> }
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt PATCH v6 02/15] xen_common: Change xenConfigGetUUID to using virConfGetValueString
On 09/18/2018 02:48 PM, Fabiano Fidêncio wrote:
> Signed-off-by: Fabiano Fidêncio
> ---
> src/xenconfig/xen_common.c | 16 ++--
> 1 file changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
> index a35e1aff58..08fbfff44f 100644
> --- a/src/xenconfig/xen_common.c
> +++ b/src/xenconfig/xen_common.c
> @@ -183,7 +183,7 @@ xenConfigCopyStringOpt(virConfPtr conf, const char *name,
> char **value)
> static int
> xenConfigGetUUID(virConfPtr conf, const char *name, unsigned char *uuid)
> {
> -virConfValuePtr val;
> +char *string = NULL;
>
> if (!uuid || !name || !conf) {
> virReportError(VIR_ERR_INVALID_ARG, "%s",
> @@ -191,7 +191,7 @@ xenConfigGetUUID(virConfPtr conf, const char *name,
> unsigned char *uuid)
> return -1;
> }
>
> -if (!(val = virConfGetValue(conf, name))) {
This is the "isn't found" or "was missing" test...
> +if (virConfGetValueString(conf, name, ) <= 0) {
But this takes it one step too far. If return < 0, then we had "if
(cval->type != VIR_CONF_STRING)" or "VIR_STRDUP" failure.
However, if you change to:
int rc;
...
if ((rc = virConfGetValueString(conf, name, )) < 0)
return -1;
if (rc == 0) {
then I believe we're good.
With the change,
Reviewed-by: John Ferlan
John
[I can make the change locally, but I've forgotten the magic incantation
or rpm that allows the xen* files to be built on my host, so I'm flying
blind w/r/t proper syntax ;-)]
> if (virUUIDGenerate(uuid) < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> "%s", _("Failed to generate UUID"));
[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt PATCH v6 01/15] xen_common: Change xenConfigCopyStringInternal to using virConfGetValueString
On 09/18/2018 02:48 PM, Fabiano Fidêncio wrote: > Signed-off-by: Fabiano Fidêncio > --- > src/xenconfig/xen_common.c | 18 -- > 1 file changed, 4 insertions(+), 14 deletions(-) > Reviewed-by: John Ferlan John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [QEMU PATCH] net: Deprecate the old way of using a legacy net via "name" instead of "id"
In early times, network backends were specified by a "vlan" and "name"
tuple. With the introduction of netdevs, the "name" was replaced by an
"id" (which is supposed to be unique), but the "name" parameter stayed
as an alias which could be used instead of "id". Unfortunately, we miss
the duplication check for "name":
$ qemu-system-x86_64 -net user,name=n1 -net user,name=n1
... starts without an error, while "id" correctly complains:
$ qemu-system-x86_64 -net user,id=n1 -net user,id=n1
qemu-system-x86_64: -net user,id=n1: Duplicate ID 'n1' for net
Instead of trying to fix the code for the legacy "name" parameter, let's
rather get rid of this old interface and deprecate the "name" parameter
now - this will also be less confusing for the users in the long run.
While we're at it, also deprecate the old syntax for the hostfwd_add
and hostfwd_remove commands that still work with this legacy "hub"
plus "name" tuple. It is enough to specify the netdev id there instead.
Also add a missing dependency to the Makefile to make sure that the
docs get correctly regenerated when qemu-deprecated.texi is changed.
Signed-off-by: Thomas Huth
---
Makefile | 2 +-
net/net.c| 4
net/slirp.c | 2 ++
qemu-deprecated.texi | 13 +
4 files changed, 20 insertions(+), 1 deletion(-)
diff --git a/Makefile b/Makefile
index fe623e4..f42e176 100644
--- a/Makefile
+++ b/Makefile
@@ -978,7 +978,7 @@ txt: qemu-doc.txt docs/interop/qemu-qmp-ref.txt
docs/interop/qemu-ga-ref.txt
qemu-doc.html qemu-doc.info qemu-doc.pdf qemu-doc.txt: \
qemu-img.texi qemu-nbd.texi qemu-options.texi qemu-option-trace.texi \
- qemu-monitor.texi qemu-img-cmds.texi qemu-ga.texi \
+ qemu-deprecated.texi qemu-monitor.texi qemu-img-cmds.texi qemu-ga.texi \
qemu-monitor-info.texi docs/qemu-block-drivers.texi \
docs/qemu-cpu-models.texi
diff --git a/net/net.c b/net/net.c
index 2a31339..cdcd5cf 100644
--- a/net/net.c
+++ b/net/net.c
@@ -984,6 +984,10 @@ static int net_client_init1(const void *object, bool
is_netdev, Error **errp)
/* missing optional values have been initialized to "all bits zero" */
name = net->has_id ? net->id : net->name;
+if (net->has_name) {
+warn_report("The 'name' parameter is deprecated, use 'id'
instead");
+}
+
/* Map the old options to the new flat type */
switch (opts->type) {
case NET_LEGACY_OPTIONS_TYPE_NONE:
diff --git a/net/slirp.c b/net/slirp.c
index c18060f..073bc69 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -404,6 +404,8 @@ static SlirpState *slirp_lookup(Monitor *mon, const char
*hub_id,
monitor_printf(mon, "unrecognized (hub-id, stackname) pair\n");
return NULL;
}
+warn_report("Using 'hub-id' is deprecated. Specify the netdev id "
+"directly instead");
} else {
nc = qemu_find_netdev(name);
if (!nc) {
diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 8a2e399..eed7132 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -83,6 +83,11 @@ The 'file' driver for drives is no longer appropriate for
character or host
devices and will only accept regular files (S_IFREG). The correct driver
for these file types is 'host_cdrom' or 'host_device' as appropriate.
+@subsection -net ...,name=@var{name} (since 3.1)
+
+The @option{name} parameter of the @option{-net} option is an old synonym
+for the @option{id} parameter which should now be used instead.
+
@section QEMU Machine Protocol (QMP) commands
@subsection block-dirty-bitmap-add "autoload" parameter (since 2.12.0)
@@ -99,6 +104,14 @@ The ``query-cpus'' command is replaced by the
``query-cpus-fast'' command.
The ``arch'' output member of the ``query-cpus-fast'' command is
replaced by the ``target'' output member.
+@section System emulator human monitor commands
+
+@subsection The hub_id parameter of 'hostfwd_add' / 'hostfwd_remove' (since
3.1)
+
+The @option{hub_id} parameter of the 'hostfwd_add' and 'hostfwd_remove' HMP
+commands is redundant. It is enough to specify the netdev ID of the backend
+that should be changed.
+
@section System emulator devices
@subsection ivshmem (since 2.6.0)
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/5] qemu: Avoid duplicate resume events and state changes
On 09/12/2018 08:55 AM, Jiri Denemark wrote:
> The only place where VIR_DOMAIN_EVENT_RESUMED is generated is the RESUME
I assume you meant "should be generated"
> event handler to make sure we don't generate duplicate events or state
> changes. In the worse case the duplicity can revert or cover changes
> done by other event handlers.
>
> For example, after QEMU sent RESUME, BLOCK_IO_ERROR, and STOP events
> we could happily mark the domain as running and report
> VIR_DOMAIN_EVENT_RESUMED to registered clients.
>
This patch removes the non QEMU event handling resume processing such as
1. User initiated domain resume
2. Resume after revert from snapshot
3. Resume on migration source after failure detected
4. Resume on migration destination once finished
5. Resume after Shutdown/Reboot or Panic when guest is configured to
process a fake reboot.
deferring completely to the resume event QEMU will send as a result of
the CPUs being successfully started.
...
My caveats:
1. I think that list is correct - I may have miss-worded #5 - I forget
exactly how that code works until I'm debugging it ;-)
2. The CPUs being successfully restarted is I think the "cont" command
being successful.
> https://bugzilla.redhat.com/show_bug.cgi?id=1612943
>
> Signed-off-by: Jiri Denemark
> ---
> src/qemu/qemu_driver.c| 13 -
> src/qemu/qemu_migration.c | 36 ++--
> src/qemu/qemu_process.c | 10 --
> 3 files changed, 10 insertions(+), 49 deletions(-)
>
What I type above could be "massaged" into the commit message - so it's
"easier" at a glance to understand what's going on. It's of course my
wording and making sure I've properly understood what happening without
being able to stop by your cube and ask directly ;-).
[...]
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 825a9d399b..4771f26938 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
[...]
>
> @@ -5074,13 +5060,8 @@ qemuMigrationDstFinish(virQEMUDriverPtr driver,
> goto endjob;
> }
>
> -if (inPostCopy) {
> +if (inPostCopy)
> doKill = false;
> -event = virDomainEventLifecycleNewFromObj(vm,
> -VIR_DOMAIN_EVENT_RESUMED,
> -VIR_DOMAIN_EVENT_RESUMED_POSTCOPY);
> -virObjectEventStateQueue(driver->domainEventState, event);
> -}
> }
>
> if (mig->jobInfo) {
> @@ -5111,11 +5092,6 @@ qemuMigrationDstFinish(virQEMUDriverPtr driver,
>
> dom = virGetDomain(dconn, vm->def->name, vm->def->uuid, vm->def->id);
>
> -event = virDomainEventLifecycleNewFromObj(vm,
> - VIR_DOMAIN_EVENT_RESUMED,
> -
> VIR_DOMAIN_EVENT_RESUMED_MIGRATED);
> -virObjectEventStateQueue(driver->domainEventState, event);
> -
> if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_PAUSED) {
> virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, VIR_DOMAIN_PAUSED_USER);
> event = virDomainEventLifecycleNewFromObj(vm,
For this path 2 events were generated if @inPostCopy == True - one right
after the qemuProcessStartCPUs for RESUMED_POSTCOPY and then one for
SUSPENDED_PAUSED once qemuMigrationDstWaitForCompletion occurred.
IIUC, the changes here when @inPostCopy == true would only generate the
POSTCOPY event, but not the MIGRATED event. Or is there something subtle
I'm missing. Will the post copy processing generate two resume events?
If so, we perhaps should document that. If not, then perhaps this second
event that's being deleted needs to be moved inside that inPostCopy
condition just above with the note that this one case is "abnormal" (or
you could just do the *StartCPUs again with the RESUMED_MIGRATED and
close your eyes ;-)).
Everything else is fine - it's just this one case.
John
[...]
Curious, for the future, would it be useful to change the FakeReboot
path to generate a different detail reason? It's hard to distinguish
between someone using virsh resume (or recovery from Save, Dump,
Snapshot failure) from that fake reboot path - at least from just the
event. I guess it seems VIR_DOMAIN_EVENT_RESUMED_UNPAUSED is just too
generic since it's more than just a "normal resume due to admin
unpause", but that I would think would be extra given the "scope" of
this particular problem.
Second curiosity... would it be useful/easy to generate an event on the
failure scenario for StartCPUs? Knowing from whence we were called, we
should be able to generate a suspended event rather than having the
callers decide.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH V3 1/2] libxl: drop support for Xen < 4.6
On 9/19/18 2:09 AM, Andrea Bolognani wrote: On Tue, 2018-09-18 at 10:16 +0200, Andrea Bolognani wrote: On Mon, 2018-09-17 at 10:00 -0600, Jim Fehlig wrote: On 9/17/18 2:59 AM, Andrea Bolognani wrote: Depending on how long it takes to get that fixed in Fedora, we might want to temporarily reintroduce the fallback path... Ok. What is your estimate of "long" in this case? :-) By the end of the day? End of week? Before freeze for next release? I'd say we give the maintainer a few days to respond, and if nothing happens by next Monday we just put the code back in. The maintainer has fixed the issue in Fedora 29+, but they're understandably not going to backport the change to earlier Fedora releases: https://bugzilla.redhat.com/show_bug.cgi?id=1629643#c2 Yep, saw that. So we're going to need the fallback code for ~6 more months. Would you mind posting a patch that reintroduces it along with a comment mentioning that we can drop it as soon as Fedora 28 goes out of support? https://www.redhat.com/archives/libvir-list/2018-September/msg00937.html Regards, Jim -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/2] build: fix CI builds
Fix CI builds broken by commit 5bdcef13. While doing so I noticed more unused variables, which are removed by patch1. Jim Fehlig (2): build: remove unused variables from virt-driver-libxl.m4 libxl: fallback to lib probe if pkgconfig file not found m4/virt-driver-libxl.m4 | 23 --- 1 file changed, 20 insertions(+), 3 deletions(-) -- 2.18.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/2] libxl: fallback to lib probe if pkgconfig file not found
With the assumption that all Xen >= 4.6 contains a pkgconfig file for libxenlight, commit 5bdcef13 dropped the fallback check to probe libxenlight with LIBVIRT_CHECK_LIB. At the time it was not known that the various Xen pkgconfig files are in the -runtime package in Fedora, instead of the traditional -devel package. This bug [1] was fixed in Fedora > 28, but until Fedora 28 reaches EOL we'll need to re-introduce the fallback check. [1] https://bugzilla.redhat.com/show_bug.cgi?id=1629643 Signed-off-by: Jim Fehlig --- m4/virt-driver-libxl.m4 | 22 +- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/m4/virt-driver-libxl.m4 b/m4/virt-driver-libxl.m4 index 422ff27022..479d9116a4 100644 --- a/m4/virt-driver-libxl.m4 +++ b/m4/virt-driver-libxl.m4 @@ -29,11 +29,31 @@ AC_DEFUN([LIBVIRT_DRIVER_CHECK_LIBXL], [ LIBXL_API_VERSION="-DLIBXL_API_VERSION=0x040500" dnl search for libxl, aka libxenlight - LIBVIRT_CHECK_PKG([LIBXL], [xenlight], [4.6.0]) + old_with_libxl="$with_libxl" + LIBVIRT_CHECK_PKG([LIBXL], [xenlight], [4.6.0], [true]) if test "x$with_libxl" = "xyes" ; then LIBXL_FIRMWARE_DIR=$($PKG_CONFIG --variable xenfirmwaredir xenlight) LIBXL_EXECBIN_DIR=$($PKG_CONFIG --variable libexec_bin xenlight) + fi + dnl In Fedora <= 28, the xenlight pkgconfig file is in the -runtime package + dnl https://bugzilla.redhat.com/show_bug.cgi?id=1629643 + dnl Until Fedora 28 reaches EOL, fallback to lib probe if xenlight.pc is + dnl not found + if test "x$with_libxl" = "xno" ; then +with_libxl="$old_with_libxl" + +save_CFLAGS="$CFLAGS" +CFLAGS="$CFLAGS $LIBXL_API_VERSION" +LIBVIRT_CHECK_LIB([LIBXL], [xenlight], [libxl_cpupool_cpuadd_cpumap], [libxl.h], [fail="1"]) +CFLAGS="$save_CFLAGS" + +if test $fail = 1; then + AC_MSG_ERROR([You must install the libxl Library from Xen >= 4.6 to compile libxenlight driver with -lxl]) +fi + fi + + if test "$with_libxl" = "yes"; then LIBXL_CFLAGS="$LIBXL_CFLAGS $LIBXL_API_VERSION" dnl If building with libxl, use the libxl utility header and lib too -- 2.18.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/2] build: remove unused variables from virt-driver-libxl.m4
Signed-off-by: Jim Fehlig --- m4/virt-driver-libxl.m4 | 3 --- 1 file changed, 3 deletions(-) diff --git a/m4/virt-driver-libxl.m4 b/m4/virt-driver-libxl.m4 index 70c5c59e6a..422ff27022 100644 --- a/m4/virt-driver-libxl.m4 +++ b/m4/virt-driver-libxl.m4 @@ -34,9 +34,6 @@ AC_DEFUN([LIBVIRT_DRIVER_CHECK_LIBXL], [ LIBXL_FIRMWARE_DIR=$($PKG_CONFIG --variable xenfirmwaredir xenlight) LIBXL_EXECBIN_DIR=$($PKG_CONFIG --variable libexec_bin xenlight) -old_LIBS="$LIBS" -old_CFLAGS="$CFLAGS" - LIBXL_CFLAGS="$LIBXL_CFLAGS $LIBXL_API_VERSION" dnl If building with libxl, use the libxl utility header and lib too -- 2.18.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] src: More cleanup of some system headers already contained in internal.h
On Wed, Sep 19, 2018 at 05:42:18PM +0200, Michal Privoznik wrote: > On 09/19/2018 12:22 PM, Erik Skultety wrote: > > All of the ones being removed are pulled in by internal.h. The only > > exception is sanlock which expects the application to include > > before sanlock's headers, because sanlock prototypes use fixed width > > int, but they don't include stdint.h themselves, so we have to leave > > that one in place. > > > > Signed-off-by: Erik Skultety > > --- > > Is there an automated way to verify this? I don't expect anybody going > one file after another just to see if this is correct. That would be madness. In fact what I did after sed'ing this was trying to compile it until it passed. > > I think the fact that this would pass travis/jenkins could be enough. > But there has to be a better way. That's in fact what I did, I built the repo on ubuntu-18, centos-7, fedora-28, rawhide, freebsd-11. I also verified with mingw and Clang. Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] src: More cleanup of some system headers already contained in internal.h
On 09/19/2018 12:22 PM, Erik Skultety wrote: > All of the ones being removed are pulled in by internal.h. The only > exception is sanlock which expects the application to include > before sanlock's headers, because sanlock prototypes use fixed width > int, but they don't include stdint.h themselves, so we have to leave > that one in place. > > Signed-off-by: Erik Skultety > --- Is there an automated way to verify this? I don't expect anybody going one file after another just to see if this is correct. I think the fact that this would pass travis/jenkins could be enough. But there has to be a better way. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 3/3] qemu: add memfd source type
* Marc-André Lureau (marcandre.lur...@redhat.com) wrote: > Hi > > On Wed, Sep 19, 2018 at 5:58 PM Michal Privoznik wrote: > > > > On 09/19/2018 12:03 PM, Marc-André Lureau wrote: > > > Hi > > > > > > On Wed, Sep 19, 2018 at 1:41 PM Michal Privoznik > > > wrote: > > >> > > >> On 09/17/2018 03:14 PM, marcandre.lur...@redhat.com wrote: > > >>> From: Marc-André Lureau > > >>> > > >>> Add a new memoryBacking source type "memfd", supported by QEMU (when > > >>> the apability is available). > > >>> > > >>> A memfd is a specialized anonymous memory kind. As such, an anonymous > > >>> source type could be automatically using a memfd. However, there are > > >>> some complications when migrating from different memory backends in > > >>> qemu (mainly due to the internal object naming at this point, but > > >>> there could be more). For now, it is simpler and safer to simply > > >>> introduce a new source type "memfd". Eventually, the "anonymous" type > > >>> could learn to use memfd transparently in a seperate change. > > >>> > > >>> The main benefits are that it doesn't need to create filesystem files, > > >>> and it also enforces sealing, providing a bit more safety. > > >>> > > >>> Signed-off-by: Marc-André Lureau > > >>> --- > > >>> docs/formatdomain.html.in | 9 +-- > > >>> docs/schemas/domaincommon.rng | 1 + > > >>> src/conf/domain_conf.c| 3 +- > > >>> src/conf/domain_conf.h| 1 + > > >>> src/qemu/qemu_command.c | 69 +-- > > >>> src/qemu/qemu_domain.c| 12 +++- > > >>> .../memfd-memory-numa.x86_64-latest.args | 34 + > > >>> tests/qemuxml2argvdata/memfd-memory-numa.xml | 36 ++ > > >>> tests/qemuxml2argvtest.c | 2 + > > >>> 9 files changed, 140 insertions(+), 27 deletions(-) > > >>> create mode 100644 > > >>> tests/qemuxml2argvdata/memfd-memory-numa.x86_64-latest.args > > >>> create mode 100644 tests/qemuxml2argvdata/memfd-memory-numa.xml > > >>> > > >>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > > >>> index 1f12ab5b42..1f6d40 100644 > > >>> --- a/docs/formatdomain.html.in > > >>> +++ b/docs/formatdomain.html.in > > >>> @@ -1099,7 +1099,7 @@ > > >>> /hugepages > > >>> nosharepages/ > > >>> locked/ > > >>> -source type="file|anonymous"/ > > >>> +source type="file|anonymous|memfd"/ > > >> > > >> I'm sorry but I do not think this is the way we should go. This > > >> effectively avoids libvirt making the decision and exposes the backend > > >> used directly. This puts unnecessary burden on mgmt applications because > > >> they have to make yet another decision (track another domain attribute). > > >> > > >> IIUC, memfd is like memory-backend-file and -ram combined. It can do > > >> hugepages or just plain malloc(). Therefore it should be our first > > >> choice for freshly started domains. And only if qemu doesn't support it > > >> we should fall back to either -file or -ram backends. > > > > > > memory-backend-memfd doesn't replace either -file or -ram though. It's > > > a specialized anonymous memory kind, linux-only atm, and not widely > > > available. > > > > Well, neither libvirt nor qemu really support hugepages on anything else > > than linux. > > > > Nor it ever will? Because if we merge these patches and expose it in > > domain XML, there is no turning back. We can't stop supporting it. > > > > > > > > -file should be used for nvram or complex hugepage/numa setup for ex. > > > > How come? I can see .host-nodes and .policy attributes for -memfd > > backend too. Sure, nvram is special, but for plain hugepages use case > > -file and -memfd are interchangeable, aren't they? > > Sorry, I think I misunderstood the problem then. The qemu mbind() > might do all the work. > > David, didn't you point out limitation of -memfd compared to -file for > NUMA setup? I think we came to the conclusion they're mostly the same, but with the gotcha that it's harder to control allocation with memfd. I think for example you can create a fixed size hugetlbfs mount and put a set of VMs in it and no they're limited to that size. I think you can do similar things with /dev/shm like mounts. Dave > > > > -object memory-backend-memfd,id=ram-node0,\ > > hugetlb=yes,hugetlbsize=2097152,\ > > share=yes,size=15032385536,host-nodes=3,policy=preferred > > > > -object memory-backend-file,id=ram-node0,\ > > path=/path/to/2M/hugetlfs,\ > > size=15032385536,host-nodes=3,policy=preferred > > > > > > And for -ram there is no difference from usage/libvirt POV. > > > > -object memory-backend-memfd,id=ram-node0,\ > > share=yes,size=15032385536,host-nodes=3,policy=preferred > > > > -object memory-backend-ram,id=ram-node0,\ > > size=15032385536,host-nodes=3,policy=preferred > > > > > > > > > > But it's legitimate that a VM user request memfd to be used. > > > > > > The point of this patch
Re: [libvirt] [PATCH 4/5] qemu: Map running reason to resume event detail
On 09/12/2018 08:55 AM, Jiri Denemark wrote: > Thanks to the previous commit the RESUME event handler knows what reason > should be used when changing the domain state to VIR_DOMAIN_RUNNING, but > the emitted VIR_DOMAIN_EVENT_RESUMED event still uses a generic > VIR_DOMAIN_EVENT_RESUMED_UNPAUSED detail. Luckily, the event detail can > be easily deduced from the running reason, which saves us from having to > pass one more value to the handler. > > Signed-off-by: Jiri Denemark > --- > src/qemu/qemu_domain.c | 29 + > src/qemu/qemu_domain.h | 3 +++ > src/qemu/qemu_process.c | 11 +++ > 3 files changed, 39 insertions(+), 4 deletions(-) > Reviewed-by: John Ferlan John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/5] qemu: Pass running reason to RESUME event handler
On 09/12/2018 08:55 AM, Jiri Denemark wrote:
> Whenever we get the RESUME event from QEMU, we change the state of the
> affected domain to VIR_DOMAIN_RUNNING with VIR_DOMAIN_RUNNING_UNPAUSED
> reason. This is fine if the domain is resumed unexpectedly, but when we
> sent "cont" to QEMU we usually have a better reason for the state
> change. The better reason is used in qemuProcessStartCPUs which also
> sets the domain state to running if qemuMonitorStartCPUs reports
> success. Thus we may end up with two state updates in a row, but the
> final reason is correct.
>
> This patch is a preparation for dropping the state change done in
> qemuMonitorStartCPUs for which we need to pass the actual running reason
> to the RESUME event handler and use it there instead of
> VIR_DOMAIN_RUNNING_UNPAUSED.
>
> Signed-off-by: Jiri Denemark
> ---
> src/qemu/qemu_domain.h | 4
> src/qemu/qemu_process.c | 23 +--
> 2 files changed, 21 insertions(+), 6 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index 914c9a6a8d..3f3f7ccf18 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -366,6 +366,10 @@ struct _qemuDomainObjPrivate {
>
> /* counter for generating node names for qemu disks */
> unsigned long long nodenameindex;
> +
> +/* qemuProcessStartCPUs stores the reason for starting vCPUs here for the
> + * RESUME event handler to use it */
> +virDomainRunningReason runningReason;
So what happens in the libvirtd restart case/condition? This isn't
Format/Parse'd so it's lost or essentially set to RUNNING_UNKNOWN.
> };
>
> # define QEMU_DOMAIN_PRIVATE(vm) \
The rest seems fine, I think the qemuDomainObjPrivateXML{Parse|Format}
code is "simple enough" to copy from other examples that you don't need
to respin/repost - you can show a diff.
Assuming a proper Parse/Format,
Reviewed-by: John Ferlan
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] secret: Makefile: Fix an EXTRA_DIST typo
On Wed, Sep 19, 2018 at 04:49:26PM +0200, Erik Skultety wrote: > So, when trying to add some secret util sources, we referenced them with > a non-existent symbol. > > Signed-off-by: Erik Skultety > --- I pushed this as trivial. Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 3/3] qemu: add memfd source type
Hi On Wed, Sep 19, 2018 at 5:58 PM Michal Privoznik wrote: > > On 09/19/2018 12:03 PM, Marc-André Lureau wrote: > > Hi > > > > On Wed, Sep 19, 2018 at 1:41 PM Michal Privoznik > > wrote: > >> > >> On 09/17/2018 03:14 PM, marcandre.lur...@redhat.com wrote: > >>> From: Marc-André Lureau > >>> > >>> Add a new memoryBacking source type "memfd", supported by QEMU (when > >>> the apability is available). > >>> > >>> A memfd is a specialized anonymous memory kind. As such, an anonymous > >>> source type could be automatically using a memfd. However, there are > >>> some complications when migrating from different memory backends in > >>> qemu (mainly due to the internal object naming at this point, but > >>> there could be more). For now, it is simpler and safer to simply > >>> introduce a new source type "memfd". Eventually, the "anonymous" type > >>> could learn to use memfd transparently in a seperate change. > >>> > >>> The main benefits are that it doesn't need to create filesystem files, > >>> and it also enforces sealing, providing a bit more safety. > >>> > >>> Signed-off-by: Marc-André Lureau > >>> --- > >>> docs/formatdomain.html.in | 9 +-- > >>> docs/schemas/domaincommon.rng | 1 + > >>> src/conf/domain_conf.c| 3 +- > >>> src/conf/domain_conf.h| 1 + > >>> src/qemu/qemu_command.c | 69 +-- > >>> src/qemu/qemu_domain.c| 12 +++- > >>> .../memfd-memory-numa.x86_64-latest.args | 34 + > >>> tests/qemuxml2argvdata/memfd-memory-numa.xml | 36 ++ > >>> tests/qemuxml2argvtest.c | 2 + > >>> 9 files changed, 140 insertions(+), 27 deletions(-) > >>> create mode 100644 > >>> tests/qemuxml2argvdata/memfd-memory-numa.x86_64-latest.args > >>> create mode 100644 tests/qemuxml2argvdata/memfd-memory-numa.xml > >>> > >>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > >>> index 1f12ab5b42..1f6d40 100644 > >>> --- a/docs/formatdomain.html.in > >>> +++ b/docs/formatdomain.html.in > >>> @@ -1099,7 +1099,7 @@ > >>> /hugepages > >>> nosharepages/ > >>> locked/ > >>> -source type="file|anonymous"/ > >>> +source type="file|anonymous|memfd"/ > >> > >> I'm sorry but I do not think this is the way we should go. This > >> effectively avoids libvirt making the decision and exposes the backend > >> used directly. This puts unnecessary burden on mgmt applications because > >> they have to make yet another decision (track another domain attribute). > >> > >> IIUC, memfd is like memory-backend-file and -ram combined. It can do > >> hugepages or just plain malloc(). Therefore it should be our first > >> choice for freshly started domains. And only if qemu doesn't support it > >> we should fall back to either -file or -ram backends. > > > > memory-backend-memfd doesn't replace either -file or -ram though. It's > > a specialized anonymous memory kind, linux-only atm, and not widely > > available. > > Well, neither libvirt nor qemu really support hugepages on anything else > than linux. > > Nor it ever will? Because if we merge these patches and expose it in > domain XML, there is no turning back. We can't stop supporting it. > > > > > -file should be used for nvram or complex hugepage/numa setup for ex. > > How come? I can see .host-nodes and .policy attributes for -memfd > backend too. Sure, nvram is special, but for plain hugepages use case > -file and -memfd are interchangeable, aren't they? Sorry, I think I misunderstood the problem then. The qemu mbind() might do all the work. David, didn't you point out limitation of -memfd compared to -file for NUMA setup? > > -object memory-backend-memfd,id=ram-node0,\ > hugetlb=yes,hugetlbsize=2097152,\ > share=yes,size=15032385536,host-nodes=3,policy=preferred > > -object memory-backend-file,id=ram-node0,\ > path=/path/to/2M/hugetlfs,\ > size=15032385536,host-nodes=3,policy=preferred > > > And for -ram there is no difference from usage/libvirt POV. > > -object memory-backend-memfd,id=ram-node0,\ > share=yes,size=15032385536,host-nodes=3,policy=preferred > > -object memory-backend-ram,id=ram-node0,\ > size=15032385536,host-nodes=3,policy=preferred > > > > > > But it's legitimate that a VM user request memfd to be used. > > > > The point of this patch is not to say that we shouldn't try to use > > memfd when possible, but rather let the user request specifically > > memfd, for security reasons for example. If the setup cannot be > > satisfied with -memfd, the user should get an error. > > What security reasons do you have in mind? grow/shrink sealing (and avoiding somewhat hazardous file system operations). > > > > >> > >> This means we have to track what backend the domain was started with so > >> that we preserve that on migration (although, the fact that these > >> backends are not interchangeable makes me question 'backend' in their > >>
Re: [libvirt] [PATCH 2/5] qemu: Report more appropriate running reasons
On 09/12/2018 08:55 AM, Jiri Denemark wrote: > This patch replaces some rather generic VIR_DOMAIN_RUNNING_UNPAUSED > reasons when changing domain state to running with more specific ones. These are done in the (ahem) event that libvirtd was restarted before the migration was completed and would have called the *StartCPUs... > > Signed-off-by: Jiri Denemark > --- > src/qemu/qemu_process.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > You could augment the commit message, but it's not that important. Reviewed-by: John Ferlan John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] secret: Makefile: Fix an EXTRA_DIST typo
So, when trying to add some secret util sources, we referenced them with a non-existent symbol. Signed-off-by: Erik Skultety --- src/secret/Makefile.inc.am | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/secret/Makefile.inc.am b/src/secret/Makefile.inc.am index 305c4a1ead..79c2b2d74a 100644 --- a/src/secret/Makefile.inc.am +++ b/src/secret/Makefile.inc.am @@ -13,7 +13,7 @@ DRIVER_SOURCE_FILES += $(SECRET_DRIVER_SOURCES) STATEFUL_DRIVER_SOURCE_FILES += $(SECRET_DRIVER_SOURCES) EXTRA_DIST += \ $(SECRET_DRIVER_SOURCES) \ - $(SECRET_UTIL_SOURCESQ) \ + $(SECRET_UTIL_SOURCES) \ $(NULL) noinst_LTLIBRARIES += libvirt_secret.la -- 2.17.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/5] qemu: Properly report VIR_DOMAIN_EVENT_RESUMED_FROM_SNAPSHOT
On 09/12/2018 08:55 AM, Jiri Denemark wrote: > VIR_DOMAIN_EVENT_RESUMED_FROM_SNAPSHOT was defined but not used anywhere > in our event generation code. This fixes qemuDomainRevertToSnapshot to > properly report why the domain was resumed. > > Signed-off-by: Jiri Denemark > --- > src/qemu/qemu_driver.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > Reviewed-by: John Ferlan John So the event detail was introduced by commit c1ff5dc63d, but never added to anything. Even when commit 88fe7a4b originally implemented the "was_stopped" as STARTED_FROM_SNAPSHOT. When I changed the logic in commit 5efcfb96 it doesn't seem I was considering the "detail", but rather just the event. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 3/3] qemu: add memfd source type
Hi On Wed, Sep 19, 2018 at 5:31 PM Michal Privoznik wrote: > > On 09/19/2018 01:43 PM, Jiri Denemark wrote: > BTW: what features does this new memfd backend provides that can't be > achieved via traditional -file or -ram backends? 2 things stand out: - no need for files & mountpoint - sealing, preventing grow/srink (qemu doesn't seal write) memfd is thus a better way to share anonymous memory between processes on linux (when you don't have other weird constrains). Read also the initial post: https://dvdhrm.wordpress.com/tag/memfd/. Since then memfd has grown a few more features, such as hugetlb support. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3] qemu: Ignore nwfilter binding instantiation issues during reconnect
ping^3?
Tks
John
On 09/06/2018 07:03 PM, John Ferlan wrote:
>
> ping^2?
>
> Tks,
>
> John
>
> These are essentially what Daniel and I agreed upon during the v2 review
> - link the patches below...
>
> On 08/30/2018 06:24 PM, John Ferlan wrote:
>> ping?
>>
>> Tks,
>>
>> John
>>
>> On 08/24/2018 10:02 AM, John Ferlan wrote:
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1607202
>>>
>>> It's essentially stated in the nwfilterBindingDelete that we
>>> will allow the admin to shoot themselves in the foot by deleting
>>> the nwfilter binding which then allows them to undefine the
>>> nwfilter that is in use for the running guest...
>>>
>>> However, by allowing this we cause a problem for libvirtd
>>> restart reconnect processing which would then try to recreate
>>> the missing binding attempting to use the deleted filter
>>> resulting in an error and thus shutting the guest down.
>>>
>>> So rather than keep adding virDomainConfNWFilterInstantiate
>>> flags to "ignore" specific error conditions, modify the logic
>>> to ignore, but VIR_WARN errors other than ignoreExists. This
>>> will at least allow the guest to not shutdown for only nwfilter
>>> binding errors that we can now perhaps recover from since we
>>> have the binding create/delete capability.
>>>
>>> Signed-off-by: John Ferlan
>>> ---
>>>
>>> v2: https://www.redhat.com/archives/libvir-list/2018-August/msg01567.html
>>>
>>> Differences to v2. Leave the ignoreExists bool, but just allow and
>>> VIR_WARN other errors from virDomainConfNWFilterInstantiate. Continue
>>> processing all filters from error point too.
>>>
>>> src/qemu/qemu_process.c | 24
>>> 1 file changed, 16 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>>> index ab749389ee..61a277f468 100644
>>> --- a/src/qemu/qemu_process.c
>>> +++ b/src/qemu/qemu_process.c
>>> @@ -3160,20 +3160,29 @@ qemuProcessNotifyNets(virDomainDefPtr def)
>>> }
>>> }
>>>
>>> -static int
>>> -qemuProcessFiltersInstantiate(virDomainDefPtr def, bool ignoreExists)
>>> +/* Attempt to instantiate the filters. Ignore failures because it's
>>> + * possible that someone deleted a filter binding and the associated
>>> + * filter while the guest was running and we don't want that action
>>> + * to cause failure to keep the guest running during the reconnection
>>> + * processing. Nor do we necessarily want other failures to do the
>>> + * same. We'll just log the error conditions other than of course
>>> + * ignoreExists possibility (e.g. the true flag) */
>>> +static void
>>> +qemuProcessFiltersInstantiate(virDomainDefPtr def)
>>> {
>>> size_t i;
>>>
>>> for (i = 0; i < def->nnets; i++) {
>>> virDomainNetDefPtr net = def->nets[i];
>>> if ((net->filter) && (net->ifname)) {
>>> -if (virDomainConfNWFilterInstantiate(def->name, def->uuid,
>>> net, ignoreExists) < 0)
>>> -return 1;
>>> +if (virDomainConfNWFilterInstantiate(def->name, def->uuid, net,
>>> + true) < 0) {
>>> +VIR_WARN("filter '%s' instantiation for '%s' failed '%s'",
>>> + net->filter, net->ifname,
>>> virGetLastErrorMessage());
>>> +virResetLastError();
>>> +}
>>> }
>>> }
>>> -
>>> -return 0;
>>> }
>>>
>>> static int
>>> @@ -7892,8 +7901,7 @@ qemuProcessReconnect(void *opaque)
>>>
>>> qemuProcessNotifyNets(obj->def);
>>>
>>> -if (qemuProcessFiltersInstantiate(obj->def, true))
>>> -goto error;
>>> +qemuProcessFiltersInstantiate(obj->def);
>>>
>>> if (qemuProcessRefreshDisks(driver, obj, QEMU_ASYNC_JOB_NONE) < 0)
>>> goto error;
>>>
>>
>> --
>> libvir-list mailing list
>> libvir-list@redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
>>
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 3/3] qemu: add memfd source type
On 09/19/2018 12:03 PM, Marc-André Lureau wrote: > Hi > > On Wed, Sep 19, 2018 at 1:41 PM Michal Privoznik wrote: >> >> On 09/17/2018 03:14 PM, marcandre.lur...@redhat.com wrote: >>> From: Marc-André Lureau >>> >>> Add a new memoryBacking source type "memfd", supported by QEMU (when >>> the apability is available). >>> >>> A memfd is a specialized anonymous memory kind. As such, an anonymous >>> source type could be automatically using a memfd. However, there are >>> some complications when migrating from different memory backends in >>> qemu (mainly due to the internal object naming at this point, but >>> there could be more). For now, it is simpler and safer to simply >>> introduce a new source type "memfd". Eventually, the "anonymous" type >>> could learn to use memfd transparently in a seperate change. >>> >>> The main benefits are that it doesn't need to create filesystem files, >>> and it also enforces sealing, providing a bit more safety. >>> >>> Signed-off-by: Marc-André Lureau >>> --- >>> docs/formatdomain.html.in | 9 +-- >>> docs/schemas/domaincommon.rng | 1 + >>> src/conf/domain_conf.c| 3 +- >>> src/conf/domain_conf.h| 1 + >>> src/qemu/qemu_command.c | 69 +-- >>> src/qemu/qemu_domain.c| 12 +++- >>> .../memfd-memory-numa.x86_64-latest.args | 34 + >>> tests/qemuxml2argvdata/memfd-memory-numa.xml | 36 ++ >>> tests/qemuxml2argvtest.c | 2 + >>> 9 files changed, 140 insertions(+), 27 deletions(-) >>> create mode 100644 >>> tests/qemuxml2argvdata/memfd-memory-numa.x86_64-latest.args >>> create mode 100644 tests/qemuxml2argvdata/memfd-memory-numa.xml >>> >>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in >>> index 1f12ab5b42..1f6d40 100644 >>> --- a/docs/formatdomain.html.in >>> +++ b/docs/formatdomain.html.in >>> @@ -1099,7 +1099,7 @@ >>> /hugepages >>> nosharepages/ >>> locked/ >>> -source type="file|anonymous"/ >>> +source type="file|anonymous|memfd"/ >> >> I'm sorry but I do not think this is the way we should go. This >> effectively avoids libvirt making the decision and exposes the backend >> used directly. This puts unnecessary burden on mgmt applications because >> they have to make yet another decision (track another domain attribute). >> >> IIUC, memfd is like memory-backend-file and -ram combined. It can do >> hugepages or just plain malloc(). Therefore it should be our first >> choice for freshly started domains. And only if qemu doesn't support it >> we should fall back to either -file or -ram backends. > > memory-backend-memfd doesn't replace either -file or -ram though. It's > a specialized anonymous memory kind, linux-only atm, and not widely > available. Well, neither libvirt nor qemu really support hugepages on anything else than linux. Nor it ever will? Because if we merge these patches and expose it in domain XML, there is no turning back. We can't stop supporting it. > > -file should be used for nvram or complex hugepage/numa setup for ex. How come? I can see .host-nodes and .policy attributes for -memfd backend too. Sure, nvram is special, but for plain hugepages use case -file and -memfd are interchangeable, aren't they? -object memory-backend-memfd,id=ram-node0,\ hugetlb=yes,hugetlbsize=2097152,\ share=yes,size=15032385536,host-nodes=3,policy=preferred -object memory-backend-file,id=ram-node0,\ path=/path/to/2M/hugetlfs,\ size=15032385536,host-nodes=3,policy=preferred And for -ram there is no difference from usage/libvirt POV. -object memory-backend-memfd,id=ram-node0,\ share=yes,size=15032385536,host-nodes=3,policy=preferred -object memory-backend-ram,id=ram-node0,\ size=15032385536,host-nodes=3,policy=preferred > > But it's legitimate that a VM user request memfd to be used. > > The point of this patch is not to say that we shouldn't try to use > memfd when possible, but rather let the user request specifically > memfd, for security reasons for example. If the setup cannot be > satisfied with -memfd, the user should get an error. What security reasons do you have in mind? > >> >> This means we have to track what backend the domain was started with so >> that we preserve that on migration (although, the fact that these >> backends are not interchangeable makes me question 'backend' in their >> name :-P). For that we can use status/migration XML as I suggested earlier. >> >> Once again, status XML is not editable by user [*] and is used solely by >> libvirtd to store runtime information for a running domain (and backend >> used falls into that category). > > Why not do this transparent memfd-usage in a seperate series? Depends what we want libvirt to be. If we want it to be mere XML->qemu cmd line generator, then we can expose all qemu settings as they are. If we want it to have some logic built in (so
Re: [libvirt] [PATCH v2 3/3] qemu: add memfd source type
On 09/19/2018 01:43 PM, Jiri Denemark wrote: > On Wed, Sep 19, 2018 at 13:10:42 +0200, Michal Privoznik wrote: >> On 09/19/2018 12:02 PM, Pavel Hrdina wrote: >>> One thing about the migration though. I'm not sure what are we >>> officially supporting in libvirt because it might cause us some issues. >>> >>> We need to make sure that if you live-migrate domain from old libvirt >>> to new libvirt you should be able to migrate that domain back to old >>> libvirt. The question is, whether this applies if you destroy and >>> start the domain on the new libvirt before you live-migrate it back >>> to old libvirt. >>> >>> Without the restart there is no issue, because the backend would not >>> be changed, but once you start the same domain again we would pick >>> new backend which would prevent migrating it back to the old libvirt. >>> >> >> This is not supported. Exactly because of this reason. If we were to >> preserve this forward compatibility (i.e. migration from newer libvirt >> to older) then we couldn't use any new feature qemu has. For instance, >> if qemu introduces a new device and a user starts a domain with it, >> migration to older qemu will not work then, obviously. This also applies >> for other qemu capabilities. >> >> Migrating from newer version to older is not supported. It might work, >> but that's rather coincidence than intent. > > Not really. The key point is whether a user explicitly asked for the new > feature. In other words, taking an old XML usable with old libvirt and > starting it on new libvirt should result in a domain which can be > migrated back to the old version. > > Think about oVirt and its transient domains which are always started > from scratch using a generated XML. They need to be able to support > heterogeneous clusters where some hosts run an old OS while some were > already updated to a newer version of the host OS (and libvirt). Both > existing and newly started domains should be migratable to any host in > the cluster no matter when they were initially started. Unless of course > oVirt's cluster level (or what the name for it is) is upgraded from at > which point all hosts need to support new features. > > So much for a theory. I think have bugs which prevent such migration > compatibility in some specific cases. I'm not sure whether we > intentionally broke this in the past, but I think we should avoid > breaking it if possible. Well, in that case we don't have a good option. If we have say three options to chose from (say mem backend to use), and they are not interchangeable, the moment we chose one the domain is not migratable. Yet, when starting the domain we want to give our users the best option available. BTW: what features does this new memfd backend provides that can't be achieved via traditional -file or -ram backends? Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 3/3] qemu: add memfd source type
On 09/19/2018 07:10 AM, Michal Privoznik wrote: > On 09/19/2018 12:02 PM, Pavel Hrdina wrote: >> On Wed, Sep 19, 2018 at 11:41:11AM +0200, Michal Privoznik wrote: >>> On 09/17/2018 03:14 PM, marcandre.lur...@redhat.com wrote: From: Marc-André Lureau Add a new memoryBacking source type "memfd", supported by QEMU (when the apability is available). A memfd is a specialized anonymous memory kind. As such, an anonymous source type could be automatically using a memfd. However, there are some complications when migrating from different memory backends in qemu (mainly due to the internal object naming at this point, but there could be more). For now, it is simpler and safer to simply introduce a new source type "memfd". Eventually, the "anonymous" type could learn to use memfd transparently in a seperate change. The main benefits are that it doesn't need to create filesystem files, and it also enforces sealing, providing a bit more safety. Signed-off-by: Marc-André Lureau --- docs/formatdomain.html.in | 9 +-- docs/schemas/domaincommon.rng | 1 + src/conf/domain_conf.c| 3 +- src/conf/domain_conf.h| 1 + src/qemu/qemu_command.c | 69 +-- src/qemu/qemu_domain.c| 12 +++- .../memfd-memory-numa.x86_64-latest.args | 34 + tests/qemuxml2argvdata/memfd-memory-numa.xml | 36 ++ tests/qemuxml2argvtest.c | 2 + 9 files changed, 140 insertions(+), 27 deletions(-) create mode 100644 tests/qemuxml2argvdata/memfd-memory-numa.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/memfd-memory-numa.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 1f12ab5b42..1f6d40 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1099,7 +1099,7 @@ /hugepages nosharepages/ locked/ -source type="file|anonymous"/ +source type="file|anonymous|memfd"/ >>> >>> I'm sorry but I do not think this is the way we should go. This >>> effectively avoids libvirt making the decision and exposes the backend >>> used directly. This puts unnecessary burden on mgmt applications because >>> they have to make yet another decision (track another domain attribute). >>> >>> IIUC, memfd is like memory-backend-file and -ram combined. It can do >>> hugepages or just plain malloc(). Therefore it should be our first >>> choice for freshly started domains. And only if qemu doesn't support it >>> we should fall back to either -file or -ram backends. >>> >>> This means we have to track what backend the domain was started with so >>> that we preserve that on migration (although, the fact that these >>> backends are not interchangeable makes me question 'backend' in their >>> name :-P). For that we can use status/migration XML as I suggested earlier. >>> >>> Once again, status XML is not editable by user [*] and is used solely by >>> libvirtd to store runtime information for a running domain (and backend >>> used falls into that category). >> >> I have to agree with Michal, we should not expose this implementation >> detail in domain XML if we can hide it in status/migratable XML. >> >> One thing about the migration though. I'm not sure what are we >> officially supporting in libvirt because it might cause us some issues. >> >> We need to make sure that if you live-migrate domain from old libvirt >> to new libvirt you should be able to migrate that domain back to old >> libvirt. The question is, whether this applies if you destroy and >> start the domain on the new libvirt before you live-migrate it back >> to old libvirt. >> >> Without the restart there is no issue, because the backend would not >> be changed, but once you start the same domain again we would pick >> new backend which would prevent migrating it back to the old libvirt. >> > > This is not supported. Exactly because of this reason. If we were to > preserve this forward compatibility (i.e. migration from newer libvirt > to older) then we couldn't use any new feature qemu has. For instance, > if qemu introduces a new device and a user starts a domain with it, > migration to older qemu will not work then, obviously. This also applies > for other qemu capabilities. > > Migrating from newer version to older is not supported. It might work, > but that's rather coincidence than intent. > Tough time to decide which of these to reply to... So I'll go here. Anyway, perhaps it should be noted that in V1 the "decision" was to force the consumer to select "anonymous" as their source type. Go back to patch 3: +if (!mem->nvdimmPath && +def->mem.source == VIR_DOMAIN_MEMORY_SOURCE_ANONYMOUS && +
Re: [libvirt] [PATCH v2 3/3] qemu: add memfd source type
On Wed, Sep 19, 2018 at 13:10:42 +0200, Michal Privoznik wrote: > On 09/19/2018 12:02 PM, Pavel Hrdina wrote: > > One thing about the migration though. I'm not sure what are we > > officially supporting in libvirt because it might cause us some issues. > > > > We need to make sure that if you live-migrate domain from old libvirt > > to new libvirt you should be able to migrate that domain back to old > > libvirt. The question is, whether this applies if you destroy and > > start the domain on the new libvirt before you live-migrate it back > > to old libvirt. > > > > Without the restart there is no issue, because the backend would not > > be changed, but once you start the same domain again we would pick > > new backend which would prevent migrating it back to the old libvirt. > > > > This is not supported. Exactly because of this reason. If we were to > preserve this forward compatibility (i.e. migration from newer libvirt > to older) then we couldn't use any new feature qemu has. For instance, > if qemu introduces a new device and a user starts a domain with it, > migration to older qemu will not work then, obviously. This also applies > for other qemu capabilities. > > Migrating from newer version to older is not supported. It might work, > but that's rather coincidence than intent. Not really. The key point is whether a user explicitly asked for the new feature. In other words, taking an old XML usable with old libvirt and starting it on new libvirt should result in a domain which can be migrated back to the old version. Think about oVirt and its transient domains which are always started from scratch using a generated XML. They need to be able to support heterogeneous clusters where some hosts run an old OS while some were already updated to a newer version of the host OS (and libvirt). Both existing and newly started domains should be migratable to any host in the cluster no matter when they were initially started. Unless of course oVirt's cluster level (or what the name for it is) is upgraded from at which point all hosts need to support new features. So much for a theory. I think have bugs which prevent such migration compatibility in some specific cases. I'm not sure whether we intentionally broke this in the past, but I think we should avoid breaking it if possible. Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/9] cgroup cleanups and preparation for v2
Pavel, On Tue, Sep 18, 2018 at 2:17 PM, Pavel Hrdina wrote: > Pavel Hrdina (9): > vircgroup: cleanup controllers not managed by systemd on error > vircgroup: fix bug in virCgroupEnableMissingControllers > vircgroup: rename virCgroupAdd.*Task to virCgroupAdd.*Process > vircgroup: introduce virCgroupTaskFlags > vircgroup: introduce virCgroupAddThread > vircgroupmock: cleanup unused cgroup files > vircgroupmock: rewrite cgroup fopen mocking > vircgrouptest: call virCgroupDetectMounts directly > vircgrouptest: call virCgroupNewSelf instead virCgroupDetectMounts > > src/libvirt-lxc.c | 2 +- > src/libvirt_private.syms | 6 +- > src/lxc/lxc_controller.c | 4 +- > src/qemu/qemu_process.c | 4 +- > src/qemu/qemu_tpm.c | 2 +- > src/util/vircgroup.c | 135 +++- > src/util/vircgroup.h | 5 +- > src/util/vircgrouppriv.h | 4 - > tests/vircgroupdata/all-in-one.cgroups| 7 + > tests/vircgroupdata/all-in-one.mounts | 2 +- > tests/vircgroupdata/all-in-one.parsed | 12 +- > tests/vircgroupdata/all-in-one.self.cgroup| 1 + > tests/vircgroupdata/cgroups1.cgroups | 11 + > tests/vircgroupdata/cgroups1.self.cgroup | 11 + > tests/vircgroupdata/cgroups2.cgroups | 10 + > tests/vircgroupdata/cgroups2.self.cgroup | 10 + > tests/vircgroupdata/cgroups3.cgroups | 12 + > tests/vircgroupdata/cgroups3.self.cgroup | 12 + > tests/vircgroupdata/fedora-18.cgroups | 10 + > tests/vircgroupdata/fedora-18.self.cgroup | 9 + > tests/vircgroupdata/fedora-21.cgroups | 12 + > tests/vircgroupdata/fedora-21.self.cgroup | 10 + > tests/vircgroupdata/kubevirt.cgroups | 10 + > tests/vircgroupdata/kubevirt.self.cgroup | 10 + > tests/vircgroupdata/logind.cgroups| 10 + > tests/vircgroupdata/logind.mounts | 2 + > tests/vircgroupdata/logind.self.cgroup| 1 + > tests/vircgroupdata/ovirt-node-6.6.cgroups| 9 + > .../vircgroupdata/ovirt-node-6.6.self.cgroup | 8 + > tests/vircgroupdata/ovirt-node-7.1.cgroups| 11 + > .../vircgroupdata/ovirt-node-7.1.self.cgroup | 10 + > tests/vircgroupdata/rhel-7.1.cgroups | 11 + > tests/vircgroupdata/rhel-7.1.self.cgroup | 10 + > tests/vircgroupdata/systemd.cgroups | 8 + > tests/vircgroupdata/systemd.mounts| 11 + > tests/vircgroupdata/systemd.self.cgroup | 6 + > tests/vircgroupmock.c | 206 ++ > tests/vircgrouptest.c | 24 +- > 38 files changed, 362 insertions(+), 276 deletions(-) > create mode 100644 tests/vircgroupdata/all-in-one.cgroups > create mode 100644 tests/vircgroupdata/all-in-one.self.cgroup > create mode 100644 tests/vircgroupdata/cgroups1.cgroups > create mode 100644 tests/vircgroupdata/cgroups1.self.cgroup > create mode 100644 tests/vircgroupdata/cgroups2.cgroups > create mode 100644 tests/vircgroupdata/cgroups2.self.cgroup > create mode 100644 tests/vircgroupdata/cgroups3.cgroups > create mode 100644 tests/vircgroupdata/cgroups3.self.cgroup > create mode 100644 tests/vircgroupdata/fedora-18.cgroups > create mode 100644 tests/vircgroupdata/fedora-18.self.cgroup > create mode 100644 tests/vircgroupdata/fedora-21.cgroups > create mode 100644 tests/vircgroupdata/fedora-21.self.cgroup > create mode 100644 tests/vircgroupdata/kubevirt.cgroups > create mode 100644 tests/vircgroupdata/kubevirt.self.cgroup > create mode 100644 tests/vircgroupdata/logind.cgroups > create mode 100644 tests/vircgroupdata/logind.mounts > create mode 100644 tests/vircgroupdata/logind.self.cgroup > create mode 100644 tests/vircgroupdata/ovirt-node-6.6.cgroups > create mode 100644 tests/vircgroupdata/ovirt-node-6.6.self.cgroup > create mode 100644 tests/vircgroupdata/ovirt-node-7.1.cgroups > create mode 100644 tests/vircgroupdata/ovirt-node-7.1.self.cgroup > create mode 100644 tests/vircgroupdata/rhel-7.1.cgroups > create mode 100644 tests/vircgroupdata/rhel-7.1.self.cgroup > create mode 100644 tests/vircgroupdata/systemd.cgroups > create mode 100644 tests/vircgroupdata/systemd.mounts > create mode 100644 tests/vircgroupdata/systemd.self.cgroup > > -- > 2.17.1 > > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list > I went through your series and the patches are mostly looking there. There are a few comments in the series related to some typos and an actual problem in the last patch. Please, do *not* push the reviewed patches already as I'd give them the chance to be reviewed by someone who's more experienced with this piece of code. Let me know when you submit a v2 and I'll just check the 9th patch. Best Regards, --
Re: [libvirt] [PATCH 8/9] vircgrouptest: call virCgroupDetectMounts directly
On Tue, Sep 18, 2018 at 2:18 PM, Pavel Hrdina wrote:
> Because we can set which files to return for cgroup tests there
> is no need to have special function tailored to run tests.
>
> Signed-off-by: Pavel Hrdina
> ---
> src/libvirt_private.syms | 2 +-
> src/util/vircgroup.c | 21 +
> src/util/vircgrouppriv.h | 4 +---
> tests/vircgrouptest.c| 16
> 4 files changed, 15 insertions(+), 28 deletions(-)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index ad7ce57b65..7f3b5738c4 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1513,7 +1513,7 @@ virCgroupDelThread;
> virCgroupDenyAllDevices;
> virCgroupDenyDevice;
> virCgroupDenyDevicePath;
> -virCgroupDetectMountsFromFile;
> +virCgroupDetectMounts;
> virCgroupFree;
> virCgroupGetBlkioDeviceReadBps;
> virCgroupGetBlkioDeviceReadIops;
> diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
> index 3211f63cb1..f8ef76136b 100644
> --- a/src/util/vircgroup.c
> +++ b/src/util/vircgroup.c
> @@ -430,9 +430,7 @@ virCgroupMountOptsMatchController(const char *mntOpts,
> * mounted and where
> */
> int
> -virCgroupDetectMountsFromFile(virCgroupPtr group,
> - const char *path,
> - bool checkLinks)
> +virCgroupDetectMounts(virCgroupPtr group)
> {
> size_t i;
> FILE *mounts = NULL;
> @@ -440,9 +438,9 @@ virCgroupDetectMountsFromFile(virCgroupPtr group,
> char buf[CGROUP_MAX_VAL];
> int ret = -1;
>
> -mounts = fopen(path, "r");
> +mounts = fopen("/proc/mounts", "r");
> if (mounts == NULL) {
> -virReportSystemError(errno, _("Unable to open %s"), path);
> +virReportSystemError(errno, "%s", _("Unable to open
> /proc/mounts"));
> return -1;
> }
>
> @@ -470,8 +468,7 @@ virCgroupDetectMountsFromFile(virCgroupPtr group,
>
> /* If it is a co-mount it has a filename like
> "cpu,cpuacct"
> * and we must identify the symlink path */
> -if (checkLinks &&
> -virCgroupResolveMountLink(entry.mnt_dir, typestr,
> +if (virCgroupResolveMountLink(entry.mnt_dir, typestr,
>controller) < 0) {
> goto cleanup;
> }
> @@ -485,12 +482,6 @@ virCgroupDetectMountsFromFile(virCgroupPtr group,
> return ret;
> }
>
> -static int
> -virCgroupDetectMounts(virCgroupPtr group)
> -{
> -return virCgroupDetectMountsFromFile(group, "/proc/mounts", true);
> -}
> -
>
> static int
> virCgroupCopyPlacement(virCgroupPtr group,
> @@ -4082,9 +4073,7 @@ virCgroupAvailable(void)
>
>
> int
> -virCgroupDetectMountsFromFile(virCgroupPtr group ATTRIBUTE_UNUSED,
> - const char *path ATTRIBUTE_UNUSED,
> - bool checkLinks ATTRIBUTE_UNUSED)
> +virCgroupDetectMounts(virCgroupPtr group ATTRIBUTE_UNUSED)
> {
> virReportSystemError(ENXIO, "%s",
> _("Control groups not supported on this
> platform"));
> diff --git a/src/util/vircgrouppriv.h b/src/util/vircgrouppriv.h
> index a0034f3889..f78fe8bb9c 100644
> --- a/src/util/vircgrouppriv.h
> +++ b/src/util/vircgrouppriv.h
> @@ -50,9 +50,7 @@ struct _virCgroup {
> virCgroupController controllers[VIR_CGROUP_CONTROLLER_LAST];
> };
>
> -int virCgroupDetectMountsFromFile(virCgroupPtr group,
> - const char *path,
> - bool checkLinks);
> +int virCgroupDetectMounts(virCgroupPtr group);
>
> int virCgroupNewPartition(const char *path,
>bool create,
> diff --git a/tests/vircgrouptest.c b/tests/vircgrouptest.c
> index 7968903cad..6a38091a86 100644
> --- a/tests/vircgrouptest.c
> +++ b/tests/vircgrouptest.c
> @@ -164,21 +164,21 @@ testCgroupDetectMounts(const void *args)
> {
> int result = -1;
> const char *file = args;
> -char *mounts = NULL;
> char *parsed = NULL;
> const char *actual;
> virCgroupPtr group = NULL;
> virBuffer buf = VIR_BUFFER_INITIALIZER;
> size_t i;
>
> -if (virAsprintf(, "%s/vircgroupdata/%s.mounts",
> -abs_srcdir, file) < 0 ||
> -virAsprintf(, "%s/vircgroupdata/%s.parsed",
> -abs_srcdir, file) < 0 ||
> -VIR_ALLOC(group) < 0)
> +setenv("VIR_CGROUP_MOCK_FILENAME", file, 1);
> +
> +if (virAsprintf(, "%s/vircgroupdata/%s.parsed", abs_srcdir,
> file) < 0)
> +goto cleanup;
> +
> +if (VIR_ALLOC(group) < 0)
>
Just a comment ... although there was no need to split this in two
different checks, I do believe it's cleaner to read.
> goto cleanup;
>
> -if (virCgroupDetectMountsFromFile(group, mounts, false) < 0)
> +if (virCgroupDetectMounts(group) < 0)
> goto cleanup;
>
> for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) {
> @@ -196,7 +196,7
Re: [libvirt] [PATCH 7/9] vircgroupmock: rewrite cgroup fopen mocking
On Tue, Sep 18, 2018 at 2:17 PM, Pavel Hrdina wrote: > Move all the cgroup data into separate files out of vircgroupmock.c > and rework the fopen function to load data from files. This will > make it easier to add more test cases. > > Signed-off-by: Pavel Hrdina > Reviewed-by: Fabiano Fidêncio > --- > tests/vircgroupdata/all-in-one.cgroups | 7 ++ > tests/vircgroupdata/all-in-one.mounts | 2 +- > tests/vircgroupdata/all-in-one.parsed | 12 +- > tests/vircgroupdata/all-in-one.self.cgroup | 1 + > tests/vircgroupdata/logind.cgroups | 10 ++ > tests/vircgroupdata/logind.mounts | 2 + > tests/vircgroupdata/logind.self.cgroup | 1 + > tests/vircgroupdata/systemd.cgroups| 8 ++ > tests/vircgroupdata/systemd.mounts | 11 ++ > tests/vircgroupdata/systemd.self.cgroup| 6 + > tests/vircgroupmock.c | 133 - > tests/vircgrouptest.c | 10 +- > 12 files changed, 79 insertions(+), 124 deletions(-) > create mode 100644 tests/vircgroupdata/all-in-one.cgroups > create mode 100644 tests/vircgroupdata/all-in-one.self.cgroup > create mode 100644 tests/vircgroupdata/logind.cgroups > create mode 100644 tests/vircgroupdata/logind.mounts > create mode 100644 tests/vircgroupdata/logind.self.cgroup > create mode 100644 tests/vircgroupdata/systemd.cgroups > create mode 100644 tests/vircgroupdata/systemd.mounts > create mode 100644 tests/vircgroupdata/systemd.self.cgroup > > diff --git a/tests/vircgroupdata/all-in-one.cgroups > b/tests/vircgroupdata/all-in-one.cgroups > new file mode 100644 > index 00..7208e5a0b6 > --- /dev/null > +++ b/tests/vircgroupdata/all-in-one.cgroups > @@ -0,0 +1,7 @@ > +#subsys_namehierarchy num_cgroups enabled > +cpuset 6 1 1 > +cpu 6 1 1 > +cpuacct 6 1 1 > +memory 6 1 1 > +devices 6 1 1 > +blkio6 1 1 > diff --git a/tests/vircgroupdata/all-in-one.mounts > b/tests/vircgroupdata/all-in-one.mounts > index 14093b961c..76c579ff69 100644 > --- a/tests/vircgroupdata/all-in-one.mounts > +++ b/tests/vircgroupdata/all-in-one.mounts > @@ -4,4 +4,4 @@ proc /proc proc rw,nosuid,nodev,noexec,relatime 0 0 > udev /dev devtmpfs rw,relatime,size=16458560k,nr_inodes=4114640,mode=755 > 0 0 > devpts /dev/pts devpts rw,nosuid,noexec,relatime,gid=5,mode=620,ptmxmode=000 > 0 0 > nfsd /proc/fs/nfsd nfsd rw,relatime 0 0 > -cgroup /sys/fs/cgroup cgroup > rw,relatime,blkio,devices,memory,cpuacct,cpu,cpuset > 0 0 > +cgroup /not/really/sys/fs/cgroup cgroup > rw,relatime,blkio,devices,memory,cpuacct,cpu,cpuset > 0 0 > diff --git a/tests/vircgroupdata/all-in-one.parsed > b/tests/vircgroupdata/all-in-one.parsed > index 2701778fea..d703d08fb9 100644 > --- a/tests/vircgroupdata/all-in-one.parsed > +++ b/tests/vircgroupdata/all-in-one.parsed > @@ -1,10 +1,10 @@ > -cpu /sys/fs/cgroup > -cpuacct /sys/fs/cgroup > -cpuset /sys/fs/cgroup > -memory /sys/fs/cgroup > -devices /sys/fs/cgroup > +cpu /not/really/sys/fs/cgroup > +cpuacct /not/really/sys/fs/cgroup > +cpuset /not/really/sys/fs/cgroup > +memory /not/really/sys/fs/cgroup > +devices /not/really/sys/fs/cgroup > freezer > -blkio/sys/fs/cgroup > +blkio/not/really/sys/fs/cgroup > net_cls > perf_event > name=systemd > diff --git a/tests/vircgroupdata/all-in-one.self.cgroup > b/tests/vircgroupdata/all-in-one.self.cgroup > new file mode 100644 > index 00..cf237502e9 > --- /dev/null > +++ b/tests/vircgroupdata/all-in-one.self.cgroup > @@ -0,0 +1 @@ > +6:blkio,devices,memory,cpuacct,cpu,cpuset:/ > diff --git a/tests/vircgroupdata/logind.cgroups > b/tests/vircgroupdata/logind.cgroups > new file mode 100644 > index 00..9d46f130e0 > --- /dev/null > +++ b/tests/vircgroupdata/logind.cgroups > @@ -0,0 +1,10 @@ > +#subsys_namehierarchy num_cgroups enabled > +cpuset0 1 1 > +cpu 0 1 1 > +cpuacct 0 1 1 > +memory0 1 0 > +devices 0 1 1 > +freezer 0 1 1 > +net_cls 0 1 1 > +blkio 0 1 1 > +perf_event 0 1 1 > diff --git a/tests/vircgroupdata/logind.mounts > b/tests/vircgroupdata/logind.mounts > new file mode 100644 > index 00..3ab908aee9 > --- /dev/null > +++ b/tests/vircgroupdata/logind.mounts > @@ -0,0 +1,2 @@ > +none /not/really/sys/fs/cgroup tmpfs rw,rootcontext=system_u:object > _r:sysfs_t:s0,seclabel,relatime,size=4k,mode=755 0 0 > +systemd /not/really/sys/fs/cgroup/systemd cgroup > rw,nosuid,nodev,noexec,relatime,name=systemd 0 0 > diff --git a/tests/vircgroupdata/logind.self.cgroup > b/tests/vircgroupdata/logind.self.cgroup > new file mode 100644 > index 00..31e0cfe8eb > --- /dev/null > +++ b/tests/vircgroupdata/logind.self.cgroup > @@ -0,0 +1 @@ > +0:name=systemd:/ > diff --git a/tests/vircgroupdata/systemd.cgroups > b/tests/vircgroupdata/systemd.cgroups > new file mode 100644 > index 00..d32dfab222 > ---
Re: [libvirt] [PATCH 2/9] vircgroup: fix bug in virCgroupEnableMissingControllers
On Tue, Sep 18, 2018 at 2:17 PM, Pavel Hrdina wrote: > If we are on host with systemd we need to build cgroup hierarchy > ourselves for controllers that are not managed by systemd. > > As a starting parent we need need to force root group because > typo: need need -> need > virCgroupMakeGroup() takes that parent in order to inherit values > for cpuset controller. > > By default cpuset controller is managed by systemd so we will never > hit the issue but for v2 cgroups we need to use parent cgroup every > time. > > Signed-off-by: Pavel Hrdina > Reviewed-by: Fabiano Fidêncio > --- > src/util/vircgroup.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c > index 6aa30a82be..2328957818 100644 > --- a/src/util/vircgroup.c > +++ b/src/util/vircgroup.c > @@ -1495,7 +1495,7 @@ virCgroupEnableMissingControllers(char *path, > int ret = -1; > > if (virCgroupNew(pidleader, > - "", > + "/", > NULL, > controllers, > ) < 0) > -- > 2.17.1 > > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/9] vircgroup: introduce virCgroupTaskFlags
On Tue, Sep 18, 2018 at 2:17 PM, Pavel Hrdina wrote:
> Use flags in virCgroupAddTaskInternal instead of boolean parameter.
> Following patch will ad new flag to indicate thread instead of process.
>
typo: ad -> add
>
> Signed-off-by: Pavel Hrdina
>
---
> src/util/vircgroup.c | 19 +++
> 1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
> index cf510fb019..1d361762c5 100644
> --- a/src/util/vircgroup.c
> +++ b/src/util/vircgroup.c
> @@ -1156,8 +1156,16 @@ virCgroupNew(pid_t pid,
> }
>
>
> +typedef enum {
> +VIR_CGROUP_TASK_PROCESS = 0,
> +VIR_CGROUP_TASK_SYSTEMD = 1 << 0,
>
Although not strictly needed, would be nice to have a comment describing
what the flags mean.
> +} virCgroupTaskFlags;
> +
> +
> static int
> -virCgroupAddTaskInternal(virCgroupPtr group, pid_t pid, bool withSystemd)
> +virCgroupAddTaskInternal(virCgroupPtr group,
> + pid_t pid,
> + unsigned int flags)
> {
> int ret = -1;
> size_t i;
> @@ -1170,7 +1178,8 @@ virCgroupAddTaskInternal(virCgroupPtr group, pid_t
> pid, bool withSystemd)
> /* We must never add tasks in systemd's hierarchy
> * unless we're intentionally trying to move a
> * task into a systemd machine scope */
> -if (i == VIR_CGROUP_CONTROLLER_SYSTEMD && !withSystemd)
> +if (i == VIR_CGROUP_CONTROLLER_SYSTEMD &&
> +!(flags & VIR_CGROUP_TASK_SYSTEMD))
> continue;
>
> if (virCgroupSetValueI64(group, i, "tasks", pid) < 0)
> @@ -1196,7 +1205,7 @@ virCgroupAddTaskInternal(virCgroupPtr group, pid_t
> pid, bool withSystemd)
> int
> virCgroupAddProcess(virCgroupPtr group, pid_t pid)
> {
> -return virCgroupAddTaskInternal(group, pid, false);
> +return virCgroupAddTaskInternal(group, pid, VIR_CGROUP_TASK_PROCESS);
> }
>
> /**
> @@ -1213,7 +1222,9 @@ virCgroupAddProcess(virCgroupPtr group, pid_t pid)
> int
> virCgroupAddMachineProcess(virCgroupPtr group, pid_t pid)
> {
> -return virCgroupAddTaskInternal(group, pid, true);
> +return virCgroupAddTaskInternal(group, pid,
> +VIR_CGROUP_TASK_PROCESS |
> +VIR_CGROUP_TASK_SYSTEMD);
> }
>
>
> --
> 2.17.1
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
Reviewed-by: Fabiano Fidêncio
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 9/9] vircgrouptest: call virCgroupNewSelf instead virCgroupDetectMounts
On Tue, Sep 18, 2018 at 2:18 PM, Pavel Hrdina wrote:
> This will be required once cgroup v2 is introduced. The cgroup
> detection is not simple and we will have multiple backends so we
> should not just jump into the middle of the detection code.
>
> In order to use virCgroupNewSelf we need to create all the remaining
> data files.
>
> Signed-off-by: Pavel Hrdina
> ---
> src/libvirt_private.syms | 1 -
> src/util/vircgroup.c | 11 +--
> src/util/vircgrouppriv.h | 2 --
> tests/vircgroupdata/cgroups1.cgroups | 11 +++
> tests/vircgroupdata/cgroups1.self.cgroup | 11 +++
> tests/vircgroupdata/cgroups2.cgroups | 10 ++
> tests/vircgroupdata/cgroups2.self.cgroup | 10 ++
> tests/vircgroupdata/cgroups3.cgroups | 12
> tests/vircgroupdata/cgroups3.self.cgroup | 12
> tests/vircgroupdata/fedora-18.cgroups | 10 ++
> tests/vircgroupdata/fedora-18.self.cgroup | 9 +
> tests/vircgroupdata/fedora-21.cgroups | 12
> tests/vircgroupdata/fedora-21.self.cgroup | 10 ++
> tests/vircgroupdata/kubevirt.cgroups | 10 ++
> tests/vircgroupdata/kubevirt.self.cgroup | 10 ++
> tests/vircgroupdata/ovirt-node-6.6.cgroups | 9 +
> tests/vircgroupdata/ovirt-node-6.6.self.cgroup | 8
> tests/vircgroupdata/ovirt-node-7.1.cgroups | 11 +++
> tests/vircgroupdata/ovirt-node-7.1.self.cgroup | 10 ++
> tests/vircgroupdata/rhel-7.1.cgroups | 11 +++
> tests/vircgroupdata/rhel-7.1.self.cgroup | 10 ++
> tests/vircgrouptest.c | 6 +-
> 22 files changed, 188 insertions(+), 18 deletions(-)
> create mode 100644 tests/vircgroupdata/cgroups1.cgroups
> create mode 100644 tests/vircgroupdata/cgroups1.self.cgroup
> create mode 100644 tests/vircgroupdata/cgroups2.cgroups
> create mode 100644 tests/vircgroupdata/cgroups2.self.cgroup
> create mode 100644 tests/vircgroupdata/cgroups3.cgroups
> create mode 100644 tests/vircgroupdata/cgroups3.self.cgroup
> create mode 100644 tests/vircgroupdata/fedora-18.cgroups
> create mode 100644 tests/vircgroupdata/fedora-18.self.cgroup
> create mode 100644 tests/vircgroupdata/fedora-21.cgroups
> create mode 100644 tests/vircgroupdata/fedora-21.self.cgroup
> create mode 100644 tests/vircgroupdata/kubevirt.cgroups
> create mode 100644 tests/vircgroupdata/kubevirt.self.cgroup
> create mode 100644 tests/vircgroupdata/ovirt-node-6.6.cgroups
> create mode 100644 tests/vircgroupdata/ovirt-node-6.6.self.cgroup
> create mode 100644 tests/vircgroupdata/ovirt-node-7.1.cgroups
> create mode 100644 tests/vircgroupdata/ovirt-node-7.1.self.cgroup
> create mode 100644 tests/vircgroupdata/rhel-7.1.cgroups
> create mode 100644 tests/vircgroupdata/rhel-7.1.self.cgroup
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 7f3b5738c4..75c59fbf89 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1513,7 +1513,6 @@ virCgroupDelThread;
> virCgroupDenyAllDevices;
> virCgroupDenyDevice;
> virCgroupDenyDevicePath;
> -virCgroupDetectMounts;
> virCgroupFree;
> virCgroupGetBlkioDeviceReadBps;
> virCgroupGetBlkioDeviceReadIops;
> diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
> index f8ef76136b..e85e0bde24 100644
> --- a/src/util/vircgroup.c
> +++ b/src/util/vircgroup.c
> @@ -429,7 +429,7 @@ virCgroupMountOptsMatchController(const char *mntOpts,
> * Process /proc/mounts figuring out what controllers are
> * mounted and where
> */
> -int
> +static int
> virCgroupDetectMounts(virCgroupPtr group)
> {
> size_t i;
> @@ -4072,15 +4072,6 @@ virCgroupAvailable(void)
> }
>
>
> -int
> -virCgroupDetectMounts(virCgroupPtr group ATTRIBUTE_UNUSED)
> -{
> -virReportSystemError(ENXIO, "%s",
> - _("Control groups not supported on this
> platform"));
> -return -1;
> -}
> -
> -
> int
> virCgroupNewPartition(const char *path ATTRIBUTE_UNUSED,
>bool create ATTRIBUTE_UNUSED,
> diff --git a/src/util/vircgrouppriv.h b/src/util/vircgrouppriv.h
> index f78fe8bb9c..046c96c52c 100644
> --- a/src/util/vircgrouppriv.h
> +++ b/src/util/vircgrouppriv.h
> @@ -50,8 +50,6 @@ struct _virCgroup {
> virCgroupController controllers[VIR_CGROUP_CONTROLLER_LAST];
> };
>
> -int virCgroupDetectMounts(virCgroupPtr group);
> -
> int virCgroupNewPartition(const char *path,
>bool create,
>int controllers,
> diff --git a/tests/vircgroupdata/cgroups1.cgroups
> b/tests/vircgroupdata/cgroups1.cgroups
> new file mode 100644
> index 00..a03c290a98
> --- /dev/null
> +++ b/tests/vircgroupdata/cgroups1.cgroups
> @@ -0,0 +1,11 @@
> +#subsys_namehierarchy num_cgroups enabled
>
Re: [libvirt] [PATCH 5/9] vircgroup: introduce virCgroupAddThread
On Tue, Sep 18, 2018 at 2:17 PM, Pavel Hrdina wrote:
> Once we introduce cgroup v2 support we need to handle processes and
> threads differently.
>
> Signed-off-by: Pavel Hrdina
>
Reviewed-by: Fabiano Fidêncio
> ---
> src/libvirt_private.syms | 1 +
> src/qemu/qemu_process.c | 2 +-
> src/util/vircgroup.c | 29 +
> src/util/vircgroup.h | 1 +
> 4 files changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index eac66b0174..ad7ce57b65 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1500,6 +1500,7 @@ virBufferVasprintf;
> # util/vircgroup.h
> virCgroupAddMachineProcess;
> virCgroupAddProcess;
> +virCgroupAddThread;
> virCgroupAllowAllDevices;
> virCgroupAllowDevice;
> virCgroupAllowDevicePath;
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 249dac39f2..00dcd5b580 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -2549,7 +2549,7 @@ qemuProcessSetupPid(virDomainObjPtr vm,
> goto cleanup;
>
> /* Move the thread to the sub dir */
> -if (virCgroupAddProcess(cgroup, pid) < 0)
> +if (virCgroupAddThread(cgroup, pid) < 0)
>
goto cleanup;
>
> }
> diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
> index 1d361762c5..3211f63cb1 100644
> --- a/src/util/vircgroup.c
> +++ b/src/util/vircgroup.c
> @@ -1159,6 +1159,7 @@ virCgroupNew(pid_t pid,
> typedef enum {
> VIR_CGROUP_TASK_PROCESS = 0,
> VIR_CGROUP_TASK_SYSTEMD = 1 << 0,
> +VIR_CGROUP_TASK_THREAD = 1 << 1,
> } virCgroupTaskFlags;
>
>
> @@ -1227,6 +1228,24 @@ virCgroupAddMachineProcess(virCgroupPtr group,
> pid_t pid)
> VIR_CGROUP_TASK_SYSTEMD);
> }
>
> +/**
> + * virCgroupAddThread:
> + *
> + * @group: The cgroup to add a thread to
> + * @pid: The pid of the thread to add
> + *
> + * Will add the thread to all controllers, except the
> + * systemd unit controller.
> + *
> + * Returns: 0 on success, -1 on error
> + */
> +int
> +virCgroupAddThread(virCgroupPtr group,
> + pid_t pid)
> +{
> +return virCgroupAddTaskInternal(group, pid, VIR_CGROUP_TASK_THREAD);
> +}
> +
>
> static int
> virCgroupSetPartitionSuffix(const char *path, char **res)
> @@ -4228,6 +4247,16 @@ virCgroupAddMachineProcess(virCgroupPtr group
> ATTRIBUTE_UNUSED,
> }
>
>
> +int
> +virCgroupAddThread(virCgroupPtr group ATTRIBUTE_UNUSED,
> + pid_t pid ATTRIBUTE_UNUSED)
> +{
> +virReportSystemError(ENXIO, "%s",
> + _("Control groups not supported on this
> platform"));
> +return -1;
> +}
> +
> +
> int
> virCgroupGetBlkioIoServiced(virCgroupPtr group ATTRIBUTE_UNUSED,
> long long *bytes_read ATTRIBUTE_UNUSED,
> diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h
> index bbd4c2ed57..1f676f21c3 100644
> --- a/src/util/vircgroup.h
> +++ b/src/util/vircgroup.h
> @@ -120,6 +120,7 @@ int virCgroupPathOfController(virCgroupPtr group,
>
> int virCgroupAddProcess(virCgroupPtr group, pid_t pid);
> int virCgroupAddMachineProcess(virCgroupPtr group, pid_t pid);
> +int virCgroupAddThread(virCgroupPtr group, pid_t pid);
>
> int virCgroupSetBlkioWeight(virCgroupPtr group, unsigned int weight);
> int virCgroupGetBlkioWeight(virCgroupPtr group, unsigned int *weight);
> --
> 2.17.1
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/9] vircgroup: rename virCgroupAdd.*Task to virCgroupAdd.*Process
On Tue, Sep 18, 2018 at 2:17 PM, Pavel Hrdina wrote:
> In cgroup v2 we need to handle processes and threads differently,
> following patch will introduce virCgroupAddThread.
>
> Signed-off-by: Pavel Hrdina
>
Reviewed-by: Fabiano Fidêncio
> ---
> src/libvirt-lxc.c| 2 +-
> src/libvirt_private.syms | 4 ++--
> src/lxc/lxc_controller.c | 4 ++--
> src/qemu/qemu_process.c | 4 ++--
> src/qemu/qemu_tpm.c | 2 +-
> src/util/vircgroup.c | 32
> src/util/vircgroup.h | 4 ++--
> 7 files changed, 26 insertions(+), 26 deletions(-)
>
> diff --git a/src/libvirt-lxc.c b/src/libvirt-lxc.c
> index c9f2146487..9bf0174b95 100644
> --- a/src/libvirt-lxc.c
> +++ b/src/libvirt-lxc.c
> @@ -306,7 +306,7 @@ int virDomainLxcEnterCGroup(virDomainPtr domain,
> if (virCgroupNewDetect(domain->id, -1, ) < 0)
> goto error;
>
> -if (virCgroupAddTask(cgroup, getpid()) < 0)
> +if (virCgroupAddProcess(cgroup, getpid()) < 0)
> goto error;
>
> virCgroupFree();
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index b9dabfef1b..eac66b0174 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1498,8 +1498,8 @@ virBufferVasprintf;
>
>
> # util/vircgroup.h
> -virCgroupAddMachineTask;
> -virCgroupAddTask;
> +virCgroupAddMachineProcess;
> +virCgroupAddProcess;
> virCgroupAllowAllDevices;
> virCgroupAllowDevice;
> virCgroupAllowDevicePath;
> diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
> index 4e84391bf5..4ead2dc9f0 100644
> --- a/src/lxc/lxc_controller.c
> +++ b/src/lxc/lxc_controller.c
> @@ -875,12 +875,12 @@ static int
> virLXCControllerSetupCgroupLimits(virLXCControllerPtr
> ctrl)
> ctrl->nicindexes)))
> goto cleanup;
>
> -if (virCgroupAddMachineTask(ctrl->cgroup, getpid()) < 0)
> +if (virCgroupAddMachineProcess(ctrl->cgroup, getpid()) < 0)
> goto cleanup;
>
> /* Add all qemu-nbd tasks to the cgroup */
> for (i = 0; i < ctrl->nnbdpids; i++) {
> -if (virCgroupAddMachineTask(ctrl->cgroup, ctrl->nbdpids[i]) < 0)
> +if (virCgroupAddMachineProcess(ctrl->cgroup, ctrl->nbdpids[i]) <
> 0)
> goto cleanup;
> }
>
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 72a59dec55..249dac39f2 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -2549,7 +2549,7 @@ qemuProcessSetupPid(virDomainObjPtr vm,
> goto cleanup;
>
> /* Move the thread to the sub dir */
> -if (virCgroupAddTask(cgroup, pid) < 0)
> +if (virCgroupAddProcess(cgroup, pid) < 0)
> goto cleanup;
>
> }
> @@ -2787,7 +2787,7 @@ qemuProcessStartManagedPRDaemon(virDomainObjPtr vm)
> }
>
> if (priv->cgroup &&
> -virCgroupAddMachineTask(priv->cgroup, cpid) < 0)
> +virCgroupAddMachineProcess(priv->cgroup, cpid) < 0)
> goto cleanup;
>
> if (qemuSecurityDomainSetPathLabel(driver, vm, socketPath, true) < 0)
> diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
> index 278b262c48..c64114feac 100644
> --- a/src/qemu/qemu_tpm.c
> +++ b/src/qemu/qemu_tpm.c
> @@ -905,7 +905,7 @@ qemuExtTPMSetupCgroup(virQEMUDriverPtr driver,
> _("Could not get process id of swtpm"));
> goto cleanup;
> }
> -if (virCgroupAddTask(cgroup, pid) < 0)
> +if (virCgroupAddProcess(cgroup, pid) < 0)
> goto cleanup;
> break;
> case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
> diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
> index 2328957818..cf510fb019 100644
> --- a/src/util/vircgroup.c
> +++ b/src/util/vircgroup.c
> @@ -1183,35 +1183,35 @@ virCgroupAddTaskInternal(virCgroupPtr group,
> pid_t pid, bool withSystemd)
> }
>
> /**
> - * virCgroupAddTask:
> + * virCgroupAddProcess:
> *
> - * @group: The cgroup to add a task to
> - * @pid: The pid of the task to add
> + * @group: The cgroup to add a process to
> + * @pid: The pid of the process to add
> *
> - * Will add the task to all controllers, except the
> + * Will add the process to all controllers, except the
> * systemd unit controller.
> *
> * Returns: 0 on success, -1 on error
> */
> int
> -virCgroupAddTask(virCgroupPtr group, pid_t pid)
> +virCgroupAddProcess(virCgroupPtr group, pid_t pid)
> {
> return virCgroupAddTaskInternal(group, pid, false);
> }
>
> /**
> - * virCgroupAddMachineTask:
> + * virCgroupAddMachineProcess:
> *
> - * @group: The cgroup to add a task to
> - * @pid: The pid of the task to add
> + * @group: The cgroup to add a process to
> + * @pid: The pid of the process to add
> *
> - * Will add the task to all controllers, including the
> + * Will add the process to all controllers, including the
> * systemd unit controller.
> *
> * Returns: 0 on success, -1 on error
> */
> int
> -virCgroupAddMachineTask(virCgroupPtr
Re: [libvirt] [PATCH 6/9] vircgroupmock: cleanup unused cgroup files
On Tue, Sep 18, 2018 at 2:17 PM, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina
>
Reviewed-by: Fabiano Fidêncio
> ---
> tests/vircgroupmock.c | 73 ---
> 1 file changed, 73 deletions(-)
>
> diff --git a/tests/vircgroupmock.c b/tests/vircgroupmock.c
> index 51861be38e..e0024b2c63 100644
> --- a/tests/vircgroupmock.c
> +++ b/tests/vircgroupmock.c
> @@ -214,13 +214,7 @@ static int make_controller(const char *path, mode_t
> mode)
> if (STRPREFIX(controller, "cpu,cpuacct")) {
> MAKE_FILE("cpu.cfs_period_us", "10\n");
> MAKE_FILE("cpu.cfs_quota_us", "-1\n");
> -MAKE_FILE("cpu.rt_period_us", "100\n");
> -MAKE_FILE("cpu.rt_runtime_us", "95\n");
> MAKE_FILE("cpu.shares", "1024\n");
> -MAKE_FILE("cpu.stat",
> - "nr_periods 0\n"
> - "nr_throttled 0\n"
> - "throttled_time 0\n");
> MAKE_FILE("cpuacct.stat",
>"user 216687025\n"
>"system 43421396\n");
> @@ -235,46 +229,19 @@ static int make_controller(const char *path, mode_t
> mode)
>"709566900 0 0 0 0 0 0 0 444777342 0 0 0 0 0 0 0 "
>"5683512916 0 0 0 0 0 0 0 635751356 0 0 0 0 0 0 0\n");
> } else if (STRPREFIX(controller, "cpuset")) {
> -MAKE_FILE("cpuset.cpu_exclusive", "1\n");
> if (STREQ(controller, "cpuset"))
> MAKE_FILE("cpuset.cpus", "0-1");
> else
> MAKE_FILE("cpuset.cpus", ""); /* Values don't inherit */
> -MAKE_FILE("cpuset.mem_exclusive", "1\n");
> -MAKE_FILE("cpuset.mem_hardwall", "0\n");
> MAKE_FILE("cpuset.memory_migrate", "0\n");
> -MAKE_FILE("cpuset.memory_pressure", "0\n");
> -MAKE_FILE("cpuset.memory_pressure_enabled", "0\n");
> -MAKE_FILE("cpuset.memory_spread_page", "0\n");
> -MAKE_FILE("cpuset.memory_spread_slab", "0\n");
> if (STREQ(controller, "cpuset"))
> MAKE_FILE("cpuset.mems", "0");
> else
> MAKE_FILE("cpuset.mems", ""); /* Values don't inherit */
> -MAKE_FILE("cpuset.sched_load_balance", "1\n");
> -MAKE_FILE("cpuset.sched_relax_domain_level", "-1\n");
> } else if (STRPREFIX(controller, "memory")) {
> -MAKE_FILE("memory.failcnt", "0\n");
> -MAKE_FILE("memory.force_empty", ""); /* Write only */
> -MAKE_FILE("memory.kmem.tcp.failcnt", "0\n");
> -MAKE_FILE("memory.kmem.tcp.limit_in_bytes",
> "9223372036854775807\n");
> -MAKE_FILE("memory.kmem.tcp.max_usage_in_bytes", "0\n");
> -MAKE_FILE("memory.kmem.tcp.usage_in_bytes", "16384\n");
> MAKE_FILE("memory.limit_in_bytes", "9223372036854775807\n");
> -MAKE_FILE("memory.max_usage_in_bytes", "0\n");
> -MAKE_FILE("memory.memsw.failcnt", ""); /* Not supported */
> MAKE_FILE("memory.memsw.limit_in_bytes", ""); /* Not supported */
> -MAKE_FILE("memory.memsw.max_usage_in_bytes", ""); /* Not
> supported */
> MAKE_FILE("memory.memsw.usage_in_bytes", ""); /* Not supported */
> -MAKE_FILE("memory.move_charge_at_immigrate", "0\n");
> -MAKE_FILE("memory.numa_stat",
> - "total=367664 N0=367664\n"
> - "file=314764 N0=314764\n"
> - "anon=51999 N0=51999\n"
> - "unevictable=901 N0=901\n");
> -MAKE_FILE("memory.oom_control",
> - "oom_kill_disable 0\n"
> - "under_oom 0\n");
> MAKE_FILE("memory.soft_limit_in_bytes", "9223372036854775807\n");
> MAKE_FILE("memory.stat",
>"cache 1336619008\n"
> @@ -306,50 +273,11 @@ static int make_controller(const char *path, mode_t
> mode)
>"recent_rotated_file 2547948\n"
>"recent_scanned_anon 113796164\n"
>"recent_scanned_file 8199863\n");
> -MAKE_FILE("memory.swappiness", "60\n");
> MAKE_FILE("memory.usage_in_bytes", "1455321088\n");
> MAKE_FILE("memory.use_hierarchy", "0\n");
> } else if (STRPREFIX(controller, "freezer")) {
> MAKE_FILE("freezer.state", "THAWED");
> } else if (STRPREFIX(controller, "blkio")) {
> -MAKE_FILE("blkio.io_merged",
> - "8:0 Read 1100949\n"
> - "8:0 Write 2248076\n"
> - "8:0 Sync 63063\n"
> - "8:0 Async 3285962\n"
> - "8:0 Total 3349025\n");
> -MAKE_FILE("blkio.io_queued",
> - "8:0 Read 0\n"
> - "8:0 Write 0\n"
> - "8:0 Sync 0\n"
> - "8:0 Async 0\n"
> - "8:0 Total 0\n");
> -MAKE_FILE("blkio.io_service_bytes",
> - "8:0 Read 59542078464\n"
> - "8:0 Write 397369182208\n"
> - "8:0 Sync 234080922624\n"
> - "8:0 Async
Re: [libvirt] [PATCH v4 00/23] Introduce metadata locking
Michal Privoznik [2018-09-19, 11:45AM +0200]: > On 09/19/2018 11:17 AM, Bjoern Walk wrote: > > Bjoern Walk [2018-09-12, 01:17PM +0200]: > >> Michal Privoznik [2018-09-12, 11:32AM +0200]: > > >> > > > > Still seeing the same timeout. Is this expected behaviour? > > > > Nope. I wonder if something has locked the path and forgot to unlock it > (however, virtlockd should have unlocked all the paths owned by PID on > connection close), or something is still holding the lock and connection > opened. I can reproduce on a freshly booted machine. There should be no rouge lock held anywhere. > > Can you see the timeout even when you turn off the selinux driver > (security_driver="none' in qemu.conf)? I tried to reproduce the issue Yes, same issue. > yesterday and was unsuccessful. Do you have any steps to reproduce? 0. Host setup: # /usr/sbin/sestatus SELinux status: enabled SELinuxfs mount:/sys/fs/selinux SELinux root directory: /etc/selinux Loaded policy name: targeted Current mode: enforcing Mode from config file: enforcing Policy MLS status: enabled Policy deny_unknown status: allowed Memory protection checking: actual (secure) Max kernel policy version: 31 # grep -E "^[^#]" /etc/libvirt/qemu.conf lock_manager = "lockd" metadata_lock_manager = "lockd" 1. Define two domains, u1604-1 and u1604-2, using the same image, not shared: u1604-1 e49679de-eca9-4a72-8d1a-56f437541157 524288 524288 2 hvm destroy restart preserve /usr/bin/qemu-system-s390x 2. Start domain u1604-1: # ls -Z /var/lib/libvirt/images/u1604.qcow2 system_u:object_r:svirt_image_t:s0:c387,c937 /var/lib/libvirt/images/u1604.qcow2 3. Start domain u1604-2. 4. Result: the libvirtd hangs for 60 seconds after which the expected locking message appears. Security labels of the image are not changed: # virsh start u1604-2 error: Failed to start domain u1604-2 error: internal error: child reported: resource busy: Lockspace resource '/var/lib/libvirt/images/u1604.qcow2' is locked # ls -Z /var/lib/libvirt/images/u1604.qcow2 system_u:object_r:svirt_image_t:s0:c387,c937 /var/lib/libvirt/images/u1604.qcow2 Backtrace is the same I sent earlier. Let me know if you need more info or if anything is borked in my setup. > > Michal > signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/9] vircgroup: cleanup controllers not managed by systemd on error
On Tue, Sep 18, 2018 at 2:17 PM, Pavel Hrdina wrote:
> If virCgroupEnableMissingControllers() fails it could already create
> some directories, we should clean it up as well.
>
> Signed-off-by: Pavel Hrdina
>
Reviewed-by: Fabiano Fidêncio
> ---
> src/util/vircgroup.c | 25 +++--
> 1 file changed, 15 insertions(+), 10 deletions(-)
>
> diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
> index 64507bf8aa..6aa30a82be 100644
> --- a/src/util/vircgroup.c
> +++ b/src/util/vircgroup.c
> @@ -1555,6 +1555,7 @@ virCgroupNewMachineSystemd(const char *name,
> int rv;
> virCgroupPtr init;
> VIR_AUTOFREE(char *) path = NULL;
> +virErrorPtr saved = NULL;
>
> VIR_DEBUG("Trying to setup machine '%s' via systemd", name);
> if ((rv = virSystemdCreateMachine(name,
> @@ -1588,20 +1589,24 @@ virCgroupNewMachineSystemd(const char *name,
>
> if (virCgroupEnableMissingControllers(path, pidleader,
>controllers, group) < 0) {
> -return -1;
> +goto error;
> }
>
> -if (virCgroupAddTask(*group, pidleader) < 0) {
> -virErrorPtr saved = virSaveLastError();
> -virCgroupRemove(*group);
> -virCgroupFree(group);
> -if (saved) {
> -virSetError(saved);
> -virFreeError(saved);
> -}
> -}
> +if (virCgroupAddTask(*group, pidleader) < 0)
> +goto error;
>
> return 0;
> +
> + error:
> +saved = virSaveLastError();
> +virCgroupRemove(*group);
> +virCgroupFree(group);
> +if (saved) {
> +virSetError(saved);
> +virFreeError(saved);
> +}
> +
> +return -1;
> }
>
>
> --
> 2.17.1
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 3/3] qemu: add memfd source type
On 09/19/2018 12:02 PM, Pavel Hrdina wrote: > On Wed, Sep 19, 2018 at 11:41:11AM +0200, Michal Privoznik wrote: >> On 09/17/2018 03:14 PM, marcandre.lur...@redhat.com wrote: >>> From: Marc-André Lureau >>> >>> Add a new memoryBacking source type "memfd", supported by QEMU (when >>> the apability is available). >>> >>> A memfd is a specialized anonymous memory kind. As such, an anonymous >>> source type could be automatically using a memfd. However, there are >>> some complications when migrating from different memory backends in >>> qemu (mainly due to the internal object naming at this point, but >>> there could be more). For now, it is simpler and safer to simply >>> introduce a new source type "memfd". Eventually, the "anonymous" type >>> could learn to use memfd transparently in a seperate change. >>> >>> The main benefits are that it doesn't need to create filesystem files, >>> and it also enforces sealing, providing a bit more safety. >>> >>> Signed-off-by: Marc-André Lureau >>> --- >>> docs/formatdomain.html.in | 9 +-- >>> docs/schemas/domaincommon.rng | 1 + >>> src/conf/domain_conf.c| 3 +- >>> src/conf/domain_conf.h| 1 + >>> src/qemu/qemu_command.c | 69 +-- >>> src/qemu/qemu_domain.c| 12 +++- >>> .../memfd-memory-numa.x86_64-latest.args | 34 + >>> tests/qemuxml2argvdata/memfd-memory-numa.xml | 36 ++ >>> tests/qemuxml2argvtest.c | 2 + >>> 9 files changed, 140 insertions(+), 27 deletions(-) >>> create mode 100644 >>> tests/qemuxml2argvdata/memfd-memory-numa.x86_64-latest.args >>> create mode 100644 tests/qemuxml2argvdata/memfd-memory-numa.xml >>> >>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in >>> index 1f12ab5b42..1f6d40 100644 >>> --- a/docs/formatdomain.html.in >>> +++ b/docs/formatdomain.html.in >>> @@ -1099,7 +1099,7 @@ >>> /hugepages >>> nosharepages/ >>> locked/ >>> -source type="file|anonymous"/ >>> +source type="file|anonymous|memfd"/ >> >> I'm sorry but I do not think this is the way we should go. This >> effectively avoids libvirt making the decision and exposes the backend >> used directly. This puts unnecessary burden on mgmt applications because >> they have to make yet another decision (track another domain attribute). >> >> IIUC, memfd is like memory-backend-file and -ram combined. It can do >> hugepages or just plain malloc(). Therefore it should be our first >> choice for freshly started domains. And only if qemu doesn't support it >> we should fall back to either -file or -ram backends. >> >> This means we have to track what backend the domain was started with so >> that we preserve that on migration (although, the fact that these >> backends are not interchangeable makes me question 'backend' in their >> name :-P). For that we can use status/migration XML as I suggested earlier. >> >> Once again, status XML is not editable by user [*] and is used solely by >> libvirtd to store runtime information for a running domain (and backend >> used falls into that category). > > I have to agree with Michal, we should not expose this implementation > detail in domain XML if we can hide it in status/migratable XML. > > One thing about the migration though. I'm not sure what are we > officially supporting in libvirt because it might cause us some issues. > > We need to make sure that if you live-migrate domain from old libvirt > to new libvirt you should be able to migrate that domain back to old > libvirt. The question is, whether this applies if you destroy and > start the domain on the new libvirt before you live-migrate it back > to old libvirt. > > Without the restart there is no issue, because the backend would not > be changed, but once you start the same domain again we would pick > new backend which would prevent migrating it back to the old libvirt. > This is not supported. Exactly because of this reason. If we were to preserve this forward compatibility (i.e. migration from newer libvirt to older) then we couldn't use any new feature qemu has. For instance, if qemu introduces a new device and a user starts a domain with it, migration to older qemu will not work then, obviously. This also applies for other qemu capabilities. Migrating from newer version to older is not supported. It might work, but that's rather coincidence than intent. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/5] qemu: Change the way we generate VIR_DOMAIN_EVENT_RESUMED
On Wed, Sep 12, 2018 at 14:55:53 +0200, Jiri Denemark wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1612943 > > Jiri Denemark (5): > qemu: Properly report VIR_DOMAIN_EVENT_RESUMED_FROM_SNAPSHOT > qemu: Report more appropriate running reasons > qemu: Pass running reason to RESUME event handler > qemu: Map running reason to resume event detail > qemu: Avoid duplicate resume events and state changes > > src/qemu/qemu_domain.c| 29 > src/qemu/qemu_domain.h| 7 ++ > src/qemu/qemu_driver.c| 13 --- > src/qemu/qemu_migration.c | 36 +- > src/qemu/qemu_process.c | 46 --- > 5 files changed, 71 insertions(+), 60 deletions(-) ping -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/2] internal: Move include to internal.h
It doesn't really make sense for us to have stdlib.h and string.h but not stdio.h in the internal.h header. Signed-off-by: Erik Skultety --- src/bhyve/bhyve_capabilities.c| 1 - src/conf/capabilities.c | 1 - src/conf/storage_conf.c | 1 - src/esx/esx_storage_backend_iscsi.c | 1 - src/esx/esx_storage_backend_vmfs.c| 1 - src/internal.h| 1 + src/libvirt.c | 1 - src/locking/lock_driver_sanlock.c | 1 - src/locking/sanlock_helper.c | 1 - src/lxc/lxc_container.c | 1 - src/lxc/lxc_fuse.c| 1 - src/lxc/lxc_native.c | 1 - src/network/bridge_driver.c | 1 - src/network/leaseshelper.c| 1 - src/node_device/node_device_hal.c | 1 - src/openvz/openvz_conf.c | 1 - src/openvz/openvz_driver.c| 1 - src/phyp/phyp_driver.c| 1 - src/qemu/qemu_driver.c| 1 - src/rpc/virnetservermdns.c| 1 - src/security/virt-aa-helper.c | 1 - src/storage/parthelper.c | 1 - src/storage/storage_backend_disk.c| 1 - src/storage/storage_backend_fs.c | 1 - src/storage/storage_backend_logical.c | 1 - src/storage/storage_backend_mpath.c | 1 - src/storage/storage_backend_scsi.c| 1 - src/storage/storage_driver.c | 1 - src/storage/storage_util.c| 1 - src/test/test_driver.c| 1 - src/uml/uml_driver.c | 1 - src/util/iohelper.c | 1 - src/util/virarptable.c| 1 - src/util/viraudit.c | 1 - src/util/virbitmap.c | 1 - src/util/virbuffer.c | 1 - src/util/vircgroup.c | 1 - src/util/virconf.c| 1 - src/util/virdnsmasq.c | 1 - src/util/virerror.c | 1 - src/util/virfile.h| 1 - src/util/virgettext.c | 1 - src/util/virhook.c| 1 - src/util/virhostcpu.c | 1 - src/util/virhostdev.c | 1 - src/util/virhostmem.c | 1 - src/util/viriptables.c| 1 - src/util/viriscsi.c | 1 - src/util/virkeyfile.c | 1 - src/util/virlog.c | 1 - src/util/virmacaddr.c | 1 - src/util/virnetdevmacvlan.c | 1 - src/util/virnetdevopenvswitch.c | 1 - src/util/virnetdevtap.c | 1 - src/util/virnetdevvportprofile.c | 1 - src/util/virpci.c | 1 - src/util/virscsi.c| 1 - src/util/virsexpr.c | 1 - src/util/virstring.c | 1 - src/util/virsysinfo.c | 1 - src/util/virtime.c| 1 - src/util/virusb.c | 1 - src/util/virutil.c| 1 - src/util/viruuid.c| 1 - src/util/virxml.c | 1 - src/vz/vz_driver.c| 1 - src/xenapi/xenapi_utils.c | 1 - tests/commandhelper.c | 1 - tests/commandtest.c | 1 - tests/cputest.c | 1 - tests/domainsnapshotxml2xmltest.c | 1 - tests/esxutilstest.c | 1 - tests/genericxml2xmltest.c| 1 - tests/interfacexml2xmltest.c | 1 - tests/libxlxml2domconfigtest.c| 1 - tests/lxcxml2xmltest.c| 1 - tests/networkxml2conftest.c | 1 - tests/networkxml2xmltest.c| 1 - tests/networkxml2xmlupdatetest.c | 1 - tests/nodedevxml2xmltest.c| 1 - tests/nwfilterxml2xmltest.c | 1 - tests/openvzutilstest.c | 1 - tests/qemuargv2xmltest.c | 1 - tests/qemucapsprobemock.c | 1 - tests/qemumemlocktest.c | 1 - tests/qemumonitortestutils.c | 1 - tests/qemuxml2argvtest.c | 1 - tests/qemuxml2xmltest.c | 1 - tests/seclabeltest.c | 1 - tests/securityselinuxlabeltest.c | 1 - tests/securityselinuxtest.c | 1 - tests/sexpr2xmltest.c | 1 - tests/shunloadtest.c | 1 - tests/sockettest.c| 1 - tests/ssh.c | 1 - tests/storagebackendsheepdogtest.c| 1 - tests/storagepoolxml2xmltest.c| 1 - tests/storagevolxml2xmltest.c | 1 - tests/sysinfotest.c | 1 - tests/testutils.c | 1 - tests/testutils.h | 1 - tests/utiltest.c | 1 - tests/vboxsnapshotxmltest.c | 1 - tests/virbuftest.c| 1 - tests/vircgroupmock.c | 1 - tests/virconftest.c | 1 - tests/virfilewrapper.c| 1 - tests/virhashtest.c |
[libvirt] [PATCH 2/2] src: More cleanup of some system headers already contained in internal.h
All of the ones being removed are pulled in by internal.h. The only exception is sanlock which expects the application to include before sanlock's headers, because sanlock prototypes use fixed width int, but they don't include stdint.h themselves, so we have to leave that one in place. Signed-off-by: Erik Skultety --- src/conf/capabilities.c | 3 --- src/conf/network_conf.c | 1 - src/conf/node_device_conf.c | 1 - src/conf/storage_conf.c | 3 --- src/cpu/cpu_ppc64.c | 1 - src/cpu/cpu_ppc64_data.h | 1 - src/cpu/cpu_x86.c | 1 - src/cpu/cpu_x86_data.h| 1 - src/esx/esx_storage_backend_iscsi.c | 1 - src/esx/esx_storage_backend_vmfs.c| 1 - src/esx/esx_vi_types.c| 1 - src/interface/interface_backend_udev.c| 1 - src/libvirt.c | 2 -- src/libxl/libxl_logger.c | 1 - src/locking/lock_daemon.c | 1 - src/locking/lock_driver_sanlock.c | 3 --- src/locking/lock_manager.c| 1 - src/locking/sanlock_helper.c | 1 - src/logging/log_daemon.c | 1 - src/lxc/lxc_container.c | 2 -- src/lxc/lxc_controller.c | 1 - src/lxc/lxc_driver.c | 1 - src/lxc/lxc_fuse.c| 2 -- src/network/bridge_driver.c | 4 src/network/leaseshelper.c| 1 - src/node_device/node_device_driver.c | 1 - src/node_device/node_device_hal.c | 1 - src/node_device/node_device_udev.h| 1 - src/nwfilter/nwfilter_ebiptables_driver.c | 1 - src/openvz/openvz_conf.c | 4 src/openvz/openvz_driver.c| 4 src/phyp/phyp_driver.c| 4 src/qemu/qemu_agent.c | 1 - src/qemu/qemu_conf.c | 4 src/qemu/qemu_driver.c| 4 src/qemu/qemu_hostdev.c | 1 - src/qemu/qemu_monitor_json.c | 1 - src/qemu/qemu_monitor_text.c | 1 - src/qemu/qemu_security.h | 1 - src/remote/remote_daemon.c| 1 - src/rpc/virnetdaemon.c| 1 - src/rpc/virnetmessage.c | 1 - src/rpc/virnetservermdns.c| 1 - src/rpc/virnettlscontext.c| 1 - src/secret/secret_driver.c| 1 - src/security/security_apparmor.c | 1 - src/security/security_driver.c| 1 - src/security/virt-aa-helper.c | 3 --- src/storage/parthelper.c | 1 - src/storage/storage_backend.c | 1 - src/storage/storage_backend_disk.c| 1 - src/storage/storage_backend_fs.c | 2 -- src/storage/storage_backend_iscsi.c | 1 - src/storage/storage_backend_logical.c | 1 - src/storage/storage_driver.c | 2 -- src/storage/storage_file_fs.c | 1 - src/storage/storage_util.c| 1 - src/test/test_driver.c| 1 - src/uml/uml_conf.c| 4 src/uml/uml_driver.c | 4 src/util/iohelper.c | 1 - src/util/viralloc.c | 1 - src/util/virarptable.c| 1 - src/util/virauth.c| 1 - src/util/virbitmap.c | 4 src/util/virbuffer.c | 2 -- src/util/vircgroup.c | 3 --- src/util/vircommand.c | 1 - src/util/virconf.c| 1 - src/util/virdnsmasq.c | 4 src/util/virerror.c | 2 -- src/util/virevent.c | 1 - src/util/vireventpoll.c | 3 --- src/util/virfile.c| 1 - src/util/virhash.c| 2 -- src/util/virhash.h| 2 -- src/util/virhook.c| 1 - src/util/virhostcpu.c | 4 src/util/virhostdev.c | 1 - src/util/virhostmem.c | 4 src/util/viriptables.c| 4 src/util/virkeycode.c | 1 - src/util/virlog.c | 1 - src/util/virmacaddr.c | 1 - src/util/virnetdevmacvlan.c | 2 -- src/util/virnetdevtap.c | 2 -- src/util/virnetdevvportprofile.c | 2 -- src/util/virnetlink.c | 1 - src/util/virpci.c | 3 --- src/util/virprocess.c | 2 -- src/util/virrandom.c | 1 - src/util/virscsi.c| 2 -- src/util/virsexpr.c | 3 --- src/util/virstoragefile.c | 1 -
[libvirt] [PATCH 0/2] Tiny system header cleanup
Cleanup of the headers already pulled in by internal.h header. Examples are of course excluded from this change. Some more serious cleanup might come in a while. Erik Skultety (2): internal: Move include to internal.h src: More cleanup of some system headers already contained in internal.h src/bhyve/bhyve_capabilities.c| 1 - src/conf/capabilities.c | 4 src/conf/network_conf.c | 1 - src/conf/node_device_conf.c | 1 - src/conf/storage_conf.c | 4 src/cpu/cpu_ppc64.c | 1 - src/cpu/cpu_ppc64_data.h | 1 - src/cpu/cpu_x86.c | 1 - src/cpu/cpu_x86_data.h| 1 - src/esx/esx_storage_backend_iscsi.c | 2 -- src/esx/esx_storage_backend_vmfs.c| 2 -- src/esx/esx_vi_types.c| 1 - src/interface/interface_backend_udev.c| 1 - src/internal.h| 1 + src/libvirt.c | 3 --- src/libxl/libxl_logger.c | 1 - src/locking/lock_daemon.c | 1 - src/locking/lock_driver_sanlock.c | 4 src/locking/lock_manager.c| 1 - src/locking/sanlock_helper.c | 2 -- src/logging/log_daemon.c | 1 - src/lxc/lxc_container.c | 3 --- src/lxc/lxc_controller.c | 1 - src/lxc/lxc_driver.c | 1 - src/lxc/lxc_fuse.c| 3 --- src/lxc/lxc_native.c | 1 - src/network/bridge_driver.c | 5 - src/network/leaseshelper.c| 2 -- src/node_device/node_device_driver.c | 1 - src/node_device/node_device_hal.c | 2 -- src/node_device/node_device_udev.h| 1 - src/nwfilter/nwfilter_ebiptables_driver.c | 1 - src/openvz/openvz_conf.c | 5 - src/openvz/openvz_driver.c| 5 - src/phyp/phyp_driver.c| 5 - src/qemu/qemu_agent.c | 1 - src/qemu/qemu_conf.c | 4 src/qemu/qemu_driver.c| 5 - src/qemu/qemu_hostdev.c | 1 - src/qemu/qemu_monitor_json.c | 1 - src/qemu/qemu_monitor_text.c | 1 - src/qemu/qemu_security.h | 1 - src/remote/remote_daemon.c| 1 - src/rpc/virnetdaemon.c| 1 - src/rpc/virnetmessage.c | 1 - src/rpc/virnetservermdns.c| 2 -- src/rpc/virnettlscontext.c| 1 - src/secret/secret_driver.c| 1 - src/security/security_apparmor.c | 1 - src/security/security_driver.c| 1 - src/security/virt-aa-helper.c | 4 src/storage/parthelper.c | 2 -- src/storage/storage_backend.c | 1 - src/storage/storage_backend_disk.c| 2 -- src/storage/storage_backend_fs.c | 3 --- src/storage/storage_backend_iscsi.c | 1 - src/storage/storage_backend_logical.c | 2 -- src/storage/storage_backend_mpath.c | 1 - src/storage/storage_backend_scsi.c| 1 - src/storage/storage_driver.c | 3 --- src/storage/storage_file_fs.c | 1 - src/storage/storage_util.c| 2 -- src/test/test_driver.c| 2 -- src/uml/uml_conf.c| 4 src/uml/uml_driver.c | 5 - src/util/iohelper.c | 2 -- src/util/viralloc.c | 1 - src/util/virarptable.c| 2 -- src/util/viraudit.c | 1 - src/util/virauth.c| 1 - src/util/virbitmap.c | 5 - src/util/virbuffer.c | 3 --- src/util/vircgroup.c | 4 src/util/vircommand.c | 1 - src/util/virconf.c| 2 -- src/util/virdnsmasq.c | 5 - src/util/virerror.c | 3 --- src/util/virevent.c | 1 - src/util/vireventpoll.c | 3 --- src/util/virfile.c| 1 - src/util/virfile.h| 1 - src/util/virgettext.c | 1 - src/util/virhash.c| 2 -- src/util/virhash.h| 2 -- src/util/virhook.c| 2 -- src/util/virhostcpu.c | 5 - src/util/virhostdev.c | 2 -- src/util/virhostmem.c | 5 - src/util/viriptables.c| 5 - src/util/viriscsi.c | 1 - src/util/virkeycode.c | 1 - src/util/virkeyfile.c | 1 - src/util/virlog.c | 2 -- src/util/virmacaddr.c
Re: [libvirt] [PATCH v2 3/3] qemu: add memfd source type
Hi On Wed, Sep 19, 2018 at 1:41 PM Michal Privoznik wrote: > > On 09/17/2018 03:14 PM, marcandre.lur...@redhat.com wrote: > > From: Marc-André Lureau > > > > Add a new memoryBacking source type "memfd", supported by QEMU (when > > the apability is available). > > > > A memfd is a specialized anonymous memory kind. As such, an anonymous > > source type could be automatically using a memfd. However, there are > > some complications when migrating from different memory backends in > > qemu (mainly due to the internal object naming at this point, but > > there could be more). For now, it is simpler and safer to simply > > introduce a new source type "memfd". Eventually, the "anonymous" type > > could learn to use memfd transparently in a seperate change. > > > > The main benefits are that it doesn't need to create filesystem files, > > and it also enforces sealing, providing a bit more safety. > > > > Signed-off-by: Marc-André Lureau > > --- > > docs/formatdomain.html.in | 9 +-- > > docs/schemas/domaincommon.rng | 1 + > > src/conf/domain_conf.c| 3 +- > > src/conf/domain_conf.h| 1 + > > src/qemu/qemu_command.c | 69 +-- > > src/qemu/qemu_domain.c| 12 +++- > > .../memfd-memory-numa.x86_64-latest.args | 34 + > > tests/qemuxml2argvdata/memfd-memory-numa.xml | 36 ++ > > tests/qemuxml2argvtest.c | 2 + > > 9 files changed, 140 insertions(+), 27 deletions(-) > > create mode 100644 > > tests/qemuxml2argvdata/memfd-memory-numa.x86_64-latest.args > > create mode 100644 tests/qemuxml2argvdata/memfd-memory-numa.xml > > > > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > > index 1f12ab5b42..1f6d40 100644 > > --- a/docs/formatdomain.html.in > > +++ b/docs/formatdomain.html.in > > @@ -1099,7 +1099,7 @@ > > /hugepages > > nosharepages/ > > locked/ > > -source type="file|anonymous"/ > > +source type="file|anonymous|memfd"/ > > I'm sorry but I do not think this is the way we should go. This > effectively avoids libvirt making the decision and exposes the backend > used directly. This puts unnecessary burden on mgmt applications because > they have to make yet another decision (track another domain attribute). > > IIUC, memfd is like memory-backend-file and -ram combined. It can do > hugepages or just plain malloc(). Therefore it should be our first > choice for freshly started domains. And only if qemu doesn't support it > we should fall back to either -file or -ram backends. memory-backend-memfd doesn't replace either -file or -ram though. It's a specialized anonymous memory kind, linux-only atm, and not widely available. -file should be used for nvram or complex hugepage/numa setup for ex. But it's legitimate that a VM user request memfd to be used. The point of this patch is not to say that we shouldn't try to use memfd when possible, but rather let the user request specifically memfd, for security reasons for example. If the setup cannot be satisfied with -memfd, the user should get an error. > > This means we have to track what backend the domain was started with so > that we preserve that on migration (although, the fact that these > backends are not interchangeable makes me question 'backend' in their > name :-P). For that we can use status/migration XML as I suggested earlier. > > Once again, status XML is not editable by user [*] and is used solely by > libvirtd to store runtime information for a running domain (and backend > used falls into that category). Why not do this transparent memfd-usage in a seperate series? > > Michal > > * - sure, an evil admin could edit the status XML file (which is usually > stored under /var/run/libvirt/qemu/$domain.xml) and restart libvirtd to > reload the changes. But hey, the file is readable/writable by root only > and there are plenty other ways how an evil root could mess up with > running domains. We (have to) trust root. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 3/3] qemu: add memfd source type
On Wed, Sep 19, 2018 at 11:41:11AM +0200, Michal Privoznik wrote: > On 09/17/2018 03:14 PM, marcandre.lur...@redhat.com wrote: > > From: Marc-André Lureau > > > > Add a new memoryBacking source type "memfd", supported by QEMU (when > > the apability is available). > > > > A memfd is a specialized anonymous memory kind. As such, an anonymous > > source type could be automatically using a memfd. However, there are > > some complications when migrating from different memory backends in > > qemu (mainly due to the internal object naming at this point, but > > there could be more). For now, it is simpler and safer to simply > > introduce a new source type "memfd". Eventually, the "anonymous" type > > could learn to use memfd transparently in a seperate change. > > > > The main benefits are that it doesn't need to create filesystem files, > > and it also enforces sealing, providing a bit more safety. > > > > Signed-off-by: Marc-André Lureau > > --- > > docs/formatdomain.html.in | 9 +-- > > docs/schemas/domaincommon.rng | 1 + > > src/conf/domain_conf.c| 3 +- > > src/conf/domain_conf.h| 1 + > > src/qemu/qemu_command.c | 69 +-- > > src/qemu/qemu_domain.c| 12 +++- > > .../memfd-memory-numa.x86_64-latest.args | 34 + > > tests/qemuxml2argvdata/memfd-memory-numa.xml | 36 ++ > > tests/qemuxml2argvtest.c | 2 + > > 9 files changed, 140 insertions(+), 27 deletions(-) > > create mode 100644 > > tests/qemuxml2argvdata/memfd-memory-numa.x86_64-latest.args > > create mode 100644 tests/qemuxml2argvdata/memfd-memory-numa.xml > > > > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > > index 1f12ab5b42..1f6d40 100644 > > --- a/docs/formatdomain.html.in > > +++ b/docs/formatdomain.html.in > > @@ -1099,7 +1099,7 @@ > > /hugepages > > nosharepages/ > > locked/ > > -source type="file|anonymous"/ > > +source type="file|anonymous|memfd"/ > > I'm sorry but I do not think this is the way we should go. This > effectively avoids libvirt making the decision and exposes the backend > used directly. This puts unnecessary burden on mgmt applications because > they have to make yet another decision (track another domain attribute). > > IIUC, memfd is like memory-backend-file and -ram combined. It can do > hugepages or just plain malloc(). Therefore it should be our first > choice for freshly started domains. And only if qemu doesn't support it > we should fall back to either -file or -ram backends. > > This means we have to track what backend the domain was started with so > that we preserve that on migration (although, the fact that these > backends are not interchangeable makes me question 'backend' in their > name :-P). For that we can use status/migration XML as I suggested earlier. > > Once again, status XML is not editable by user [*] and is used solely by > libvirtd to store runtime information for a running domain (and backend > used falls into that category). I have to agree with Michal, we should not expose this implementation detail in domain XML if we can hide it in status/migratable XML. One thing about the migration though. I'm not sure what are we officially supporting in libvirt because it might cause us some issues. We need to make sure that if you live-migrate domain from old libvirt to new libvirt you should be able to migrate that domain back to old libvirt. The question is, whether this applies if you destroy and start the domain on the new libvirt before you live-migrate it back to old libvirt. Without the restart there is no issue, because the backend would not be changed, but once you start the same domain again we would pick new backend which would prevent migrating it back to the old libvirt. I'm adding Jiri to CC, he should know more. Pavel signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 00/23] Introduce metadata locking
On 09/19/2018 11:17 AM, Bjoern Walk wrote: > Bjoern Walk [2018-09-12, 01:17PM +0200]: >> Michal Privoznik [2018-09-12, 11:32AM +0200]: >> > > Still seeing the same timeout. Is this expected behaviour? > Nope. I wonder if something has locked the path and forgot to unlock it (however, virtlockd should have unlocked all the paths owned by PID on connection close), or something is still holding the lock and connection opened. Can you see the timeout even when you turn off the selinux driver (security_driver="none' in qemu.conf)? I tried to reproduce the issue yesterday and was unsuccessful. Do you have any steps to reproduce? Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 3/3] qemu: add memfd source type
On 09/17/2018 03:14 PM, marcandre.lur...@redhat.com wrote: > From: Marc-André Lureau > > Add a new memoryBacking source type "memfd", supported by QEMU (when > the apability is available). > > A memfd is a specialized anonymous memory kind. As such, an anonymous > source type could be automatically using a memfd. However, there are > some complications when migrating from different memory backends in > qemu (mainly due to the internal object naming at this point, but > there could be more). For now, it is simpler and safer to simply > introduce a new source type "memfd". Eventually, the "anonymous" type > could learn to use memfd transparently in a seperate change. > > The main benefits are that it doesn't need to create filesystem files, > and it also enforces sealing, providing a bit more safety. > > Signed-off-by: Marc-André Lureau > --- > docs/formatdomain.html.in | 9 +-- > docs/schemas/domaincommon.rng | 1 + > src/conf/domain_conf.c| 3 +- > src/conf/domain_conf.h| 1 + > src/qemu/qemu_command.c | 69 +-- > src/qemu/qemu_domain.c| 12 +++- > .../memfd-memory-numa.x86_64-latest.args | 34 + > tests/qemuxml2argvdata/memfd-memory-numa.xml | 36 ++ > tests/qemuxml2argvtest.c | 2 + > 9 files changed, 140 insertions(+), 27 deletions(-) > create mode 100644 > tests/qemuxml2argvdata/memfd-memory-numa.x86_64-latest.args > create mode 100644 tests/qemuxml2argvdata/memfd-memory-numa.xml > > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > index 1f12ab5b42..1f6d40 100644 > --- a/docs/formatdomain.html.in > +++ b/docs/formatdomain.html.in > @@ -1099,7 +1099,7 @@ > /hugepages > nosharepages/ > locked/ > -source type="file|anonymous"/ > +source type="file|anonymous|memfd"/ I'm sorry but I do not think this is the way we should go. This effectively avoids libvirt making the decision and exposes the backend used directly. This puts unnecessary burden on mgmt applications because they have to make yet another decision (track another domain attribute). IIUC, memfd is like memory-backend-file and -ram combined. It can do hugepages or just plain malloc(). Therefore it should be our first choice for freshly started domains. And only if qemu doesn't support it we should fall back to either -file or -ram backends. This means we have to track what backend the domain was started with so that we preserve that on migration (although, the fact that these backends are not interchangeable makes me question 'backend' in their name :-P). For that we can use status/migration XML as I suggested earlier. Once again, status XML is not editable by user [*] and is used solely by libvirtd to store runtime information for a running domain (and backend used falls into that category). Michal * - sure, an evil admin could edit the status XML file (which is usually stored under /var/run/libvirt/qemu/$domain.xml) and restart libvirtd to reload the changes. But hey, the file is readable/writable by root only and there are plenty other ways how an evil root could mess up with running domains. We (have to) trust root. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 05/13] virsh: Implement vshTable API to snapshot-list.
On 09/18/2018 04:21 PM, Simon Kobyda wrote:
> Signed-off-by: Simon Kobyda
> ---
> tools/virsh-snapshot.c | 33 -
> 1 file changed, 20 insertions(+), 13 deletions(-)
>
> diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c
> index a4ea959230..5a02d2c786 100644
> --- a/tools/virsh-snapshot.c
> +++ b/tools/virsh-snapshot.c
> @@ -41,6 +41,7 @@
> #include "virstring.h"
> #include "virxml.h"
> #include "conf/snapshot_conf.h"
> +#include "vsh-table.h"
>
> /* Helper for snapshot-create and snapshot-create-as */
> static bool
> @@ -1487,6 +1488,7 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd)
> char *parent_snap = NULL;
> virDomainSnapshotPtr start = NULL;
> virshSnapshotListPtr snaplist = NULL;
> +vshTablePtr table = NULL;
>
> VSH_EXCLUSIVE_OPTIONS_VAR(tree, name);
> VSH_EXCLUSIVE_OPTIONS_VAR(parent, roots);
> @@ -1547,15 +1549,12 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd)
>
> if (!tree && !name) {
> if (parent)
> -vshPrintExtra(ctl, " %-20s %-25s %-15s %s",
> - _("Name"), _("Creation Time"), _("State"),
> - _("Parent"));
> +table = vshTableNew("Name", "Creation Time", "State", "Parent",
> NULL);
> else
> -vshPrintExtra(ctl, " %-20s %-25s %s",
> - _("Name"), _("Creation Time"), _("State"));
> -vshPrintExtra(ctl, "\n"
> - "--"
> - "--\n");
> +table = vshTableNew("Name", "Creation Time", "State", NULL);
> +
> +if (!table)
> +goto cleanup;
> }
>
> if (tree) {
> @@ -1614,13 +1613,20 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd)
> strftime(timestr, sizeof(timestr), "%Y-%m-%d %H:%M:%S %z",
> _info);
>
> -if (parent)
> -vshPrint(ctl, " %-20s %-25s %-15s %s\n",
> - snap_name, timestr, state, parent_snap);
> -else
> -vshPrint(ctl, " %-20s %-25s %s\n", snap_name, timestr, state);
> +if (parent) {
> +if (vshTableRowAppend(table, snap_name, timestr, state,
> parent_snap,
> + NULL) < 0)
> +continue;
> +} else {
> +if (vshTableRowAppend(table, snap_name, timestr, state,
> + NULL) < 0)
> +continue;
What is the point of these 'continue'? Shouldn't we jump to cleanup instead?
> +}
> }
>
> +if (!tree && !name)
Or simply: if (table)
> +vshTablePrintToStdout(table, ctl);
> +
> ret = true;
>
> cleanup:
> @@ -1633,6 +1639,7 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd)
> xmlFreeDoc(xml);
> VIR_FREE(doc);
> virshDomainFree(dom);
> +vshTableFree(table);
>
> return ret;
> }
>
This is where I'm going to stop my review. You get the idea what changes
you need to do for v2.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 02/13] virsh: Implement vshTable API to net-list and net-dhcp-leases
On 09/18/2018 04:21 PM, Simon Kobyda wrote:
> Signed-off-by: Simon Kobyda
> ---
> tools/virsh-network.c | 55 +--
> 1 file changed, 37 insertions(+), 18 deletions(-)
>
> diff --git a/tools/virsh-network.c b/tools/virsh-network.c
> index ca07fb568f..0f975b8899 100644
> --- a/tools/virsh-network.c
> +++ b/tools/virsh-network.c
> @@ -33,6 +33,7 @@
> #include "virstring.h"
> #include "virtime.h"
> #include "conf/network_conf.h"
> +#include "vsh-table.h"
>
> #define VIRSH_COMMON_OPT_NETWORK(_helpstr, cflags) \
> {.name = "network", \
> @@ -677,6 +678,7 @@ cmdNetworkList(vshControl *ctl, const vshCmd *cmd
> ATTRIBUTE_UNUSED)
> bool optUUID = vshCommandOptBool(cmd, "uuid");
> char uuid[VIR_UUID_STRING_BUFLEN];
> unsigned int flags = VIR_CONNECT_LIST_NETWORKS_ACTIVE;
> +vshTablePtr table = NULL;
>
> if (vshCommandOptBool(cmd, "inactive"))
> flags = VIR_CONNECT_LIST_NETWORKS_INACTIVE;
> @@ -705,10 +707,10 @@ cmdNetworkList(vshControl *ctl, const vshCmd *cmd
> ATTRIBUTE_UNUSED)
> return false;
>
> if (optTable) {
> -vshPrintExtra(ctl, " %-20s %-10s %-13s %s\n", _("Name"), _("State"),
> - _("Autostart"), _("Persistent"));
> -vshPrintExtra(ctl,
> -
> "--\n");
> +table = vshTableNew("Name", "State", "Autostart",
> +"Persistent", NULL);
> +if (!table)
> +goto cleanup;
> }
>
> for (i = 0; i < list->nnets; i++) {
> @@ -722,11 +724,15 @@ cmdNetworkList(vshControl *ctl, const vshCmd *cmd
> ATTRIBUTE_UNUSED)
> else
> autostartStr = is_autostart ? _("yes") : _("no");
>
> -vshPrint(ctl, " %-20s %-10s %-13s %s\n",
> - virNetworkGetName(network),
> - virNetworkIsActive(network) ? _("active") :
> _("inactive"),
> - autostartStr,
> - virNetworkIsPersistent(network) ? _("yes") : _("no"));
> +if (vshTableRowAppend(table,
> + virNetworkGetName(network),
> + virNetworkIsActive(network) ?
> + _("active") : _("inactive"),
> + autostartStr,
> + virNetworkIsPersistent(network) ?
> + _("yes") : _("no"),
> + NULL) < 0)
> +goto cleanup;
> } else if (optUUID) {
> if (virNetworkGetUUIDString(network, uuid) < 0) {
> vshError(ctl, "%s", _("Failed to get network's UUID"));
> @@ -738,8 +744,12 @@ cmdNetworkList(vshControl *ctl, const vshCmd *cmd
> ATTRIBUTE_UNUSED)
> }
> }
>
> +if (optTable)
> +vshTablePrintToStdout(table, ctl);
> +
> ret = true;
> cleanup:
> +vshTableFree(table);
> virshNetworkListFree(list);
> return ret;
> }
> @@ -1351,6 +1361,7 @@ cmdNetworkDHCPLeases(vshControl *ctl, const vshCmd *cmd)
> size_t i;
> unsigned int flags = 0;
> virNetworkPtr network = NULL;
> +vshTablePtr table = NULL;
>
> if (vshCommandOptStringReq(ctl, cmd, "mac", ) < 0)
> return false;
> @@ -1366,11 +1377,11 @@ cmdNetworkDHCPLeases(vshControl *ctl, const vshCmd
> *cmd)
> /* Sort the list according to MAC Address/IAID */
> qsort(leases, nleases, sizeof(*leases), virshNetworkDHCPLeaseSorter);
>
> -vshPrintExtra(ctl, " %-20s %-18s %-9s %-25s %-15s %s\n%s%s\n",
> - _("Expiry Time"), _("MAC address"), _("Protocol"),
> - _("IP address"), _("Hostname"), _("Client ID or DUID"),
> -
> "--",
> -
> "-");
> +table = vshTableNew("Expiry Time", "MAC address", "Protocol",
> +"IP address", "Hostname", "Client ID or DUID",
> +NULL);
> +if (!table)
> +goto cleanup;
>
> for (i = 0; i < nleases; i++) {
> const char *typestr = NULL;
> @@ -1390,17 +1401,25 @@ cmdNetworkDHCPLeases(vshControl *ctl, const vshCmd
> *cmd)
> ignore_value(virAsprintf(_format, "%s/%d",
> lease->ipaddr, lease->prefix));
>
> -vshPrint(ctl, " %-20s %-18s %-9s %-25s %-15s %s\n",
> - expirytime, EMPTYSTR(lease->mac),
> - EMPTYSTR(typestr), cidr_format,
> - EMPTYSTR(lease->hostname), EMPTYSTR(lease->clientid));
> +if (vshTableRowAppend(table,
> + expirytime,
> + EMPTYSTR(lease->mac),
> + EMPTYSTR(typestr),
> + cidr_format,
> +
Re: [libvirt] [PATCH 01/13] virsh: Implement vsh-table to iface-list
On 09/18/2018 04:21 PM, Simon Kobyda wrote:
> Signed-off-by: Simon Kobyda
> ---
> tools/virsh-interface.c | 27 +++
> 1 file changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/tools/virsh-interface.c b/tools/virsh-interface.c
> index 50518c667b..3234845596 100644
> --- a/tools/virsh-interface.c
> +++ b/tools/virsh-interface.c
> @@ -48,6 +48,7 @@
> #include "virutil.h"
> #include "virxml.h"
> #include "virstring.h"
> +#include "vsh-table.h"
>
> virInterfacePtr
> virshCommandOptInterfaceBy(vshControl *ctl, const vshCmd *cmd,
> @@ -356,6 +357,8 @@ cmdInterfaceList(vshControl *ctl, const vshCmd *cmd
> ATTRIBUTE_UNUSED)
> unsigned int flags = VIR_CONNECT_LIST_INTERFACES_ACTIVE;
> virshInterfaceListPtr list = NULL;
> size_t i;
> +bool ret = false;
> +vshTablePtr table = NULL;
>
> if (inactive)
> flags = VIR_CONNECT_LIST_INTERFACES_INACTIVE;
> @@ -366,21 +369,29 @@ cmdInterfaceList(vshControl *ctl, const vshCmd *cmd
> ATTRIBUTE_UNUSED)
> if (!(list = virshInterfaceListCollect(ctl, flags)))
> return false;
>
> -vshPrintExtra(ctl, " %-20s %-10s %s\n", _("Name"), _("State"),
> - _("MAC Address"));
> -vshPrintExtra(ctl,
> "---\n");
> +table = vshTableNew("Name", "State", "MAC Address", NULL);
This is not right. You need to keep the gettext function. If this was
merged as is then no translation would be done. You can find more info
in the commit I've just pushed:
https://www.redhat.com/archives/libvir-list/2018-September/msg00886.html
This applies to the rest of the patches too.
> +if (!table)
> +goto cleanup;
>
> for (i = 0; i < list->nifaces; i++) {
> virInterfacePtr iface = list->ifaces[i];
>
> -vshPrint(ctl, " %-20s %-10s %s\n",
> - virInterfaceGetName(iface),
> - virInterfaceIsActive(iface) ? _("active") : _("inactive"),
> - virInterfaceGetMACString(iface));
> +if (vshTableRowAppend(table,
> + virInterfaceGetName(iface),
> + virInterfaceIsActive(iface) ? _("active")
> + : _("inactive"),
> + virInterfaceGetMACString(iface),
> + NULL) < 0)
> +goto cleanup;
> }
>
> +vshTablePrintToStdout(table, ctl);
> +
> +ret = true;
> + cleanup:
> +vshTableFree(table);
> virshInterfaceListFree(list);
> -return true;
> +return ret;
> }
>
> /*
>
ACK with that fixed.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 03/13] virsh: Implement vshTable API to secret-list
On 09/18/2018 04:21 PM, Simon Kobyda wrote:
> Signed-off-by: Simon Kobyda
> ---
> tools/virsh-secret.c | 30 ++
> 1 file changed, 22 insertions(+), 8 deletions(-)
>
> diff --git a/tools/virsh-secret.c b/tools/virsh-secret.c
> index 670beea706..0ae248b4dd 100644
> --- a/tools/virsh-secret.c
> +++ b/tools/virsh-secret.c
> @@ -35,6 +35,7 @@
> #include "virsecret.h"
> #include "virstring.h"
> #include "virtime.h"
> +#include "vsh-table.h"
>
> static virSecretPtr
> virshCommandOptSecret(vshControl *ctl, const vshCmd *cmd, const char **name)
> @@ -507,6 +508,7 @@ cmdSecretList(vshControl *ctl, const vshCmd *cmd
> ATTRIBUTE_UNUSED)
> virshSecretListPtr list = NULL;
> bool ret = false;
> unsigned int flags = 0;
> +vshTablePtr table = NULL;
>
> if (vshCommandOptBool(cmd, "ephemeral"))
> flags |= VIR_CONNECT_LIST_SECRETS_EPHEMERAL;
> @@ -523,15 +525,17 @@ cmdSecretList(vshControl *ctl, const vshCmd *cmd
> ATTRIBUTE_UNUSED)
> if (!(list = virshSecretListCollect(ctl, flags)))
> return false;
>
> -vshPrintExtra(ctl, " %-36s %s\n", _("UUID"), _("Usage"));
> -vshPrintExtra(ctl, ""
> - "\n");
> +table = vshTableNew("UUID", "Usage", NULL);
> +if (!table)
> +goto cleanup;
>
> for (i = 0; i < list->nsecrets; i++) {
> virSecretPtr sec = list->secrets[i];
> int usageType = virSecretGetUsageType(sec);
> const char *usageStr = virSecretUsageTypeToString(usageType);
> char uuid[VIR_UUID_STRING_BUFLEN];
> +virBuffer buf = VIR_BUFFER_INITIALIZER;
> +const char *usage;
>
> if (virSecretGetUUIDString(sec, uuid) < 0) {
> vshError(ctl, "%s", _("Failed to get uuid of secret"));
> @@ -539,18 +543,28 @@ cmdSecretList(vshControl *ctl, const vshCmd *cmd
> ATTRIBUTE_UNUSED)
> }
>
> if (usageType) {
> -vshPrint(ctl, " %-36s %s %s\n",
> - uuid, usageStr,
> - virSecretGetUsageID(sec));
> +virBufferStrcat(, usageStr, " ",
> +virSecretGetUsageID(sec), NULL);
> +usage = virBufferCurrentContent();
> +if (!usage)
> +goto cleanup;
> +
> +if (vshTableRowAppend(table, uuid, usage, NULL) < 0)
> +goto cleanup;
So if this fails the buffer is leaked. Looks like switching from
virBufferCurrentContent() to virBufferContentAndReset() will prevent this.
> +
> +virBufferFreeAndReset();
> } else {
> -vshPrint(ctl, " %-36s %s\n",
> - uuid, _("Unused"));
> +if (vshTableRowAppend(table, uuid, "Unused", NULL) < 0)
Again, you need to honour the gettext macro (src/internal.h:52).
> +goto cleanup;
> }
> }
>
> +vshTablePrintToStdout(table, ctl);
> +
> ret = true;
>
> cleanup:
> +vshTableFree(table);
> virshSecretListFree(list);
> return ret;
> }
>
Otherwise looking good.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 00/23] Introduce metadata locking
Bjoern Walk [2018-09-12, 01:17PM +0200]: > Michal Privoznik [2018-09-12, 11:32AM +0200]: > > On 09/12/2018 07:19 AM, Bjoern Walk wrote: > > > Michal Privoznik [2018-09-10, 11:36AM +0200]: > > >> Technically, this is v4 of: > > >> > > >> https://www.redhat.com/archives/libvir-list/2018-August/msg01627.html > > >> > > >> However, this is implementing different approach than any of the > > >> previous versions. > > >> > > >> One of the problems with previous version was that it was too > > >> complicated. The main reason for that was that we could not close the > > >> connection whilst there was a file locked. So we had to invent a > > >> mechanism that would prevent that (on the client side). > > >> > > >> These patches implement different approach. They rely on secdriver's > > >> transactions which bring all the paths we want to label into one place > > >> so that they can be relabelled within different namespace. > > >> I'm extending this idea so that transactions run all the time > > >> (regardless of domain namespacing) and only at the very last moment is > > >> decided which namespace would the relabeling run in. > > >> > > >> Metadata locking is then as easy as putting lock/unlock calls around one > > >> function. > > >> > > >> You can find the patches at my github too: > > >> > > >> https://github.com/zippy2/libvirt/tree/disk_metadata_lock_v4_alt > > > > > > Hey Michal, > > > > > > is was running a quick test with this patch series with two domains > > > sharing a disk image without and SELinux enabled. When > > > starting the second domain, the whole libvirtd daemon hangs for almost a > > > minute until giving the error that the image is locked. I haven't > > > debugged it yet to figure out what happens. > > > > Is this on two different hosts or one? > > On the same host. > > > I'm unable to reproduce, so can you please attach debugger and share 't > > a a bt' output with me? > > (gdb) thread apply all bt > > Thread 17 (Thread 0x3ff48dff910 (LWP 193353)): > #0 0x03ff8b70d7b8 in pthread_cond_wait@@GLIBC_2.3.2 () from > /lib64/libpthread.so.0 > #1 0x03ff8c6baf92 in virCondWait () from /lib64/libvirt.so.0 > #2 0x03ff6678c5a8 in udevEventHandleThread (opaque=) at > node_device/node_device_udev.c:1603 > #3 0x03ff8c6bad16 in virThreadHelper () from /lib64/libvirt.so.0 > #4 0x03ff8b7079a8 in start_thread () from /lib64/libpthread.so.0 > #5 0x03ff8b5f9706 in thread_start () from /lib64/libc.so.6 > > Thread 16 (Thread 0x3ff4acfe910 (LWP 193312)): > #0 0x03ff8b70d7b8 in pthread_cond_wait@@GLIBC_2.3.2 () from > /lib64/libpthread.so.0 > #1 0x03ff8c6baf92 in virCondWait () from /lib64/libvirt.so.0 > #2 0x03ff8c6bbbe0 in virThreadPoolWorker () from /lib64/libvirt.so.0 > #3 0x03ff8c6bace6 in virThreadHelper () from /lib64/libvirt.so.0 > #4 0x03ff8b7079a8 in start_thread () from /lib64/libpthread.so.0 > #5 0x03ff8b5f9706 in thread_start () from /lib64/libc.so.6 > > Thread 15 (Thread 0x3ff4b4ff910 (LWP 193311)): > #0 0x03ff8b70d7b8 in pthread_cond_wait@@GLIBC_2.3.2 () from > /lib64/libpthread.so.0 > #1 0x03ff8c6baf92 in virCondWait () from /lib64/libvirt.so.0 > #2 0x03ff8c6bbbe0 in virThreadPoolWorker () from /lib64/libvirt.so.0 > #3 0x03ff8c6bace6 in virThreadHelper () from /lib64/libvirt.so.0 > #4 0x03ff8b7079a8 in start_thread () from /lib64/libpthread.so.0 > #5 0x03ff8b5f9706 in thread_start () from /lib64/libc.so.6 > > Thread 14 (Thread 0x3ff64efd910 (LWP 193310)): > #0 0x03ff8b70d7b8 in pthread_cond_wait@@GLIBC_2.3.2 () from > /lib64/libpthread.so.0 > #1 0x03ff8c6baf92 in virCondWait () from /lib64/libvirt.so.0 > #2 0x03ff8c6bbbe0 in virThreadPoolWorker () from /lib64/libvirt.so.0 > #3 0x03ff8c6bace6 in virThreadHelper () from /lib64/libvirt.so.0 > #4 0x03ff8b7079a8 in start_thread () from /lib64/libpthread.so.0 > #5 0x03ff8b5f9706 in thread_start () from /lib64/libc.so.6 > > Thread 13 (Thread 0x3ff656fe910 (LWP 193309)): > #0 0x03ff8b70d7b8 in pthread_cond_wait@@GLIBC_2.3.2 () from > /lib64/libpthread.so.0 > #1 0x03ff8c6baf92 in virCondWait () from /lib64/libvirt.so.0 > #2 0x03ff8c6bbbe0 in virThreadPoolWorker () from /lib64/libvirt.so.0 > #3 0x03ff8c6bace6 in virThreadHelper () from /lib64/libvirt.so.0 > #4 0x03ff8b7079a8 in start_thread () from /lib64/libpthread.so.0 > #5 0x03ff8b5f9706 in thread_start () from /lib64/libc.so.6 > > Thread 12 (Thread 0x3ff65eff910 (LWP 193308)): > #0 0x03ff8b70d7b8 in pthread_cond_wait@@GLIBC_2.3.2 () from > /lib64/libpthread.so.0 > #1 0x03ff8c6baf92 in virCondWait () from /lib64/libvirt.so.0 > #2 0x03ff8c6bbbe0 in virThreadPoolWorker () from /lib64/libvirt.so.0 > #3 0x03ff8c6bace6 in virThreadHelper () from /lib64/libvirt.so.0 > #4 0x03ff8b7079a8 in start_thread () from /lib64/libpthread.so.0 > #5 0x03ff8b5f9706 in thread_start () from /lib64/libc.so.6 > > Thread 11 (Thread
Re: [libvirt] [PATCH v5 07/13] conf: Introduce parser, formatter for uid and fid
在 2018/9/13 下午9:58, Andrea Bolognani 写道:
On Thu, 2018-09-13 at 17:58 +0800, Yi Min Zhao wrote:
在 2018/9/11 下午8:05, Andrea Bolognani 写道:
On Tue, 2018-09-04 at 16:39 +0800, Yi Min Zhao wrote:
[...]
+bool
+virZPCIDeviceAddressIsEmpty(const virZPCIDeviceAddress *addr)
+{
+return !(addr->uid || addr->fid);
+}
This function belongs in virpci, besides the struct definition and
the very much related virPCIDeviceAddressIsEmpty().
I'm not very clear with your comment. Do you mean I
should move it to other place?
Yeah, the function and its definition should be in src/util/virpci.c
and src/util/virpci.h respectively.
I realize now that virPCIDeviceAddressIsValid() and
virPCIDeviceAddressIsEmpty() are *not* in util/virpci, though I
swear that I posted patches moving them over... My bad, I'll do
that right away.
Sorry for the confusion.
Got it.
[...]
@@ -267,6 +333,9 @@ virPCIDeviceAddressParseXML(xmlNodePtr node,
if (!virPCIDeviceAddressIsEmpty(addr) &&
!virPCIDeviceAddressIsValid(addr, true))
goto cleanup;
+if (virZPCIDeviceAddressParseXML(node, addr) < 0)
+goto cleanup;
I'm not super convinced about this.
On one hand, it makes it so callers can't possibly forget to parse
the zPCI part, and that's literally embedded in the PCI part now;
on the other hand, we have functions to verify separately whether
the PCI and zPCI parts are empty, which is pretty weird.
It's weird indeed. But if we merge zPCI part check into PCI code. We might
have to merge other code. But PCI address has extension only on S390
at least now.
That's not necessarily a problem, at least in principle.
See all the code dealing with "isolation groups", for example:
while the concept is only ever really used for pSeries guests, it's
implemented in a way that's designed to stay out of the way in all
other cases, and the result is that you won't have to worry about
it except when needed.
The zPCI code should ideally behave similarly.
I guess we can leave as it is for now, but the semantics are muddy
enough that I can pretty much guarantee someone will have to clean
them up at some point down the line.
OK.
[...]
@@ -1044,6 +1044,9 @@
virDomainPCIAddressReserveNextAddr(virDomainPCIAddressSetPtr addrs,
dev->isolationGroup, function) < 0)
return -1;
+if (dev->pciAddressExtFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI)
+addr.zpci = dev->addr.pci.zpci;
I'm confused by this part. Shouldn't this either request a new set
of zPCI ids, the same way it's allocating a new PCI address right
above, or do nothing at all because zPCI address allocation is
handled by later calling virDomainZPCIAddressReserveNextAddr()?
Here, we want to store the defined zPCI address which has been reserved.
If we don't do this, we might lose zPCI address and do reservation again
but the reserved one are still existing in zPCI set.
I can't picture the exact scenario right now but I assume something
like that can happen if you have
in which case the zPCI part has already been provided by the user
but we still need to allocate the PCI part ourselves.
Speaking of which, I wonder if something like
would be a more appropriate syntax: not only it clearly shows that
the uid and fid attributes are connected to zPCI, but if we ever
introduce additional PCI address extensions they could be similarly
be represented as childs of instead of adding even more
attributes to the existing element.
Either way, this kind of ad-hoc messing with the zPCI part seems
to me like clear evidence of the fact that we need to step back and
rethink the way the various pieces of the puzzle fit together.
>From the high level point of view, code outside of conf/ should,
for the most part, not need to concern itself with zPCI at all: it
would eg. ask for a PCI address to be allocated, and if the device
in question can be a zPCI device then a zPCI extension address will
be allocated for it as part of the same function call; the only
part of qemu/ that should care about the zPCI address is the one
generating the relevant command line arguments.
Can you try and see whether this kind of API would work?
I did a simple test. It worked. Do you prefer this way?
For this problem, I once thought about adding extension address into
DeviceInfo next to PCI address embraced in a struct. Then here is
not a problem.
That could almost work, seeing how virDomainDeviceInfoFormat()
doesn't use virPCIDeviceAddressFormat() to format the PCI address[1],
but at least in theory you should be able to take a
virPCIDeviceAddress and turn it into a string without having to peek
into other objects such as the parent virDomainDeviceInfo, so that
approach wouldn't be very clean at all.
Right. That's why I finally gave up it.
This is basically another manifestation of the issue I mentioned
above: we don't seem to have a well-defined and strictly adhered
to idea of how the PCI part and zPCI
[libvirt] [PATCH 4/6] build-aux:check-spacing: Add wrapper function of CheckWhiteSpaces
This patch adds CheckWhiteSpaces to simplify check-spacing.
Signed-off-by: Shi Lei
---
build-aux/check-spacing.pl | 204 +++--
1 file changed, 106 insertions(+), 98 deletions(-)
diff --git a/build-aux/check-spacing.pl b/build-aux/check-spacing.pl
index 75bcfd9..cdd991b 100755
--- a/build-aux/check-spacing.pl
+++ b/build-aux/check-spacing.pl
@@ -96,6 +96,111 @@ sub KillComments {
return;
}
+#
+# CheckWhiteSpaces:
+# $_[0]: $data(in)
+# $_[1]: $location(in), which format is file-path:line-num:line-code
+#
+# Check whitespaces according to code spec of libvirt.
+#
+sub CheckWhiteSpaces {
+my $ret = 0;
+my ($data, $location) = @_;
+
+# We need to match things like
+#
+# int foo (int bar, bool wizz);
+# foo (bar, wizz);
+#
+# but not match things like:
+#
+# typedef int (*foo)(bar wizz)
+#
+# we can't do this (efficiently) without
+# missing things like
+#
+# foo (*bar, wizz);
+#
+# We also don't want to spoil the $data so it can be used
+# later on.
+
+# For temporary modifications
+my $tmpdata = $$data;
+while ($tmpdata =~ /(\w+)\s\((?!\*)/) {
+my $kw = $1;
+
+# Allow space after keywords only
+if ($kw =~ /^(?:if|for|while|switch|return)$/) {
+$tmpdata =~ s/(?:$kw\s\()/XXX(/;
+} else {
+print "Whitespace after non-keyword:\n$$location";
+$ret = 1;
+last;
+}
+}
+
+# Require whitespace immediately after keywords
+if ($$data =~ /\b(?:if|for|while|switch|return)\(/) {
+print "No whitespace after keyword:\n$$location";
+$ret = 1;
+}
+
+# Forbid whitespace between )( of a function typedef
+if ($$data =~ /\(\*\w+\)\s+\(/) {
+print "Whitespace between ')' and '(':\n$$location";
+$ret = 1;
+}
+
+# Forbid whitespace following ( or prior to )
+# but allow whitespace before ) on a single line
+# (optionally followed by a semicolon)
+if (($$data =~ /\s\)/ && not $$data =~ /^\s+\);?$/) ||
+$$data =~ /\((?!$)\s/) {
+print "Whitespace after '(' or before ')':\n$$location";
+$ret = 1;
+}
+
+# Forbid whitespace before ";" or ",". Things like below are allowed:
+#
+# 1) The expression is empty for "for" loop. E.g.
+# for (i = 0; ; i++)
+#
+# 2) An empty statement. E.g.
+# while (write(statuswrite, , 1) == -1 &&
+# errno == EINTR)
+# ;
+#
+if ($$data =~ /\s[;,]/) {
+unless ($$data =~ /\S; ; / ||
+$$data =~ /^\s+;/) {
+print "Whitespace before semicolon or comma:\n$$location";
+$ret = 1;
+}
+}
+
+# Require EOL, macro line continuation, or whitespace after ";".
+# Allow "for (;;)" as an exception.
+if ($$data =~ /;[^ \\\n;)]/) {
+print "Invalid character after semicolon:\n$$location";
+$ret = 1;
+}
+
+# Require EOL, space, or enum/struct end after comma.
+if ($$data =~ /,[^ \\\n)}]/) {
+print "Invalid character after comma:\n$$location";
+$ret = 1;
+}
+
+# Require spaces around assignment '=', compounds and '=='
+if ($$data =~ /[^ ]\b[!<>&|\-+*\/%\^=]?=/ ||
+$$data =~ /=[^= \\\n]/) {
+print "Spacing around '=' or '==':\n$$location";
+$ret = 1;
+}
+
+return $ret;
+}
+
my $ret = 0;
foreach my $file (@ARGV) {
@@ -111,8 +216,6 @@ foreach my $file (@ARGV) {
while (defined (my $line = )) {
my $data = $line;
my $location = "$file:$.:\n$line";
-# For temporary modifications
-my $tmpdata;
# Kill any quoted , ; = or "
$data =~ s/'[";,=]'/'X'/g;
@@ -129,102 +232,7 @@ foreach my $file (@ARGV) {
KillComments(\$data, \$incomment);
-# We need to match things like
-#
-# int foo (int bar, bool wizz);
-# foo (bar, wizz);
-#
-# but not match things like:
-#
-# typedef int (*foo)(bar wizz)
-#
-# we can't do this (efficiently) without
-# missing things like
-#
-# foo (*bar, wizz);
-#
-# We also don't want to spoil the $data so it can be used
-# later on.
-$tmpdata = $data;
-while ($tmpdata =~ /(\w+)\s\((?!\*)/) {
-my $kw = $1;
-
-# Allow space after keywords only
-if ($kw =~ /^(?:if|for|while|switch|return)$/) {
-$tmpdata =~ s/(?:$kw\s\()/XXX(/;
-} else {
-print "Whitespace after non-keyword:\n";
-print "$file:$.: $line";
-$ret = 1;
-last;
-}
-}
-
-# Require whitespace immediately after keywords
-if ($data =~ /\b(?:if|for|while|switch|return)\(/) {
-print "No whitespace after keyword:\n";
-print "$file:$.:
[libvirt] [PATCH 6/6] build-aux:check-spacing: Introduce a new rule to check misaligned stuff in parenthesises
This patch introduces a new rule to check misaligned stuff in parenthesis:
1. For misaligned arguments of function
2. For misaligned conditions of [if|while|switch|...]
There're too much misalignment, so it adds a temporary filter which
permits 'src/util' now. It _should_ be removed as soon as fixing all.
Signed-off-by: Shi Lei
---
build-aux/check-spacing.pl | 104 ++---
1 file changed, 96 insertions(+), 8 deletions(-)
diff --git a/build-aux/check-spacing.pl b/build-aux/check-spacing.pl
index 729257c..d36b004 100755
--- a/build-aux/check-spacing.pl
+++ b/build-aux/check-spacing.pl
@@ -100,6 +100,7 @@ sub KillComments {
# CheckWhiteSpaces:
# $_[0]: $data(in)
# $_[1]: $location(in), which format is file-path:line-num:line-code
+# Returns 0 in case of success or 1 on failure
#
# Check whitespaces according to code spec of libvirt.
#
@@ -209,6 +210,7 @@ sub CheckWhiteSpaces {
# $_[3]: $cb_linenum(inout)
# $_[4]: $cb_code(inout)
# $_[5]: $cb_scolon(inout)
+# Returns 0 in case of success or 1 on failure
#
# Check whitespaces according to code spec of libvirt.
#
@@ -250,6 +252,74 @@ sub CheckCurlyBrackets {
return $ret;
}
+#
+# CheckMisalignment:
+# $_[0]: $data(in)
+# $_[1]: $file(in)
+# $_[2]: $line(in)
+# $_[3]: @paren_stack(inout), which maintains information
+# of the parenthesis
+# Returns 0 in case of success or 1 on failure
+#
+# Check misaligned stuff in parenthesis:
+# 1. For misaligned arguments of function
+# 2. For misaligned conditions of [if|while|switch|...]
+#
+sub CheckMisalignment {
+my $ret = 0;
+my ($data, $file, $line, $paren_stack) = @_;
+
+# Check alignment based on @paren_stack
+if (@$paren_stack) {
+if ($$data =~ /(\S+.*$)/) {
+my $pos = $$paren_stack[-1][0];
+my $linenum = $$paren_stack[-1][1];
+my $code = $$paren_stack[-1][2];
+if ($pos + 1 != length($`)) {
+my $pad = "";
+if ($. > $linenum + 1) {
+$pad = " " x $pos . " ...\n";
+}
+print "Misaligned line in parenthesis:\n";
+print "$$file:$linenum-$.:\n$code$pad$$line\n";
+$ret = 1;
+}
+}
+}
+
+# Maintain @paren_stack
+if ($$data =~ /.*[()]/) {
+my $pos = 0;
+my $temp = $$data;
+
+# Kill the content between matched parenthesis and themselves
+# within the current line.
+$temp =~ s,(\((?:[^()]++|(?R))*+\)),"X" x (length $&),ge;
+
+# Pop a item for the open-paren when finding close-paren
+while (($pos = index($temp, "\)", $pos)) >= 0) {
+if (@$paren_stack) {
+pop(@$paren_stack);
+$pos++;
+} else {
+print "Warning: found unbalanced parenthesis:\n";
+print "$$file:$.:\n$$line\n";
+$ret = 1;
+last;
+}
+}
+
+# Push the item for open-paren on @paren_stack
+# @item = [ position of the open-paren, linenum, code-line ]
+while (($pos = index($temp, "\(", $pos)) >= 0) {
+push @$paren_stack, [$pos, $., $$line];
+$pos++;
+}
+}
+
+return $ret;
+}
+
my $ret = 0;
foreach my $file (@ARGV) {
@@ -259,32 +329,50 @@ foreach my $file (@ARGV) {
my $cb_scolon = 0;
my $fn_linenum = 0;
my $incomment = 0;
+my @paren_stack;
open FILE, $file;
while (defined (my $line = )) {
+my $has_define = 0;
my $data = $line;
my $location = "$file:$.:\n$line";
# Kill any quoted , ; = or "
$data =~ s/'[";,=]'/'X'/g;
-# Kill any quoted strings
-$data =~ s,"(?:[^\\\"]|\\.)*","XXX",g;
+# Kill any quoted strings. Replace with equal-length "..."
+$data =~ s,"(([^\\\"]|\\.)*)","\"".'X'x(length $1)."\"",ge;
+$data =~ s,'(([^\\\']|\\.)*)',"\'".'X'x(length $1)."\'",ge;
# Kill any C++ style comments
$data =~ s,//.*$,//,;
-next if $data =~ /^#/;
+$has_define = 1 if $data =~ /(?:^#\s*define\b)/;
+if (not $has_define) {
+# Ignore all macros except for #define
+next if $data =~ /^#/;
-$ret = 1 if CheckFunctionBody(\$data, \$location, \$fn_linenum);
+$ret = 1 if CheckFunctionBody(\$data, \$location, \$fn_linenum);
-KillComments(\$data, \$incomment);
+KillComments(\$data, \$incomment);
+
+$ret = 1 if CheckWhiteSpaces(\$data, \$location);
+
+$ret = 1 if CheckCurlyBrackets(\$data, \$file, \$line,
+ \$cb_linenum, \$cb_code,
\$cb_scolon);
+}
-$ret = 1 if CheckWhiteSpaces(\$data, \$location);
+#
+# Temporary Filter for CheckMisalignment:
+# Here we
[libvirt] [PATCH 2/6] build-aux:check-spacing: Add wrapper function of CheckFunctionBody
This patch adds CheckFunctionBody to simplifies check-spacing.
Signed-off-by: Shi Lei
---
build-aux/check-spacing.pl | 61 +-
1 file changed, 41 insertions(+), 20 deletions(-)
diff --git a/build-aux/check-spacing.pl b/build-aux/check-spacing.pl
index da9a58b..c5d5a69 100755
--- a/build-aux/check-spacing.pl
+++ b/build-aux/check-spacing.pl
@@ -23,6 +23,45 @@
use strict;
use warnings;
+#
+# CheckFunctionBody:
+# $_[0]: $data(in)
+# $_[1]: $location(in), which format is file-path:line-num:line-code
+# $_[2]: $fn_linenum(inout), maintains start line-num of function body
+# Returns 0 in case of success or 1 on failure
+#
+# Check incorrect indentation and blank first line in function body.
+# For efficiency, it only checks the first line of function body.
+# But it's enough for most cases.
+# (It could be better that we use *state* to declare @fn_linenum and
+# move it into this subroutine. But *state* requires version >= v5.10.)
+#
+sub CheckFunctionBody {
+my $ret = 0;
+my ($data, $location, $fn_linenum) = @_;
+
+# Check first line of function block
+if ($$fn_linenum) {
+if ($$data =~ /^\s*$/) {
+print "Blank line before content in function body:\n$$location";
+$ret = 1;
+} elsif ($$data !~ /^[ ]{4}\S/) {
+unless ($$data =~ /^[ ]\w+:$/ || $$data =~ /^}/) {
+print "Incorrect indentation in function body:\n$$location";
+$ret = 1;
+}
+}
+$$fn_linenum = 0;
+}
+
+# Detect start of function block
+if ($$data =~ /^{$/) {
+$$fn_linenum = $.;
+}
+
+return $ret;
+}
+
my $ret = 0;
my $incomment = 0;
@@ -37,6 +76,7 @@ foreach my $file (@ARGV) {
while (defined (my $line = )) {
my $data = $line;
+my $location = "$file:$.:\n$line";
# For temporary modifications
my $tmpdata;
@@ -51,26 +91,7 @@ foreach my $file (@ARGV) {
next if $data =~ /^#/;
-# Detect start of function block
-if ($data =~ /^{$/) {
-$fn_linenum = $.;
-}
-
-# Handle first line of function block
-if ($fn_linenum && $fn_linenum != $.) {
-if ($data =~ /^\s*$/) {
-print "Blank line before content in function body:\n";
-print "$file:$.:\n$line";
-$ret = 1;
-} elsif ($data !~ /^[ ]{4}\S/) {
-unless ($data =~ /^[ ]\w+:$/ || $data =~ /^}/) {
-print "Incorrect indentation in function body:\n";
-print "$file:$.:\n$line";
-$ret = 1;
-}
-}
-$fn_linenum = 0;
-}
+$ret = 1 if CheckFunctionBody(\$data, \$location, \$fn_linenum);
# Kill contents of multi-line comments
# and detect end of multi-line comments
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/6] util: Fix misaligned arguments and misaligned conditions for [if|while|...]
This patch just fixes misaligned arguments and misaligned conditions
of src/util/*.c.
Signed-off-by: Shi Lei
---
src/util/vircgroup.c | 34 +-
src/util/virconf.c | 4 +--
src/util/virdbus.c | 2 +-
src/util/virdnsmasq.c| 5 +--
src/util/virerror.c | 2 +-
src/util/virevent.c | 14
src/util/vireventpoll.c | 8 ++---
src/util/virfile.c | 6 ++--
src/util/virgic.c| 2 +-
src/util/virhook.c | 2 +-
src/util/virhostdev.c| 23 ++--
src/util/viridentity.c | 18 +-
src/util/viriscsi.c | 22 ++--
src/util/virjson.c | 2 +-
src/util/virkeycode.c| 22 ++--
src/util/virkeyfile.c| 6 ++--
src/util/virlockspace.c | 2 +-
src/util/virlog.c| 18 +-
src/util/virnetdev.c | 4 +--
src/util/virnetdevbridge.c | 2 +-
src/util/virnetdevip.c | 11 +++---
src/util/virnetdevmacvlan.c | 30
src/util/virnetdevopenvswitch.c | 34 +-
src/util/virnetdevvportprofile.c | 24 ++---
src/util/virnetlink.c| 10 +++---
src/util/virobject.c | 2 +-
src/util/virpci.c| 4 +--
src/util/virperf.c | 4 +--
src/util/virprocess.c| 2 +-
src/util/virqemu.c | 4 +--
src/util/virresctrl.c| 7 ++--
src/util/virsocketaddr.c | 2 +-
src/util/virstorageencryption.c | 2 +-
src/util/virstoragefile.c| 60 ++--
src/util/virsysinfo.c| 2 +-
src/util/viruri.c| 2 +-
src/util/virutil.c | 2 +-
src/util/virxml.c| 2 +-
38 files changed, 197 insertions(+), 205 deletions(-)
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index 64507bf..ab5ff6d 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -2066,9 +2066,9 @@ virCgroupSetBlkioDeviceReadIops(virCgroupPtr group,
return -1;
return virCgroupSetValueStr(group,
- VIR_CGROUP_CONTROLLER_BLKIO,
- "blkio.throttle.read_iops_device",
- str);
+VIR_CGROUP_CONTROLLER_BLKIO,
+"blkio.throttle.read_iops_device",
+str);
}
@@ -2095,9 +2095,9 @@ virCgroupSetBlkioDeviceWriteIops(virCgroupPtr group,
return -1;
return virCgroupSetValueStr(group,
- VIR_CGROUP_CONTROLLER_BLKIO,
- "blkio.throttle.write_iops_device",
- str);
+VIR_CGROUP_CONTROLLER_BLKIO,
+"blkio.throttle.write_iops_device",
+str);
}
@@ -2124,9 +2124,9 @@ virCgroupSetBlkioDeviceReadBps(virCgroupPtr group,
return -1;
return virCgroupSetValueStr(group,
- VIR_CGROUP_CONTROLLER_BLKIO,
- "blkio.throttle.read_bps_device",
- str);
+VIR_CGROUP_CONTROLLER_BLKIO,
+"blkio.throttle.read_bps_device",
+str);
}
/**
@@ -2152,9 +2152,9 @@ virCgroupSetBlkioDeviceWriteBps(virCgroupPtr group,
return -1;
return virCgroupSetValueStr(group,
- VIR_CGROUP_CONTROLLER_BLKIO,
- "blkio.throttle.write_bps_device",
- str);
+VIR_CGROUP_CONTROLLER_BLKIO,
+"blkio.throttle.write_bps_device",
+str);
}
@@ -2182,9 +2182,9 @@ virCgroupSetBlkioDeviceWeight(virCgroupPtr group,
return -1;
return virCgroupSetValueStr(group,
- VIR_CGROUP_CONTROLLER_BLKIO,
- "blkio.weight_device",
- str);
+VIR_CGROUP_CONTROLLER_BLKIO,
+"blkio.weight_device",
+str);
}
/**
@@ -3218,8 +3218,8 @@ virCgroupGetPercpuStats(virCgroupPtr group,
goto cleanup;
for (i = start_cpu; i < need_cpus; i++) {
-if (virTypedParameterAssign([(i - start_cpu) * nparams +
-param_idx],
+int idx = (i - start_cpu) * nparams + param_idx;
+if (virTypedParameterAssign([idx],
VIR_DOMAIN_CPU_STATS_VCPUTIME,
VIR_TYPED_PARAM_ULLONG,
[libvirt] [PATCH 5/6] build-aux:check-spacing: Add wrapper function of CheckCurlyBrackets
This patch adds CheckCurlyBrackets to simplify check-spacing.
Signed-off-by: Shi Lei
---
build-aux/check-spacing.pl | 81 --
1 file changed, 51 insertions(+), 30 deletions(-)
diff --git a/build-aux/check-spacing.pl b/build-aux/check-spacing.pl
index cdd991b..729257c 100755
--- a/build-aux/check-spacing.pl
+++ b/build-aux/check-spacing.pl
@@ -201,6 +201,55 @@ sub CheckWhiteSpaces {
return $ret;
}
+#
+# CheckCurlyBrackets:
+# $_[0]: $data(in)
+# $_[1]: $file(in)
+# $_[2]: $line(in)
+# $_[3]: $cb_linenum(inout)
+# $_[4]: $cb_code(inout)
+# $_[5]: $cb_scolon(inout)
+#
+# Check whitespaces according to code spec of libvirt.
+#
+sub CheckCurlyBrackets {
+my $ret = 0;
+my ($data, $file, $line, $cb_linenum, $cb_code, $cb_scolon) = @_;
+
+# One line conditional statements with one line bodies should
+# not use curly brackets.
+if ($$data =~ /^\s*(if|while|for)\b.*\{$/) {
+$$cb_linenum = $.;
+$$cb_code = $$line;
+$$cb_scolon = 0;
+}
+
+# We need to check for exactly one semicolon inside the body,
+# because empty statements (e.g. with comment only) are
+# allowed
+if ($$cb_linenum == $. - 1 && $$data =~ /^[^;]*;[^;]*$/) {
+$$cb_code .= $$line;
+$$cb_scolon = 1;
+}
+
+if ($$data =~ /^\s*}\s*$/ &&
+$$cb_linenum == $. - 2 &&
+$$cb_scolon) {
+
+print "Curly brackets around single-line body:\n";
+print "$$file:$$cb_linenum-$.:\n$$cb_code$$line";
+$ret = 1;
+
+# There _should_ be no need to reset the values; but to
+# keep my inner peace...
+$$cb_linenum = 0;
+$$cb_scolon = 0;
+$$cb_code = "";
+}
+
+return $ret;
+}
+
my $ret = 0;
foreach my $file (@ARGV) {
@@ -234,36 +283,8 @@ foreach my $file (@ARGV) {
$ret = 1 if CheckWhiteSpaces(\$data, \$location);
-# One line conditional statements with one line bodies should
-# not use curly brackets.
-if ($data =~ /^\s*(if|while|for)\b.*\{$/) {
-$cb_linenum = $.;
-$cb_code = $line;
-$cb_scolon = 0;
-}
-
-# We need to check for exactly one semicolon inside the body,
-# because empty statements (e.g. with comment only) are
-# allowed
-if ($cb_linenum == $. - 1 && $data =~ /^[^;]*;[^;]*$/) {
-$cb_code .= $line;
-$cb_scolon = 1;
-}
-
-if ($data =~ /^\s*}\s*$/ &&
-$cb_linenum == $. - 2 &&
-$cb_scolon) {
-
-print "Curly brackets around single-line body:\n";
-print "$file:$cb_linenum-$.:\n$cb_code$line";
-$ret = 1;
-
-# There _should_ be no need to reset the values; but to
-# keep my inner peace...
-$cb_linenum = 0;
-$cb_scolon = 0;
-$cb_code = "";
-}
+$ret = 1 if CheckCurlyBrackets(\$data, \$file, \$line,
+ \$cb_linenum, \$cb_code, \$cb_scolon);
}
close FILE;
}
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/6] syntax-check: Introduce a new rule to check misaligned stuff and fix it (batch I)
This series check misaligned stuff in parenthesis: 1. For misaligned arguments of function 2. For misaligned conditions of [if|while|switch|...] It adds a temporary filter for this new rule since there're too much misalignment. Now it just fix misalignment in src/util and it's the batch I. Next step, we need modify this path filter to finish all changes. Also, it simplifies the check-spacing.pl by adding wrapper functions before introducing the new rule. Thanks, Shi Lei Shi Lei (6): util: Fix misaligned arguments and misaligned conditions for [if|while|...] build-aux:check-spacing: Add wrapper function of CheckFunctionBody build-aux:check-spacing: Add wrapper function of KillComments build-aux:check-spacing: Add wrapper function of CheckWhiteSpaces build-aux:check-spacing: Add wrapper function of CheckCurlyBrackets build-aux:check-spacing: Introduce a new rule to check misaligned stuff in parenthesises build-aux/check-spacing.pl | 476 --- src/util/vircgroup.c | 34 +-- src/util/virconf.c | 4 +- src/util/virdbus.c | 2 +- src/util/virdnsmasq.c| 5 +- src/util/virerror.c | 2 +- src/util/virevent.c | 14 +- src/util/vireventpoll.c | 8 +- src/util/virfile.c | 6 +- src/util/virgic.c| 2 +- src/util/virhook.c | 2 +- src/util/virhostdev.c| 23 +- src/util/viridentity.c | 18 +- src/util/viriscsi.c | 22 +- src/util/virjson.c | 2 +- src/util/virkeycode.c| 22 +- src/util/virkeyfile.c| 6 +- src/util/virlockspace.c | 2 +- src/util/virlog.c| 18 +- src/util/virnetdev.c | 4 +- src/util/virnetdevbridge.c | 2 +- src/util/virnetdevip.c | 11 +- src/util/virnetdevmacvlan.c | 30 +- src/util/virnetdevopenvswitch.c | 34 +-- src/util/virnetdevvportprofile.c | 24 +- src/util/virnetlink.c| 10 +- src/util/virobject.c | 2 +- src/util/virpci.c| 4 +- src/util/virperf.c | 4 +- src/util/virprocess.c| 2 +- src/util/virqemu.c | 4 +- src/util/virresctrl.c| 7 +- src/util/virsocketaddr.c | 2 +- src/util/virstorageencryption.c | 2 +- src/util/virstoragefile.c| 60 ++-- src/util/virsysinfo.c| 2 +- src/util/viruri.c| 2 +- src/util/virutil.c | 2 +- src/util/virxml.c| 2 +- 39 files changed, 512 insertions(+), 366 deletions(-) -- 2.17.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/6] build-aux:check-spacing: Add wrapper function of KillComments
This patch adds KillComments to simplifies check-spacing.
Signed-off-by: Shi Lei
---
build-aux/check-spacing.pl | 56 --
1 file changed, 36 insertions(+), 20 deletions(-)
diff --git a/build-aux/check-spacing.pl b/build-aux/check-spacing.pl
index c5d5a69..75bcfd9 100755
--- a/build-aux/check-spacing.pl
+++ b/build-aux/check-spacing.pl
@@ -62,8 +62,41 @@ sub CheckFunctionBody {
return $ret;
}
+#
+# KillComments:
+# $_[0]: $data(inout)
+# $_[1]: $incomment(inout)
+#
+# Remove all content of comments
+# (Also, the @incomment could be declared with *state* and move it in.)
+#
+sub KillComments {
+my ($data, $incomment) = @_;
+
+# Kill contents of multi-line comments
+# and detect end of multi-line comments
+if ($$incomment) {
+if ($$data =~ m,\*/,) {
+$$incomment = 0;
+$$data =~ s,^.*\*/,*/,;
+} else {
+$$data = "";
+}
+}
+
+# Kill single line comments, and detect
+# start of multi-line comments
+if ($$data =~ m,/\*.*\*/,) {
+$$data =~ s,/\*.*\*/,/* */,;
+} elsif ($$data =~ m,/\*,) {
+$$incomment = 1;
+$$data =~ s,/\*.*,/*,;
+}
+
+return;
+}
+
my $ret = 0;
-my $incomment = 0;
foreach my $file (@ARGV) {
# Per-file variables for multiline Curly Bracket (cb_) check
@@ -71,6 +104,7 @@ foreach my $file (@ARGV) {
my $cb_code = "";
my $cb_scolon = 0;
my $fn_linenum = 0;
+my $incomment = 0;
open FILE, $file;
@@ -93,25 +127,7 @@ foreach my $file (@ARGV) {
$ret = 1 if CheckFunctionBody(\$data, \$location, \$fn_linenum);
-# Kill contents of multi-line comments
-# and detect end of multi-line comments
-if ($incomment) {
-if ($data =~ m,\*/,) {
-$incomment = 0;
-$data =~ s,^.*\*/,*/,;
-} else {
-$data = "";
-}
-}
-
-# Kill single line comments, and detect
-# start of multi-line comments
-if ($data =~ m,/\*.*\*/,) {
-$data =~ s,/\*.*\*/,/* */,;
-} elsif ($data =~ m,/\*,) {
-$incomment = 1;
-$data =~ s,/\*.*,/*,;
-}
+KillComments(\$data, \$incomment);
# We need to match things like
#
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virsh: Honour user locale in cmdList
On Wed, Sep 19, 2018 at 10:26:41AM +0200, Michal Privoznik wrote: > In 2e97450425e we've mistakenly removed gettext macro for > translating static strings. This results in table header being > printed in English regardless of user locale. > > Signed-off-by: Michal Privoznik > --- Reviewed-by: Erik Skultety -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] virsh: Honour user locale in cmdList
In 2e97450425e we've mistakenly removed gettext macro for
translating static strings. This results in table header being
printed in English regardless of user locale.
Signed-off-by: Michal Privoznik
---
tools/virsh-domain-monitor.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
index adc5bb1a7a..8962586d76 100644
--- a/tools/virsh-domain-monitor.c
+++ b/tools/virsh-domain-monitor.c
@@ -1942,9 +1942,9 @@ cmdList(vshControl *ctl, const vshCmd *cmd)
/* print table header in legacy mode */
if (optTable) {
if (optTitle)
-table = vshTableNew("Id", "Name", "State", "Title", NULL);
+table = vshTableNew(_("Id"), _("Name"), _("State"), _("Title"),
NULL);
else
-table = vshTableNew("Id", "Name", "State", NULL);
+table = vshTableNew(_("Id"), _("Name"), _("State"), NULL);
if (!table)
goto cleanup;
--
2.16.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] conf: fix starting a domain with cpuset=""
On Wed, Sep 19, 2018 at 03:51:55PM +0800, wang.y...@zte.com.cn wrote:
> > On Wed, Sep 19, 2018 at 08:11:34AM +0800, wang.y...@zte.com.cn wrote:
> > > > On Mon, Sep 17, 2018 at 06:53:13PM +0800, wang.y...@zte.com.cn wrote:
> > > > > > On Sat, Sep 15, 2018 at 04:29:24PM +0800, Yi Wang wrote:
> > > > > > > Domain fails to start when its config xml including:
> > > > > > > 64
> > > > > > >
> > > > > > > # virsh create vm.xml
> > > > > > > error: Failed to create domain from vm.xml
> > > > > > > error: invalid argument: Failed to parse bitmap ''
> > > > > > >
> > > > > > > This patch fixes this.
> > > > > > >
> > > > > > > Signed-off-by: Yi Wang
> > > > > > > Reviewed-by: Xi Xu
> > > > > > > ---
> > > > > > > src/conf/domain_conf.c | 2 +-
> > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > > > > > > index 8619962..bacafb2 100644
> > > > > > > --- a/src/conf/domain_conf.c
> > > > > > > +++ b/src/conf/domain_conf.c
> > > > > > > @@ -18553,7 +18553,7 @@ virDomainVcpuParse(virDomainDefPtr def,
> > > > > > >
> > > > > > > if (def->placement_mode !=
> > > > > > > VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) {
> > > > > > > tmp = virXMLPropString(vcpuNode, "cpuset");
> > > > > > > -if (tmp) {
> > > > > > > +if (tmp && strlen(tmp) != 0) {
> > > > > >
> > > > > > '&& *tmp' would suffice.
> > > > >
> > > > > Ok.
> > > > >
> > > > > >
> > > > > > The patch is correct, but I believe there is a number of spots in
> > > > > > the massive
> > > > > > domain_conf.c file where a similar fix would be needed, it might be
> > > > > > worth
> > > > > > checking all the spots where no conversion like string-to-int
> > > > > > string-to-enum or
> > > > > > any other additional parsing like address parsing is performed,
> > > > > > those might be
> > > > > > good candidates.
> > > > > >
> > > > > > I understand the file is massive, so let me know how that goes.
> > > > >
> > > > > In most similar places we have already checked by the return values
> > > > > of function
> > > > > like string-to-int string-to-enum, and if failed, libvirt will give
> > > > > explicit report.
> > > > >
> > > > > To create a domain with iothread="" of disk, for instance:
> > > > >
> > > > > # virsh create vm.xml
> > > > > error: Failed to create domain from vm.xml
> > > > > error: XML error: Invalid iothread attribute in disk driver element:
> > > > >
> > > > > But the cpuset="" config error report may make people confuesd, and
> > > > > in the older
> > > > > version like 3.1.0 that's ok to start a domain with this config. So I
> > > > > think this patch
> > > > > is enough.
> > > >
> > > > So, I thought about this quite a bit. The fact that 3.1.0 was okay with
> > > > that
> > > > was because commit b71946af5c7 changed the helper we use during
> > > > parsing. The
> > > > original one considered NULL and foo[0] == '\0' to be the same and
> > > > always
> > > > returned NULL. However, now that I looked at the code more closely, I
> > > > don't
> > > > think we want this patch, in fact, an empty cpuset is an invalid
> > > > attribute,
> > > > what's the point of having an empty cpuset vs not having it at all, if
> > > > you don't
> > > > want static pinning then don't specify one, there's no practical use
> > > > case in
> > > > that. That being said, I also find the error very generic and vague, so
> > > > if
> > > > anyone wants to actually figure out what the part causing problems in
> > > > the XML
> > > > parsing is, they have no chance, we should improve that. Paradoxically,
> > > > we had
> > > > that back in 2013 until commit 106a2ddaa7b changed it with absolutely no
> > > > rationale or a corresponding BZ on why decreasing user experience was a
> > > > good
> > > > idea, I know, rather a rare use case what you have here, but still, why
> > > > is okay
> > > > to parse numbers from string with no error, thus giving a clear error
> > > > (like your
> > > > iothread example), but not okay for bitmaps, CC'ing Peter to give more
> > > > information about that change.
> > >
> > > I have checked the commit b71946af5c7 before sending this patch and came
> > > up with two solutions:
> > >
> > > - make cpuset="" passed the check, like this patch does
> > > - add specified error report, rather than complaining about parsing bitmap
> > >
> > > The first method can keep consistence with the older version, so I chose
> > > this one.
> >
> > Well, in this case being consistent with something which wasn't imho
> > entirely
> > correct is not the way to go, the consistency should have been with all the
> > other cases where the parsing simply fails because it can't parse an empty
> > string into anything meaningful. As I said, I don't see a point in using
> > cpuset="" in real world. It would be different if the kind of change commit
> > b71946af5c7 introduced made some VMs
Re: [libvirt] [PATCH V3 1/2] libxl: drop support for Xen < 4.6
On Tue, 2018-09-18 at 10:16 +0200, Andrea Bolognani wrote: > On Mon, 2018-09-17 at 10:00 -0600, Jim Fehlig wrote: > > On 9/17/18 2:59 AM, Andrea Bolognani wrote: > > > Depending on how long it takes to get that fixed in Fedora, we > > > might want to temporarily reintroduce the fallback path... > > > > Ok. What is your estimate of "long" in this case? :-) By the end of the > > day? End > > of week? Before freeze for next release? > > I'd say we give the maintainer a few days to respond, and if > nothing happens by next Monday we just put the code back in. The maintainer has fixed the issue in Fedora 29+, but they're understandably not going to backport the change to earlier Fedora releases: https://bugzilla.redhat.com/show_bug.cgi?id=1629643#c2 So we're going to need the fallback code for ~6 more months. Would you mind posting a patch that reintroduces it along with a comment mentioning that we can drop it as soon as Fedora 28 goes out of support? -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/2] cpu_map: Add Icelake CPU models
On 09/18/2018 03:55 PM, Jiri Denemark wrote: > The second patch mentions that ospke feature was removed in QEMU 3.0, > see > http://lists.nongnu.org/archive/html/qemu-devel/2018-09/msg02113.html > for discussion about how to deal with removed CPU features. > > Jiri Denemark (2): > cpu_map: Add features for Icelake CPUs > cpu_map: Add Icelake CPU models > > src/cpu_map/x86_Icelake-Client.xml| 85 + > src/cpu_map/x86_Icelake-Server.xml| 95 +++ > src/cpu_map/x86_features.xml | 33 +++ > .../x86_64-cpuid-Core-i5-6600-guest.xml | 1 + > .../x86_64-cpuid-Core-i5-6600-host.xml| 1 + > .../x86_64-cpuid-Core-i7-5600U-arat-guest.xml | 1 + > .../x86_64-cpuid-Core-i7-5600U-arat-host.xml | 1 + > .../x86_64-cpuid-Core-i7-5600U-guest.xml | 1 + > .../x86_64-cpuid-Core-i7-5600U-host.xml | 1 + > .../x86_64-cpuid-Core-i7-5600U-ibrs-guest.xml | 1 + > .../x86_64-cpuid-Core-i7-5600U-ibrs-host.xml | 1 + > .../x86_64-cpuid-Core-i7-7700-guest.xml | 1 + > .../x86_64-cpuid-Core-i7-7700-host.xml| 1 + > .../x86_64-cpuid-Xeon-E3-1245-v5-guest.xml| 1 + > .../x86_64-cpuid-Xeon-E3-1245-v5-host.xml | 1 + > .../x86_64-cpuid-Xeon-E5-2623-v4-guest.xml| 1 + > .../x86_64-cpuid-Xeon-E5-2623-v4-host.xml | 1 + > .../x86_64-cpuid-Xeon-E5-2650-v4-guest.xml| 1 + > .../x86_64-cpuid-Xeon-E5-2650-v4-host.xml | 1 + > .../x86_64-cpuid-Xeon-Gold-5115-guest.xml | 1 + > .../x86_64-cpuid-Xeon-Gold-5115-host.xml | 1 + > .../x86_64-cpuid-Xeon-Gold-6148-guest.xml | 1 + > .../x86_64-cpuid-Xeon-Gold-6148-host.xml | 1 + > 23 files changed, 233 insertions(+) > create mode 100644 src/cpu_map/x86_Icelake-Client.xml > create mode 100644 src/cpu_map/x86_Icelake-Server.xml > ACK Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] conf: fix starting a domain with cpuset=""
> On Wed, Sep 19, 2018 at 08:11:34AM +0800, wang.y...@zte.com.cn wrote:
> > > On Mon, Sep 17, 2018 at 06:53:13PM +0800, wang.y...@zte.com.cn wrote:
> > > > > On Sat, Sep 15, 2018 at 04:29:24PM +0800, Yi Wang wrote:
> > > > > > Domain fails to start when its config xml including:
> > > > > > 64
> > > > > >
> > > > > > # virsh create vm.xml
> > > > > > error: Failed to create domain from vm.xml
> > > > > > error: invalid argument: Failed to parse bitmap ''
> > > > > >
> > > > > > This patch fixes this.
> > > > > >
> > > > > > Signed-off-by: Yi Wang
> > > > > > Reviewed-by: Xi Xu
> > > > > > ---
> > > > > > src/conf/domain_conf.c | 2 +-
> > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > > > > > index 8619962..bacafb2 100644
> > > > > > --- a/src/conf/domain_conf.c
> > > > > > +++ b/src/conf/domain_conf.c
> > > > > > @@ -18553,7 +18553,7 @@ virDomainVcpuParse(virDomainDefPtr def,
> > > > > >
> > > > > > if (def->placement_mode !=
> > > > > > VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) {
> > > > > > tmp = virXMLPropString(vcpuNode, "cpuset");
> > > > > > -if (tmp) {
> > > > > > +if (tmp && strlen(tmp) != 0) {
> > > > >
> > > > > '&& *tmp' would suffice.
> > > >
> > > > Ok.
> > > >
> > > > >
> > > > > The patch is correct, but I believe there is a number of spots in the
> > > > > massive
> > > > > domain_conf.c file where a similar fix would be needed, it might be
> > > > > worth
> > > > > checking all the spots where no conversion like string-to-int
> > > > > string-to-enum or
> > > > > any other additional parsing like address parsing is performed, those
> > > > > might be
> > > > > good candidates.
> > > > >
> > > > > I understand the file is massive, so let me know how that goes.
> > > >
> > > > In most similar places we have already checked by the return values of
> > > > function
> > > > like string-to-int string-to-enum, and if failed, libvirt will give
> > > > explicit report.
> > > >
> > > > To create a domain with iothread="" of disk, for instance:
> > > >
> > > > # virsh create vm.xml
> > > > error: Failed to create domain from vm.xml
> > > > error: XML error: Invalid iothread attribute in disk driver element:
> > > >
> > > > But the cpuset="" config error report may make people confuesd, and in
> > > > the older
> > > > version like 3.1.0 that's ok to start a domain with this config. So I
> > > > think this patch
> > > > is enough.
> > >
> > > So, I thought about this quite a bit. The fact that 3.1.0 was okay with
> > > that
> > > was because commit b71946af5c7 changed the helper we use during parsing.
> > > The
> > > original one considered NULL and foo[0] == '\0' to be the same and always
> > > returned NULL. However, now that I looked at the code more closely, I
> > > don't
> > > think we want this patch, in fact, an empty cpuset is an invalid
> > > attribute,
> > > what's the point of having an empty cpuset vs not having it at all, if
> > > you don't
> > > want static pinning then don't specify one, there's no practical use case
> > > in
> > > that. That being said, I also find the error very generic and vague, so if
> > > anyone wants to actually figure out what the part causing problems in the
> > > XML
> > > parsing is, they have no chance, we should improve that. Paradoxically,
> > > we had
> > > that back in 2013 until commit 106a2ddaa7b changed it with absolutely no
> > > rationale or a corresponding BZ on why decreasing user experience was a
> > > good
> > > idea, I know, rather a rare use case what you have here, but still, why
> > > is okay
> > > to parse numbers from string with no error, thus giving a clear error
> > > (like your
> > > iothread example), but not okay for bitmaps, CC'ing Peter to give more
> > > information about that change.
> >
> > I have checked the commit b71946af5c7 before sending this patch and came up
> > with two solutions:
> >
> > - make cpuset="" passed the check, like this patch does
> > - add specified error report, rather than complaining about parsing bitmap
> >
> > The first method can keep consistence with the older version, so I chose
> > this one.
>
> Well, in this case being consistent with something which wasn't imho entirely
> correct is not the way to go, the consistency should have been with all the
> other cases where the parsing simply fails because it can't parse an empty
> string into anything meaningful. As I said, I don't see a point in using
> cpuset="" in real world. It would be different if the kind of change commit
> b71946af5c7 introduced made some VMs disappear, but that's not the case here,
> you just happen to use an old XML to create a transient VM, so I'd suggest
> updating the XML you use instead.
Ok, I agree with what you said. However, the error report now is not
user-friendly after
all, so should I send a new patch to improve the code, such like:
Re: [libvirt] [PATCH] conf: fix starting a domain with cpuset=""
On Wed, Sep 19, 2018 at 08:11:34AM +0800, wang.y...@zte.com.cn wrote:
> > On Mon, Sep 17, 2018 at 06:53:13PM +0800, wang.y...@zte.com.cn wrote:
> > > > On Sat, Sep 15, 2018 at 04:29:24PM +0800, Yi Wang wrote:
> > > > > Domain fails to start when its config xml including:
> > > > > 64
> > > > >
> > > > > # virsh create vm.xml
> > > > > error: Failed to create domain from vm.xml
> > > > > error: invalid argument: Failed to parse bitmap ''
> > > > >
> > > > > This patch fixes this.
> > > > >
> > > > > Signed-off-by: Yi Wang
> > > > > Reviewed-by: Xi Xu
> > > > > ---
> > > > > src/conf/domain_conf.c | 2 +-
> > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > > > > index 8619962..bacafb2 100644
> > > > > --- a/src/conf/domain_conf.c
> > > > > +++ b/src/conf/domain_conf.c
> > > > > @@ -18553,7 +18553,7 @@ virDomainVcpuParse(virDomainDefPtr def,
> > > > >
> > > > > if (def->placement_mode !=
> > > > > VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) {
> > > > > tmp = virXMLPropString(vcpuNode, "cpuset");
> > > > > -if (tmp) {
> > > > > +if (tmp && strlen(tmp) != 0) {
> > > >
> > > > '&& *tmp' would suffice.
> > >
> > > Ok.
> > >
> > > >
> > > > The patch is correct, but I believe there is a number of spots in the
> > > > massive
> > > > domain_conf.c file where a similar fix would be needed, it might be
> > > > worth
> > > > checking all the spots where no conversion like string-to-int
> > > > string-to-enum or
> > > > any other additional parsing like address parsing is performed, those
> > > > might be
> > > > good candidates.
> > > >
> > > > I understand the file is massive, so let me know how that goes.
> > >
> > > In most similar places we have already checked by the return values of
> > > function
> > > like string-to-int string-to-enum, and if failed, libvirt will give
> > > explicit report.
> > >
> > > To create a domain with iothread="" of disk, for instance:
> > >
> > > # virsh create vm.xml
> > > error: Failed to create domain from vm.xml
> > > error: XML error: Invalid iothread attribute in disk driver element:
> > >
> > > But the cpuset="" config error report may make people confuesd, and in
> > > the older
> > > version like 3.1.0 that's ok to start a domain with this config. So I
> > > think this patch
> > > is enough.
> >
> > So, I thought about this quite a bit. The fact that 3.1.0 was okay with that
> > was because commit b71946af5c7 changed the helper we use during parsing. The
> > original one considered NULL and foo[0] == '\0' to be the same and always
> > returned NULL. However, now that I looked at the code more closely, I don't
> > think we want this patch, in fact, an empty cpuset is an invalid attribute,
> > what's the point of having an empty cpuset vs not having it at all, if you
> > don't
> > want static pinning then don't specify one, there's no practical use case in
> > that. That being said, I also find the error very generic and vague, so if
> > anyone wants to actually figure out what the part causing problems in the
> > XML
> > parsing is, they have no chance, we should improve that. Paradoxically, we
> > had
> > that back in 2013 until commit 106a2ddaa7b changed it with absolutely no
> > rationale or a corresponding BZ on why decreasing user experience was a good
> > idea, I know, rather a rare use case what you have here, but still, why is
> > okay
> > to parse numbers from string with no error, thus giving a clear error (like
> > your
> > iothread example), but not okay for bitmaps, CC'ing Peter to give more
> > information about that change.
>
> I have checked the commit b71946af5c7 before sending this patch and came up
> with two solutions:
>
> - make cpuset="" passed the check, like this patch does
> - add specified error report, rather than complaining about parsing bitmap
>
> The first method can keep consistence with the older version, so I chose this
> one.
Well, in this case being consistent with something which wasn't imho entirely
correct is not the way to go, the consistency should have been with all the
other cases where the parsing simply fails because it can't parse an empty
string into anything meaningful. As I said, I don't see a point in using
cpuset="" in real world. It would be different if the kind of change commit
b71946af5c7 introduced made some VMs disappear, but that's not the case here,
you just happen to use an old XML to create a transient VM, so I'd suggest
updating the XML you use instead.
Erik
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
