- Devel - libvirt List Archives

[libvirt] [PATCH v2] systemd: Modernize machine naming
by Martin Kletzander 03 Feb '16

03 Feb '16

So, systemd-machined has this philosophy that machine names are like hostnames and hence should follow the same rules. But we always allowed international characters in domain names. Thus we need to modify the machine name we are passing to systemd. In order to change some machinenames that we will be passing to systemd, we also need to call TerminateMachine at the end of a lifetime of a domain. Even for domains that were started with older libvirt. That can be achieved thanks to virSystemdGetMachineNameByPID(). And because we can change machine names, we can get rid of the inconsistent and pointless escaping of domain names when creating machine names. So this patch modifies the naming in the following way. It creates the name as <drivername>-<id>-<name> where invalid hostname characters are stripped out of the name and if the resulting name is longer, it truncates it to 64 characters. That way we can start domains we couldn't start before. Well, at least on systemd. To make it work all together, the machineName (which is needed only for systemd) is saved in domain's private data. That way the generation is moved to the driver and we don't need to pass various unnecessary arguments to cgroup functions. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1282846 Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com> --- src/lxc/lxc_cgroup.c | 13 ++++++++-- src/lxc/lxc_domain.c | 1 + src/lxc/lxc_domain.h | 1 + src/lxc/lxc_process.c | 19 +++++++++----- src/qemu/qemu_cgroup.c | 27 ++++++++++++++------ src/qemu/qemu_cgroup.h | 2 +- src/qemu/qemu_domain.c | 1 + src/qemu/qemu_domain.h | 1 + src/qemu/qemu_process.c | 4 +-- src/util/vircgroup.c | 25 +++++++++--------- src/util/vircgroup.h | 12 ++++----- src/util/virsystemd.c | 67 ++++++++++++++++++++++++++++++++++--------------- src/util/virsystemd.h | 11 ++++---- tests/virsystemdtest.c | 45 +++++++++++++++++---------------- 14 files changed, 145 insertions(+), 84 deletions(-) diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c index ad254e4934fc..31489466cfbf 100644 --- a/src/lxc/lxc_cgroup.c +++ b/src/lxc/lxc_cgroup.c @@ -29,6 +29,7 @@ #include "viralloc.h" #include "vircgroup.h" #include "virstring.h" +#include "virsystemd.h" #define VIR_FROM_THIS VIR_FROM_LXC @@ -483,6 +484,13 @@ virCgroupPtr virLXCCgroupCreate(virDomainDefPtr def, int *nicindexes) { virCgroupPtr cgroup = NULL; + char *machineName = virSystemdMakeMachineName("lxc", + def->id, + def->name, + true); + + if (!machineName) + goto cleanup; if (def->resource->partition[0] != '/') { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -491,9 +499,8 @@ virCgroupPtr virLXCCgroupCreate(virDomainDefPtr def, goto cleanup; } - if (virCgroupNewMachine(def->name, + if (virCgroupNewMachine(machineName, "lxc", - true, def->uuid, NULL, initpid, @@ -517,6 +524,8 @@ virCgroupPtr virLXCCgroupCreate(virDomainDefPtr def, } cleanup: + VIR_FREE(machineName); + return cgroup; } diff --git a/src/lxc/lxc_domain.c b/src/lxc/lxc_domain.c index c3f7a564b36b..6732e934453f 100644 --- a/src/lxc/lxc_domain.c +++ b/src/lxc/lxc_domain.c @@ -28,6 +28,7 @@ #include "virerror.h" #include <libxml/xpathInternals.h> #include "virstring.h" +#include "vircgroup.h" #include "virutil.h" #include "virfile.h" diff --git a/src/lxc/lxc_domain.h b/src/lxc/lxc_domain.h index 2119c7899007..39c6e7def9ce 100644 --- a/src/lxc/lxc_domain.h +++ b/src/lxc/lxc_domain.h @@ -64,6 +64,7 @@ struct _virLXCDomainObjPrivate { pid_t initpid; virCgroupPtr cgroup; + char *machineName; }; extern virDomainXMLNamespace virLXCDriverDomainXMLNamespace; diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index f7e2b810b74b..b14b69da4a16 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -233,8 +233,7 @@ static void virLXCProcessCleanup(virLXCDriverPtr driver, * properly. See https://bugs.freedesktop.org/show_bug.cgi?id=68370 for * the bug we are working around here. */ - virSystemdTerminateMachine(vm->def->name, "lxc", true); - + virCgroupTerminateMachine(priv->machineName); /* The "release" hook cleans up additional resources */ if (virHookPresent(VIR_HOOK_DRIVER_LXC)) { @@ -1494,8 +1493,8 @@ int virLXCProcessStart(virConnectPtr conn, * point so lets detect that first, since it gives us a * more reliable way to kill everything off if something * goes wrong from here onwards ... */ - if (virCgroupNewDetectMachine(vm->def->name, "lxc", vm->pid, - -1, &priv->cgroup) < 0) + if (virCgroupNewDetectMachine(vm->def->name, vm->def->uuid, "lxc", + vm->pid, -1, &priv->cgroup) < 0) goto cleanup; if (!priv->cgroup) { @@ -1505,6 +1504,11 @@ int virLXCProcessStart(virConnectPtr conn, goto cleanup; } + /* Get the machine name so we can properly delete it through + * systemd later */ + if (!(priv->machineName = virSystemdGetMachineNameByPID(vm->pid))) + virResetLastError(); + /* And we can get the first monitor connection now too */ if (!(priv->monitor = virLXCProcessConnectMonitor(driver, vm))) { /* Intentionally overwrite the real monitor error message, @@ -1677,8 +1681,8 @@ virLXCProcessReconnectDomain(virDomainObjPtr vm, if (!(priv->monitor = virLXCProcessConnectMonitor(driver, vm))) goto error; - if (virCgroupNewDetectMachine(vm->def->name, "lxc", vm->pid, - -1, &priv->cgroup) < 0) + if (virCgroupNewDetectMachine(vm->def->name, vm->def->uuid, "lxc", + vm->pid, -1, &priv->cgroup) < 0) goto error; if (!priv->cgroup) { @@ -1688,6 +1692,9 @@ virLXCProcessReconnectDomain(virDomainObjPtr vm, goto error; } + if (!(priv->machineName = virSystemdGetMachineNameByPID(vm->pid))) + virResetLastError(); + if (virLXCUpdateActiveUSBHostdevs(driver, vm->def) < 0) goto error; diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index e41f4617c455..705594d93dc5 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -36,6 +36,7 @@ #include "virfile.h" #include "virtypedparam.h" #include "virnuma.h" +#include "virsystemd.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -772,9 +773,19 @@ qemuInitCgroup(virQEMUDriverPtr driver, goto cleanup; } - if (virCgroupNewMachine(vm->def->name, + /* + * We need to do this because of systemd-machined, because + * CreateMachine requires the name to be a valid hostname. + */ + priv->machineName = virSystemdMakeMachineName("qemu", + vm->def->id, + vm->def->name, + virQEMUDriverIsPrivileged(driver)); + if (!priv->machineName) + goto cleanup; + + if (virCgroupNewMachine(priv->machineName, "qemu", - true, vm->def->uuid, NULL, vm->pid, @@ -887,12 +898,17 @@ qemuConnectCgroup(virQEMUDriverPtr driver, virCgroupFree(&priv->cgroup); if (virCgroupNewDetectMachine(vm->def->name, + vm->def->uuid, "qemu", vm->pid, cfg->cgroupControllers, &priv->cgroup) < 0) goto cleanup; + priv->machineName = virSystemdGetMachineNameByPID(vm->pid); + if (!priv->machineName) + virResetLastError(); + qemuRestoreCgroupState(vm); done: @@ -1264,17 +1280,14 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm) } int -qemuRemoveCgroup(virQEMUDriverPtr driver, - virDomainObjPtr vm) +qemuRemoveCgroup(virDomainObjPtr vm) { qemuDomainObjPrivatePtr priv = vm->privateData; if (priv->cgroup == NULL) return 0; /* Not supported, so claim success */ - if (virCgroupTerminateMachine(vm->def->name, - "qemu", - virQEMUDriverIsPrivileged(driver)) < 0) { + if (virCgroupTerminateMachine(priv->machineName) < 0) { if (!virCgroupNewIgnoreError()) VIR_DEBUG("Failed to terminate cgroup for %s", vm->def->name); } diff --git a/src/qemu/qemu_cgroup.h b/src/qemu/qemu_cgroup.h index 2bcf071d3792..347d126f7394 100644 --- a/src/qemu/qemu_cgroup.h +++ b/src/qemu/qemu_cgroup.h @@ -56,7 +56,7 @@ int qemuSetupCgroupCpusetCpus(virCgroupPtr cgroup, virBitmapPtr cpumask); int qemuSetupCgroupForVcpu(virDomainObjPtr vm); int qemuSetupCgroupForIOThreads(virDomainObjPtr vm); int qemuSetupCgroupForEmulator(virDomainObjPtr vm); -int qemuRemoveCgroup(virQEMUDriverPtr driver, virDomainObjPtr vm); +int qemuRemoveCgroup(virDomainObjPtr vm); int qemuAddToCgroup(virDomainObjPtr vm); #endif /* __QEMU_CGROUP_H__ */ diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 189552045177..09ad2f75916f 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -39,6 +39,7 @@ #include "virtime.h" #include "virstoragefile.h" #include "virstring.h" +#include "virsystemd.h" #include "virthreadjob.h" #include "viratomic.h" #include "virprocess.h" diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 6a8cf705d8af..ab798faf0fb0 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -203,6 +203,7 @@ struct _qemuDomainObjPrivate { bool signalIOError; /* true if the domain condition should be signalled on I/O error */ + char *machineName; }; # define QEMU_DOMAIN_DISK_PRIVATE(disk) \ diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 7b09ba73896b..b3f8c93d1a93 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4610,7 +4610,7 @@ qemuProcessLaunch(virConnectPtr conn, /* Ensure no historical cgroup for this VM is lying around bogus * settings */ VIR_DEBUG("Ensuring no historical cgroup is lying around"); - qemuRemoveCgroup(driver, vm); + qemuRemoveCgroup(vm); VIR_DEBUG("Setting up ports for graphics"); if (qemuProcessSetupGraphics(driver, vm) < 0) @@ -5405,7 +5405,7 @@ void qemuProcessStop(virQEMUDriverPtr driver, } retry: - if ((ret = qemuRemoveCgroup(driver, vm)) < 0) { + if ((ret = qemuRemoveCgroup(vm)) < 0) { if (ret == -EBUSY && (retries++ < 5)) { usleep(200*1000); goto retry; diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index da0df7ae7a5e..4aed5d20b818 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -43,6 +43,7 @@ #include "vircgrouppriv.h" #include "virutil.h" +#include "viruuid.h" #include "viralloc.h" #include "virerror.h" #include "virlog.h" @@ -243,12 +244,14 @@ static bool virCgroupValidateMachineGroup(virCgroupPtr group, const char *name, const char *drivername, + const unsigned char *uuid, bool stripEmulatorSuffix) { size_t i; bool valid = false; char *partname; char *scopename; + char uuidstr[VIR_UUID_STRING_BUFLEN]; if (virAsprintf(&partname, "%s.libvirt-%s", name, drivername) < 0) @@ -263,6 +266,8 @@ virCgroupValidateMachineGroup(virCgroupPtr group, if (virCgroupPartitionEscape(&scopename) < 0) goto cleanup; + virUUIDFormat(uuid, uuidstr); + for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) { char *tmp; @@ -290,6 +295,7 @@ virCgroupValidateMachineGroup(virCgroupPtr group, tmp++; if (STRNEQ(tmp, name) && + STRNEQ(tmp, uuidstr) && STRNEQ(tmp, partname) && STRNEQ(tmp, scopename)) { VIR_DEBUG("Name '%s' for controller '%s' does not match " @@ -1554,6 +1560,7 @@ virCgroupNewDetect(pid_t pid, */ int virCgroupNewDetectMachine(const char *name, + const unsigned char *uuid, const char *drivername, pid_t pid, int controllers, @@ -1565,7 +1572,7 @@ virCgroupNewDetectMachine(const char *name, return -1; } - if (!virCgroupValidateMachineGroup(*group, name, drivername, true)) { + if (!virCgroupValidateMachineGroup(*group, name, drivername, uuid, true)) { VIR_DEBUG("Failed to validate machine name for '%s' driver '%s'", name, drivername); virCgroupFree(group); @@ -1582,7 +1589,6 @@ virCgroupNewDetectMachine(const char *name, static int virCgroupNewMachineSystemd(const char *name, const char *drivername, - bool privileged, const unsigned char *uuid, const char *rootdir, pid_t pidleader, @@ -1602,7 +1608,6 @@ virCgroupNewMachineSystemd(const char *name, VIR_DEBUG("Trying to setup machine '%s' via systemd", name); if ((rv = virSystemdCreateMachine(name, drivername, - privileged, uuid, rootdir, pidleader, @@ -1690,11 +1695,9 @@ virCgroupNewMachineSystemd(const char *name, /* * Returns 0 on success, -1 on fatal error */ -int virCgroupTerminateMachine(const char *name, - const char *drivername, - bool privileged) +int virCgroupTerminateMachine(const char *name) { - return virSystemdTerminateMachine(name, drivername, privileged); + return virSystemdTerminateMachine(name); } @@ -1749,7 +1752,6 @@ virCgroupNewMachineManual(const char *name, int virCgroupNewMachine(const char *name, const char *drivername, - bool privileged, const unsigned char *uuid, const char *rootdir, pid_t pidleader, @@ -1766,7 +1768,6 @@ virCgroupNewMachine(const char *name, if ((rv = virCgroupNewMachineSystemd(name, drivername, - privileged, uuid, rootdir, pidleader, @@ -4240,6 +4241,7 @@ virCgroupNewDetect(pid_t pid ATTRIBUTE_UNUSED, int virCgroupNewDetectMachine(const char *name ATTRIBUTE_UNUSED, + const unsigned char *uuid ATTRIBUTE_UNUSED, const char *drivername ATTRIBUTE_UNUSED, pid_t pid ATTRIBUTE_UNUSED, int controllers ATTRIBUTE_UNUSED, @@ -4251,9 +4253,7 @@ virCgroupNewDetectMachine(const char *name ATTRIBUTE_UNUSED, } -int virCgroupTerminateMachine(const char *name ATTRIBUTE_UNUSED, - const char *drivername ATTRIBUTE_UNUSED, - bool privileged ATTRIBUTE_UNUSED) +int virCgroupTerminateMachine(const char *name ATTRIBUTE_UNUSED) { virReportSystemError(ENXIO, "%s", _("Control groups not supported on this platform")); @@ -4264,7 +4264,6 @@ int virCgroupTerminateMachine(const char *name ATTRIBUTE_UNUSED, int virCgroupNewMachine(const char *name ATTRIBUTE_UNUSED, const char *drivername ATTRIBUTE_UNUSED, - bool privileged ATTRIBUTE_UNUSED, const unsigned char *uuid ATTRIBUTE_UNUSED, const char *rootdir ATTRIBUTE_UNUSED, pid_t pidleader ATTRIBUTE_UNUSED, diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h index d754b1f3bd7a..dd4cb719f1f1 100644 --- a/src/util/vircgroup.h +++ b/src/util/vircgroup.h @@ -90,14 +90,16 @@ int virCgroupNewDetect(pid_t pid, virCgroupPtr *group); int virCgroupNewDetectMachine(const char *name, + const unsigned char *uuid, const char *drivername, pid_t pid, int controllers, - virCgroupPtr *group); + virCgroupPtr *group) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) + ATTRIBUTE_NONNULL(3); int virCgroupNewMachine(const char *name, const char *drivername, - bool privileged, const unsigned char *uuid, const char *rootdir, pid_t pidleader, @@ -110,10 +112,8 @@ int virCgroupNewMachine(const char *name, ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4); -int virCgroupTerminateMachine(const char *name, - const char *drivername, - bool privileged) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +int virCgroupTerminateMachine(const char *name) + ATTRIBUTE_NONNULL(1); bool virCgroupNewIgnoreError(void); diff --git a/src/util/virsystemd.c b/src/util/virsystemd.c index 9e85e24b2c4b..5c6cd872bee0 100644 --- a/src/util/virsystemd.c +++ b/src/util/virsystemd.c @@ -25,6 +25,8 @@ # include <systemd/sd-daemon.h> #endif +#include "internal.h" + #include "virsystemd.h" #include "viratomic.h" #include "virdbus.h" @@ -33,6 +35,7 @@ #include "virutil.h" #include "virlog.h" #include "virerror.h" +#include "viruuid.h" #define VIR_FROM_THIS VIR_FROM_SYSTEMD @@ -113,10 +116,42 @@ char *virSystemdMakeSliceName(const char *partition) return virBufferContentAndReset(&buf); } +#define HOSTNAME_CHARS \ + "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-" -char *virSystemdMakeMachineName(const char *name, - const char *drivername, - bool privileged) +static void +virSystemdAppendValidMachineName(virBufferPtr buf, + const char *name) +{ + bool skip_dot = false; + + for (; *name; name++) { + if (strlen(virBufferCurrentContent(buf)) >= 64) + break; + + if (*name == '.') { + if (!skip_dot) + virBufferAddChar(buf, *name); + skip_dot = true; + continue; + } + + skip_dot = false; + + if (!strchr(HOSTNAME_CHARS, *name)) + continue; + + virBufferAddChar(buf, *name); + } +} + +#undef HOSTNAME_CHARS + +char * +virSystemdMakeMachineName(const char *drivername, + int id, + const char *name, + bool privileged) { char *machinename = NULL; char *username = NULL; @@ -131,7 +166,8 @@ char *virSystemdMakeMachineName(const char *name, virBufferAsprintf(&buf, "%s-%s-", username, drivername); } - virSystemdEscapeName(&buf, name); + virBufferAsprintf(&buf, "%d-", id); + virSystemdAppendValidMachineName(&buf, name); machinename = virBufferContentAndReset(&buf); cleanup: @@ -212,7 +248,6 @@ virSystemdGetMachineNameByPID(pid_t pid) */ int virSystemdCreateMachine(const char *name, const char *drivername, - bool privileged, const unsigned char *uuid, const char *rootdir, pid_t pidleader, @@ -223,7 +258,6 @@ int virSystemdCreateMachine(const char *name, { int ret; DBusConnection *conn; - char *machinename = NULL; char *creatorname = NULL; char *slicename = NULL; static int hasCreateWithNetwork = 1; @@ -239,8 +273,6 @@ int virSystemdCreateMachine(const char *name, return -1; ret = -1; - if (!(machinename = virSystemdMakeMachineName(name, drivername, privileged))) - goto cleanup; if (virAsprintf(&creatorname, "libvirt-%s", drivername) < 0) goto cleanup; @@ -318,7 +350,7 @@ int virSystemdCreateMachine(const char *name, "org.freedesktop.machine1.Manager", "CreateMachineWithNetwork", "sayssusa&ia(sv)", - machinename, + name, 16, uuid[0], uuid[1], uuid[2], uuid[3], uuid[4], uuid[5], uuid[6], uuid[7], @@ -360,7 +392,7 @@ int virSystemdCreateMachine(const char *name, "org.freedesktop.machine1.Manager", "CreateMachine", "sayssusa(sv)", - machinename, + name, 16, uuid[0], uuid[1], uuid[2], uuid[3], uuid[4], uuid[5], uuid[6], uuid[7], @@ -381,20 +413,19 @@ int virSystemdCreateMachine(const char *name, cleanup: VIR_FREE(creatorname); - VIR_FREE(machinename); VIR_FREE(slicename); return ret; } -int virSystemdTerminateMachine(const char *name, - const char *drivername, - bool privileged) +int virSystemdTerminateMachine(const char *name) { int ret; DBusConnection *conn; - char *machinename = NULL; virError error; + if (!name) + return 0; + memset(&error, 0, sizeof(error)); ret = virDBusIsServiceEnabled("org.freedesktop.machine1"); @@ -409,9 +440,6 @@ int virSystemdTerminateMachine(const char *name, if (!(conn = virDBusGetSystemBus())) goto cleanup; - if (!(machinename = virSystemdMakeMachineName(name, drivername, privileged))) - goto cleanup; - /* * The systemd DBus API we're invoking has the * following signature @@ -431,7 +459,7 @@ int virSystemdTerminateMachine(const char *name, "org.freedesktop.machine1.Manager", "TerminateMachine", "s", - machinename) < 0) + name) < 0) goto cleanup; if (error.code == VIR_ERR_ERROR && @@ -446,7 +474,6 @@ int virSystemdTerminateMachine(const char *name, cleanup: virResetError(&error); - VIR_FREE(machinename); return ret; } diff --git a/src/util/virsystemd.h b/src/util/virsystemd.h index a13a4c0c48be..9042b5f31263 100644 --- a/src/util/virsystemd.h +++ b/src/util/virsystemd.h @@ -23,18 +23,19 @@ # define __VIR_SYSTEMD_H__ # include "internal.h" +# include "virbuffer.h" char *virSystemdMakeScopeName(const char *name, const char *drivername); char *virSystemdMakeSliceName(const char *partition); -char *virSystemdMakeMachineName(const char *name, - const char *drivername, +char *virSystemdMakeMachineName(const char *drivername, + int id, + const char *name, bool privileged); int virSystemdCreateMachine(const char *name, const char *drivername, - bool privileged, const unsigned char *uuid, const char *rootdir, pid_t pidleader, @@ -43,9 +44,7 @@ int virSystemdCreateMachine(const char *name, int *nicindexes, const char *partition); -int virSystemdTerminateMachine(const char *name, - const char *drivername, - bool privileged); +int virSystemdTerminateMachine(const char *name); void virSystemdNotifyStartup(void); diff --git a/tests/virsystemdtest.c b/tests/virsystemdtest.c index 3a3cd9968032..d7fccf92bc24 100644 --- a/tests/virsystemdtest.c +++ b/tests/virsystemdtest.c @@ -166,7 +166,6 @@ static int testCreateContainer(const void *opaque ATTRIBUTE_UNUSED) }; if (virSystemdCreateMachine("demo", "lxc", - true, uuid, "/proc/123/root", 123, @@ -182,9 +181,7 @@ static int testCreateContainer(const void *opaque ATTRIBUTE_UNUSED) static int testTerminateContainer(const void *opaque ATTRIBUTE_UNUSED) { - if (virSystemdTerminateMachine("demo", - "lxc", - true) < 0) { + if (virSystemdTerminateMachine("lxc-demo") < 0) { fprintf(stderr, "%s", "Failed to terminate LXC machine\n"); return -1; } @@ -202,7 +199,6 @@ static int testCreateMachine(const void *opaque ATTRIBUTE_UNUSED) }; if (virSystemdCreateMachine("demo", "qemu", - false, uuid, NULL, 123, @@ -218,9 +214,7 @@ static int testCreateMachine(const void *opaque ATTRIBUTE_UNUSED) static int testTerminateMachine(const void *opaque ATTRIBUTE_UNUSED) { - if (virSystemdTerminateMachine("demo", - "qemu", - false) < 0) { + if (virSystemdTerminateMachine("test-qemu-demo") < 0) { fprintf(stderr, "%s", "Failed to terminate KVM machine\n"); return -1; } @@ -242,7 +236,6 @@ static int testCreateNoSystemd(const void *opaque ATTRIBUTE_UNUSED) if ((rv = virSystemdCreateMachine("demo", "qemu", - true, uuid, NULL, 123, @@ -277,7 +270,6 @@ static int testCreateSystemdNotRunning(const void *opaque ATTRIBUTE_UNUSED) if ((rv = virSystemdCreateMachine("demo", "qemu", - true, uuid, NULL, 123, @@ -312,7 +304,6 @@ static int testCreateBadSystemd(const void *opaque ATTRIBUTE_UNUSED) if ((rv = virSystemdCreateMachine("demo", "qemu", - true, uuid, NULL, 123, @@ -348,7 +339,6 @@ static int testCreateNetwork(const void *opaque ATTRIBUTE_UNUSED) size_t nnicindexes = ARRAY_CARDINALITY(nicindexes); if (virSystemdCreateMachine("demo", "lxc", - true, uuid, "/proc/123/root", 123, @@ -385,6 +375,7 @@ testGetMachineName(const void *opaque ATTRIBUTE_UNUSED) struct testNameData { const char *name; const char *expected; + int id; }; static int @@ -417,7 +408,8 @@ testMachineName(const void *opaque) int ret = -1; char *actual = NULL; - if (!(actual = virSystemdMakeMachineName(data->name, "qemu", true))) + if (!(actual = virSystemdMakeMachineName("qemu", data->id, + data->name, true))) goto cleanup; if (STRNEQ(actual, data->expected)) { @@ -518,6 +510,12 @@ mymain(void) { int ret = 0; + unsigned char uuid[VIR_UUID_BUFLEN]; + + /* The one we use in tests quite often */ + if (virUUIDParse("c7a5fdbd-edaf-9455-926a-d65c16db1809", uuid) < 0) + return EXIT_FAILURE; + if (virtTestRun("Test create container ", testCreateContainer, NULL) < 0) ret = -1; if (virtTestRun("Test terminate container ", testTerminateContainer, NULL) < 0) @@ -541,7 +539,7 @@ mymain(void) # define TEST_SCOPE(name, unitname) \ do { \ struct testNameData data = { \ - name, unitname \ + name, unitname, 0, \ }; \ if (virtTestRun("Test scopename", testScopeName, &data) < 0) \ ret = -1; \ @@ -553,20 +551,25 @@ mymain(void) TEST_SCOPE(".demo", "machine-lxc\\x2d\\x2edemo.scope"); TEST_SCOPE("bull💩", "machine-lxc\\x2dbull\\xf0\\x9f\\x92\\xa9.scope"); -# define TEST_MACHINE(name, machinename) \ +# define TEST_MACHINE(name, id, machinename) \ do { \ struct testNameData data = { \ - name, machinename \ + name, machinename, id, \ }; \ if (virtTestRun("Test scopename", testMachineName, &data) < 0) \ ret = -1; \ } while (0) - TEST_MACHINE("demo", "qemu-demo"); - TEST_MACHINE("demo-name", "qemu-demo\\x2dname"); - TEST_MACHINE("demo!name", "qemu-demo\\x21name"); - TEST_MACHINE(".demo", "qemu-\\x2edemo"); - TEST_MACHINE("bull\U0001f4a9", "qemu-bull\\xf0\\x9f\\x92\\xa9"); + TEST_MACHINE("demo", 1, "qemu-1-demo"); + TEST_MACHINE("demo-name", 2, "qemu-2-demo-name"); + TEST_MACHINE("demo!name", 3, "qemu-3-demoname"); + TEST_MACHINE(".demo", 4, "qemu-4-.demo"); + TEST_MACHINE("bull\U0001f4a9", 5, "qemu-5-bull"); + TEST_MACHINE("demo..name", 6, "qemu-6-demo.name"); + TEST_MACHINE("12345678901234567890123456789012345678901234567890123456789", 7, + "qemu-7-123456789012345678901234567890123456789012345678901234567"); + TEST_MACHINE("123456789012345678901234567890123456789012345678901234567890", 8, + "qemu-8-123456789012345678901234567890123456789012345678901234567"); # define TESTS_PM_SUPPORT_HELPER(name, function) \ do { \ -- 2.7.0

2 2

[libvirt] [PATCH v3] qemu: qemuDomainRename and virDomainObjListNumOfDomains ABBA deadlock fix
by Nikolay Shirokovskiy 03 Feb '16

03 Feb '16

A pretty nasty deadlock occurs while trying to rename a VM in parallel with virDomainObjListNumOfDomains. The short description of the problem is as follows: Thread #1: qemuDomainRename: ------> aquires domain lock by qemuDomObjFromDomain ---------> waits for domain list lock in any of the listed functions: - virDomainObjListFindByName - virDomainObjListRenameAddNew - virDomainObjListRenameRemove Thread #2: virDomainObjListNumOfDomains: ------> aquires domain list lock ---------> waits for domain lock in virDomainObjListCount Introduce generic virDomainObjListRename function for renaming domains. It aquires list lock in right order to avoid deadlock. Callback is used to make driver specific domain updates. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com> --- src/conf/virdomainobjlist.c | 98 +++++++++++++++++++----------- src/conf/virdomainobjlist.h | 16 +++-- src/libvirt_private.syms | 3 +- src/qemu/qemu_driver.c | 142 ++++++++++++++++++++------------------------ 4 files changed, 142 insertions(+), 117 deletions(-) diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index 7568f93..1f7ddaf 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -313,40 +313,6 @@ virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr doms, } -int -virDomainObjListRenameAddNew(virDomainObjListPtr doms, - virDomainObjPtr vm, - const char *name) -{ - int ret = -1; - virObjectLock(doms); - - /* Add new name into the hash table of domain names. */ - if (virHashAddEntry(doms->objsName, name, vm) < 0) - goto cleanup; - - /* Okay, this is crazy. virHashAddEntry() does not increment - * the refcounter of @vm, but virHashRemoveEntry() does - * decrement it. We need to work around it. */ - virObjectRef(vm); - - ret = 0; - cleanup: - virObjectUnlock(doms); - return ret; -} - - -int -virDomainObjListRenameRemove(virDomainObjListPtr doms, const char *name) -{ - virObjectLock(doms); - virHashRemoveEntry(doms->objsName, name); - virObjectUnlock(doms); - return 0; -} - - /* * The caller must hold a lock on the driver owning 'doms', * and must also have locked 'dom', to ensure no one else @@ -372,6 +338,70 @@ void virDomainObjListRemove(virDomainObjListPtr doms, } +/* + * The caller must hold a lock on dom. Callbacks should not sleep/wait othewise + * operations on all domains will be blocked as the callback is called with + * domains lock hold. Domain lock is dropped/reaquired during this operation + * thus domain consistency must not rely on this lock solely. + */ + +int +virDomainObjListRename(virDomainObjListPtr doms, + virDomainObjPtr dom, + const char *new_name, + unsigned int flags, + virDomainObjListRenameCallback callback, + void *opaque) +{ + int ret = -1; + char *old_name = NULL; + int rc; + + /* doms and dom locks must be attained in right order thus relock dom. */ + /* dom reference is touched for the benefit of those callers that + * hold a lock on dom but not refcount it. */ + virObjectRef(dom); + virObjectUnlock(dom); + virObjectLock(doms); + virObjectLock(dom); + virObjectUnref(dom); + + if (STREQ(dom->def->name, new_name)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("Can't rename domain to itself")); + goto cleanup; + } + + if (virHashLookup(doms->objsName, new_name) != NULL) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("domain with name '%s' already exists"), + new_name); + goto cleanup; + } + + if (VIR_STRDUP(old_name, dom->def->name) < 0) + goto cleanup; + + if (virHashAddEntry(doms->objsName, new_name, dom) < 0) + goto cleanup; + + /* Okay, this is crazy. virHashAddEntry() does not increment + * the refcounter of @dom, but virHashRemoveEntry() does + * decrement it. We need to work around it. */ + virObjectRef(dom); + + rc = callback(dom, new_name, flags, opaque); + virHashRemoveEntry(doms->objsName, rc < 0 ? new_name : old_name); + if (rc < 0) + goto cleanup; + + ret = 0; + cleanup: + virObjectUnlock(doms); + VIR_FREE(old_name); + return ret; +} + /* The caller must hold lock on 'doms' in addition to 'virDomainObjListRemove' * requirements * diff --git a/src/conf/virdomainobjlist.h b/src/conf/virdomainobjlist.h index f479598..c0f856c 100644 --- a/src/conf/virdomainobjlist.h +++ b/src/conf/virdomainobjlist.h @@ -51,11 +51,17 @@ virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr doms, unsigned int flags, virDomainDefPtr *oldDef); -int virDomainObjListRenameAddNew(virDomainObjListPtr doms, - virDomainObjPtr vm, - const char *name); -int virDomainObjListRenameRemove(virDomainObjListPtr doms, - const char *name); +typedef int (*virDomainObjListRenameCallback)(virDomainObjPtr dom, + const char *new_name, + unsigned int flags, + void *opaque); +int virDomainObjListRename(virDomainObjListPtr doms, + virDomainObjPtr dom, + const char *new_name, + unsigned int flags, + virDomainObjListRenameCallback callback, + void *opaque); + void virDomainObjListRemove(virDomainObjListPtr doms, virDomainObjPtr dom); void virDomainObjListRemoveLocked(virDomainObjListPtr doms, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3d0ec9d..3ef3468 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -893,8 +893,7 @@ virDomainObjListNew; virDomainObjListNumOfDomains; virDomainObjListRemove; virDomainObjListRemoveLocked; -virDomainObjListRenameAddNew; -virDomainObjListRenameRemove; +virDomainObjListRename; # cpu/cpu.h diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index abcdbe6..c0e750d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19976,14 +19976,14 @@ qemuDomainSetUserPassword(virDomainPtr dom, } -static int qemuDomainRename(virDomainPtr dom, - const char *new_name, - unsigned int flags) +static int +qemuDomainRenameCallback(virDomainObjPtr vm, + const char *new_name, + unsigned int flags, + void *opaque) { - virQEMUDriverPtr driver = dom->conn->privateData; + virQEMUDriverPtr driver = opaque; virQEMUDriverConfigPtr cfg = NULL; - virDomainObjPtr vm = NULL; - virDomainObjPtr tmp_dom = NULL; virObjectEventPtr event_new = NULL; virObjectEventPtr event_old = NULL; int ret = -1; @@ -19993,73 +19993,16 @@ static int qemuDomainRename(virDomainPtr dom, virCheckFlags(0, ret); - if (!(vm = qemuDomObjFromDomain(dom))) - goto cleanup; - - if (virDomainRenameEnsureACL(dom->conn, vm->def) < 0) - goto cleanup; - cfg = virQEMUDriverGetConfig(driver); - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) - goto cleanup; - - if (virDomainObjIsActive(vm)) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot rename active domain")); - goto endjob; - } - - if (!vm->persistent) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot rename a transient domain")); - goto endjob; - } - - if (virDomainObjGetState(vm, NULL) != VIR_DOMAIN_SHUTOFF) { - virReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("domain has to be shutoff before renaming")); - goto endjob; - } - - if (virDomainSnapshotObjListNum(vm->snapshots, NULL, 0) > 0) { - virReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("cannot rename domain with snapshots")); - goto endjob; - } - - if (STREQ(vm->def->name, new_name)) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("Can't rename domain to itself")); - goto endjob; - } - - /* - * This is a rather racy check, but still better than reporting - * internal error. And since new_name != name here, there's no - * deadlock imminent. - */ - tmp_dom = virDomainObjListFindByName(driver->domains, new_name); - if (tmp_dom) { - virObjectUnlock(tmp_dom); - virObjectUnref(tmp_dom); - virReportError(VIR_ERR_OPERATION_INVALID, - _("domain with name '%s' already exists"), - new_name); - goto endjob; - } - if (VIR_STRDUP(new_dom_name, new_name) < 0) - goto endjob; + goto cleanup; if (!(old_dom_cfg_file = virDomainConfigFile(cfg->configDir, vm->def->name))) { - goto endjob; + goto cleanup; } - if (virDomainObjListRenameAddNew(driver->domains, vm, new_name) < 0) - goto endjob; - event_old = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_UNDEFINED, VIR_DOMAIN_EVENT_UNDEFINED_RENAMED); @@ -20080,21 +20023,12 @@ static int qemuDomainRename(virDomainPtr dom, goto rollback; } - /* Remove old domain name from table. */ - virDomainObjListRenameRemove(driver->domains, old_dom_name); - event_new = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_DEFINED, VIR_DOMAIN_EVENT_DEFINED_RENAMED); - - /* Success, domain has been renamed. */ ret = 0; - endjob: - qemuDomainObjEndJob(driver, vm); - cleanup: - virDomainObjEndAPI(&vm); VIR_FREE(old_dom_cfg_file); VIR_FREE(old_dom_name); VIR_FREE(new_dom_name); @@ -20109,9 +20043,65 @@ static int qemuDomainRename(virDomainPtr dom, vm->def->name = old_dom_name; old_dom_name = NULL; } + goto cleanup; +} - virDomainObjListRenameRemove(driver->domains, new_name); - goto endjob; +static int qemuDomainRename(virDomainPtr dom, + const char *new_name, + unsigned int flags) +{ + virQEMUDriverPtr driver = dom->conn->privateData; + virDomainObjPtr vm = NULL; + int ret = -1; + + virCheckFlags(0, ret); + + if (!(vm = qemuDomObjFromDomain(dom))) + goto cleanup; + + if (virDomainRenameEnsureACL(dom->conn, vm->def) < 0) + goto cleanup; + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + goto cleanup; + + if (virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("cannot rename active domain")); + goto endjob; + } + + if (!vm->persistent) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("cannot rename a transient domain")); + goto endjob; + } + + if (virDomainObjGetState(vm, NULL) != VIR_DOMAIN_SHUTOFF) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain has to be shutoff before renaming")); + goto endjob; + } + + if (virDomainSnapshotObjListNum(vm->snapshots, NULL, 0) > 0) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("cannot rename domain with snapshots")); + goto endjob; + } + + if (virDomainObjListRename(driver->domains, vm, new_name, flags, + qemuDomainRenameCallback, driver) < 0) + goto endjob; + + /* Success, domain has been renamed. */ + ret = 0; + + endjob: + qemuDomainObjEndJob(driver, vm); + + cleanup: + virDomainObjEndAPI(&vm); + return ret; } static virHypervisorDriver qemuHypervisorDriver = { -- 1.8.3.1

2 2

[libvirt] [PATCH 0/5] Export remote typed params methods internally via util
by Erik Skultety 03 Feb '16

03 Feb '16

Erik Skultety (5): cfg.mk: Adjust sc_prohibit_int_ijk to support 'exempt from syntax-check' util: Introduce virTypedParameterRemote datatype util: Export remoteDeserializeTypedParameters internally via util util: Export remoteFreeTypedParameters internally via util util: Export remoteSerializeTypedParameters internally via util cfg.mk | 3 +- daemon/remote.c | 218 ++++++--------------------------------- src/libvirt_private.syms | 3 + src/remote/remote_driver.c | 196 +++--------------------------------- src/rpc/gendispatch.pl | 9 +- src/util/virtypedparam.c | 246 +++++++++++++++++++++++++++++++++++++++++++++ src/util/virtypedparam.h | 40 ++++++++ 7 files changed, 341 insertions(+), 374 deletions(-) -- 2.4.3

2 9

[libvirt] [PATCH v2] security_selinux: Fix crash in virSecuritySELinuxRestoreFileLabel
by Shanzhi Yu 03 Feb '16

03 Feb '16

virSecuritySELinuxRestoreFileLabel should never be called with NULL path add check before call this function in case of causeing libvirtd crash https://bugzilla.redhat.com/show_bug.cgi?id=1300532 Signed-off-by: Shanzhi Yu <shyu(a)redhat.com> --- src/security/security_selinux.c | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 9e98635..77e55a3 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1098,7 +1098,8 @@ virSecuritySELinuxRestoreInputLabel(virSecurityManagerPtr mgr, switch ((virDomainInputType) input->type) { case VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH: - rc = virSecuritySELinuxRestoreFileLabel(mgr, input->source.evdev); + if (input->source.evdev) + rc = virSecuritySELinuxRestoreFileLabel(mgr, input->source.evdev); break; case VIR_DOMAIN_INPUT_TYPE_MOUSE: @@ -1171,7 +1172,9 @@ virSecuritySELinuxRestoreTPMFileLabelInt(virSecurityManagerPtr mgr, switch (tpm->type) { case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: tpmdev = tpm->data.passthrough.source.data.file.path; - rc = virSecuritySELinuxRestoreFileLabel(mgr, tpmdev); + + if (tpmdev) + rc = virSecuritySELinuxRestoreFileLabel(mgr, tpmdev); if ((cancel_path = virTPMCreateCancelPath(tpmdev)) != NULL) { if (virSecuritySELinuxRestoreFileLabel(mgr, cancel_path) < 0) @@ -1722,7 +1725,9 @@ virSecuritySELinuxRestoreHostdevCapsLabel(virSecurityManagerPtr mgr, if (VIR_STRDUP(path, dev->source.caps.u.storage.block) < 0) return -1; } - ret = virSecuritySELinuxRestoreFileLabel(mgr, path); + if (path) + ret = virSecuritySELinuxRestoreFileLabel(mgr, path); + VIR_FREE(path); break; } @@ -1736,7 +1741,8 @@ virSecuritySELinuxRestoreHostdevCapsLabel(virSecurityManagerPtr mgr, if (VIR_STRDUP(path, dev->source.caps.u.misc.chardev) < 0) return -1; } - ret = virSecuritySELinuxRestoreFileLabel(mgr, path); + if (path) + ret = virSecuritySELinuxRestoreFileLabel(mgr, path); VIR_FREE(path); break; } @@ -1876,13 +1882,15 @@ virSecuritySELinuxRestoreChardevLabel(virSecurityManagerPtr mgr, switch (dev_source->type) { case VIR_DOMAIN_CHR_TYPE_DEV: case VIR_DOMAIN_CHR_TYPE_FILE: - if (virSecuritySELinuxRestoreFileLabel(mgr, dev_source->data.file.path) < 0) - goto done; + if (dev_source->data.file.path) { + if (virSecuritySELinuxRestoreFileLabel(mgr, dev_source->data.file.path) < 0) + goto done; + } ret = 0; break; case VIR_DOMAIN_CHR_TYPE_UNIX: - if (!dev_source->data.nix.listen) { + if (!dev_source->data.nix.listen && dev_source->data.file.path) { if (virSecuritySELinuxRestoreFileLabel(mgr, dev_source->data.file.path) < 0) goto done; } @@ -1898,7 +1906,8 @@ virSecuritySELinuxRestoreChardevLabel(virSecurityManagerPtr mgr, (virSecuritySELinuxRestoreFileLabel(mgr, in) < 0)) { goto done; } - } else if (virSecuritySELinuxRestoreFileLabel(mgr, dev_source->data.file.path) < 0) { + } else if (dev_source->data.file.path && + virSecuritySELinuxRestoreFileLabel(mgr, dev_source->data.file.path) < 0) { goto done; } ret = 0; -- 1.8.3.1

2 1

[libvirt] [PATCH] qemuDomainResume: allow to resume domain with guest panicked
by Dmitry Andreev 03 Feb '16

03 Feb '16

In case of guest panicked, preserved crashed domain has stopped CPUs. It's not possible to use tools like WinDbg for the problem investigation until we start CPUs back. --- src/qemu/qemu_driver.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e46404b..0930280 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1873,6 +1873,7 @@ static int qemuDomainResume(virDomainPtr dom) int ret = -1; virObjectEventPtr event = NULL; int state; + int reason; virQEMUDriverConfigPtr cfg = NULL; if (!(vm = qemuDomObjFromDomain(dom))) @@ -1892,12 +1893,14 @@ static int qemuDomainResume(virDomainPtr dom) goto endjob; } - state = virDomainObjGetState(vm, NULL); + state = virDomainObjGetState(vm, &reason); if (state == VIR_DOMAIN_PMSUSPENDED) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("domain is pmsuspended")); goto endjob; - } else if (state == VIR_DOMAIN_PAUSED) { + } else if ((state == VIR_DOMAIN_CRASHED && + reason == VIR_DOMAIN_CRASHED_PANICKED) || + state == VIR_DOMAIN_PAUSED) { if (qemuProcessStartCPUs(driver, vm, dom->conn, VIR_DOMAIN_RUNNING_UNPAUSED, QEMU_ASYNC_JOB_NONE) < 0) { -- 1.8.3.1

2 2

[libvirt] [PATCH v3] qemu: return -1 on error paths in qemuDomainSaveImageStartVM
by Nikolay Shirokovskiy 03 Feb '16

03 Feb '16

Error paths after sending the event that domain is started written as if ret = -1 which is set at the beginning of the function. It's common idioma to keep 'ret' equal to -1 until the end of function where it is set to 0. But here we use ret to keep result of restore operation too and thus breaks the idioma and its users :) Let's use different variable to hold restore result. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com> --- src/qemu/qemu_driver.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index abcdbe6..f612cf0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6732,6 +6732,7 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, qemuDomainAsyncJob asyncJob) { int ret = -1; + bool restored = false; virObjectEventPtr event; int intermediatefd = -1; virCommandPtr cmd = NULL; @@ -6758,13 +6759,14 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, } /* Set the migration source and start it up. */ - ret = qemuProcessStart(conn, driver, vm, asyncJob, - "stdio", *fd, path, NULL, - VIR_NETDEV_VPORT_PROFILE_OP_RESTORE, - VIR_QEMU_PROCESS_START_PAUSED); + if (qemuProcessStart(conn, driver, vm, asyncJob, + "stdio", *fd, path, NULL, + VIR_NETDEV_VPORT_PROFILE_OP_RESTORE, + VIR_QEMU_PROCESS_START_PAUSED) == 0) + restored = true; if (intermediatefd != -1) { - if (ret < 0) { + if (!restored) { /* if there was an error setting up qemu, the intermediate * process will wait forever to write to stdout, so we * must manually kill it. @@ -6775,7 +6777,7 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, if (virCommandWait(cmd, NULL) < 0) { qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, 0); - ret = -1; + restored = false; } VIR_DEBUG("Decompression binary stderr: %s", NULLSTR(errbuf)); } @@ -6783,18 +6785,16 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, if (VIR_CLOSE(*fd) < 0) { virReportSystemError(errno, _("cannot close file: %s"), path); - ret = -1; + restored = false; } - if (ret < 0) { - virDomainAuditStart(vm, "restored", false); + virDomainAuditStart(vm, "restored", restored); + if (!restored) goto cleanup; - } event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STARTED, VIR_DOMAIN_EVENT_STARTED_RESTORED); - virDomainAuditStart(vm, "restored", true); qemuDomainEventQueue(driver, event); -- 1.8.3.1

2 1

[libvirt] [PATCH 1/2] Move qemuGetDHCPInterfaces to separate file
by Guido Günther 03 Feb '16

03 Feb '16

so we can use it from the LXC driver as well. --- I couldn't find a nice place to add this so I went for a separate file. I'm happy to move this elsewhere. Cheers, -- Guido po/POTFILES.in | 1 + src/Makefile.am | 1 + src/libvirt_private.syms | 4 ++ src/qemu/qemu_driver.c | 104 +---------------------------------- src/util/virdhcpinterfaces.c | 127 +++++++++++++++++++++++++++++++++++++++++++ src/util/virdhcpinterfaces.h | 34 ++++++++++++ 6 files changed, 169 insertions(+), 102 deletions(-) create mode 100644 src/util/virdhcpinterfaces.c create mode 100644 src/util/virdhcpinterfaces.h diff --git a/po/POTFILES.in b/po/POTFILES.in index 82e8d3e..64eaa9d 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -186,6 +186,7 @@ src/util/vircommand.c src/util/virconf.c src/util/vircrypto.c src/util/virdbus.c +src/util/virdhcpinterfaces.c src/util/virdnsmasq.c src/util/virerror.c src/util/virerror.h diff --git a/src/Makefile.am b/src/Makefile.am index a4aef0f..bddfd21 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -104,6 +104,7 @@ UTIL_SOURCES = \ util/virconf.c util/virconf.h \ util/vircrypto.c util/vircrypto.h \ util/virdbus.c util/virdbus.h util/virdbuspriv.h \ + util/virdhcpinterfaces.c util/virdhcpinterfaces.h \ util/virdnsmasq.c util/virdnsmasq.h \ util/virebtables.c util/virebtables.h \ util/virendian.h \ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3d0ec9d..ea3e83c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1377,6 +1377,10 @@ virDBusMessageRead; virDBusSetSharedBus; +# util/virdhcpinterfaces.h +virGetDHCPInterfaces; + + # util/virdnsmasq.h dnsmasqAddDhcpHost; dnsmasqAddHost; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index abcdbe6..fe1cb6d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -101,6 +101,7 @@ #include "virnuma.h" #include "dirname.h" #include "network/bridge_driver.h" +#include "virdhcpinterfaces.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -173,10 +174,6 @@ static int qemuOpenFileAs(uid_t fallback_uid, gid_t fallback_gid, const char *path, int oflags, bool *needUnlink, bool *bypassSecurityDriver); -static int qemuGetDHCPInterfaces(virDomainPtr dom, - virDomainObjPtr vm, - virDomainInterfacePtr **ifaces); - virQEMUDriverPtr qemu_driver = NULL; @@ -19794,7 +19791,7 @@ qemuDomainInterfaceAddresses(virDomainPtr dom, switch (source) { case VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_LEASE: - ret = qemuGetDHCPInterfaces(dom, vm, ifaces); + ret = virGetDHCPInterfaces(dom, vm, ifaces); break; case VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_AGENT: @@ -19825,103 +19822,6 @@ qemuDomainInterfaceAddresses(virDomainPtr dom, return ret; } -static int -qemuGetDHCPInterfaces(virDomainPtr dom, - virDomainObjPtr vm, - virDomainInterfacePtr **ifaces) -{ - int rv = -1; - int n_leases = 0; - size_t i, j; - size_t ifaces_count = 0; - virNetworkPtr network = NULL; - char macaddr[VIR_MAC_STRING_BUFLEN]; - virDomainInterfacePtr iface = NULL; - virNetworkDHCPLeasePtr *leases = NULL; - virDomainInterfacePtr *ifaces_ret = NULL; - - if (!dom->conn->networkDriver || - !dom->conn->networkDriver->networkGetDHCPLeases) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Network driver does not support DHCP lease query")); - return -1; - } - - for (i = 0; i < vm->def->nnets; i++) { - if (vm->def->nets[i]->type != VIR_DOMAIN_NET_TYPE_NETWORK) - continue; - - virMacAddrFormat(&(vm->def->nets[i]->mac), macaddr); - virObjectUnref(network); - network = virNetworkLookupByName(dom->conn, - vm->def->nets[i]->data.network.name); - - if ((n_leases = virNetworkGetDHCPLeases(network, macaddr, - &leases, 0)) < 0) - goto error; - - if (n_leases) { - if (VIR_EXPAND_N(ifaces_ret, ifaces_count, 1) < 0) - goto error; - - if (VIR_ALLOC(ifaces_ret[ifaces_count - 1]) < 0) - goto error; - - iface = ifaces_ret[ifaces_count - 1]; - /* Assuming each lease corresponds to a separate IP */ - iface->naddrs = n_leases; - - if (VIR_ALLOC_N(iface->addrs, iface->naddrs) < 0) - goto error; - - if (VIR_STRDUP(iface->name, vm->def->nets[i]->ifname) < 0) - goto cleanup; - - if (VIR_STRDUP(iface->hwaddr, macaddr) < 0) - goto cleanup; - } - - for (j = 0; j < n_leases; j++) { - virNetworkDHCPLeasePtr lease = leases[j]; - virDomainIPAddressPtr ip_addr = &iface->addrs[j]; - - if (VIR_STRDUP(ip_addr->addr, lease->ipaddr) < 0) - goto cleanup; - - ip_addr->type = lease->type; - ip_addr->prefix = lease->prefix; - } - - for (j = 0; j < n_leases; j++) - virNetworkDHCPLeaseFree(leases[j]); - - VIR_FREE(leases); - } - - *ifaces = ifaces_ret; - ifaces_ret = NULL; - rv = ifaces_count; - - cleanup: - virObjectUnref(network); - if (leases) { - for (i = 0; i < n_leases; i++) - virNetworkDHCPLeaseFree(leases[i]); - } - VIR_FREE(leases); - - return rv; - - error: - if (ifaces_ret) { - for (i = 0; i < ifaces_count; i++) - virDomainInterfaceFree(ifaces_ret[i]); - } - VIR_FREE(ifaces_ret); - - goto cleanup; -} - static int qemuDomainSetUserPassword(virDomainPtr dom, diff --git a/src/util/virdhcpinterfaces.c b/src/util/virdhcpinterfaces.c new file mode 100644 index 0000000..80135f7 --- /dev/null +++ b/src/util/virdhcpinterfaces.c @@ -0,0 +1,127 @@ +/* + * virdhcpinterfaces.c: get a domains dhcp managed interfaces + * + * Copyright (C) 2016 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Daniel P. Berrange <berrange(a)redhat.com> + */ + +#include <config.h> + +#include "virdhcpinterfaces.h" +#include "viralloc.h" +#include "virstring.h" +#include "datatypes.h" + +#define VIR_FROM_THIS VIR_FROM_AUDIT + +int +virGetDHCPInterfaces(virDomainPtr dom, + virDomainObjPtr vm, + virDomainInterfacePtr **ifaces) +{ + int rv = -1; + int n_leases = 0; + size_t i, j; + size_t ifaces_count = 0; + virNetworkPtr network = NULL; + char macaddr[VIR_MAC_STRING_BUFLEN]; + virDomainInterfacePtr iface = NULL; + virNetworkDHCPLeasePtr *leases = NULL; + virDomainInterfacePtr *ifaces_ret = NULL; + + if (!dom->conn->networkDriver || + !dom->conn->networkDriver->networkGetDHCPLeases) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Network driver does not support DHCP lease query")); + return -1; + } + + for (i = 0; i < vm->def->nnets; i++) { + if (vm->def->nets[i]->type != VIR_DOMAIN_NET_TYPE_NETWORK) + continue; + + virMacAddrFormat(&(vm->def->nets[i]->mac), macaddr); + virObjectUnref(network); + network = virNetworkLookupByName(dom->conn, + vm->def->nets[i]->data.network.name); + + if ((n_leases = virNetworkGetDHCPLeases(network, macaddr, + &leases, 0)) < 0) + goto error; + + if (n_leases) { + if (VIR_EXPAND_N(ifaces_ret, ifaces_count, 1) < 0) + goto error; + + if (VIR_ALLOC(ifaces_ret[ifaces_count - 1]) < 0) + goto error; + + iface = ifaces_ret[ifaces_count - 1]; + /* Assuming each lease corresponds to a separate IP */ + iface->naddrs = n_leases; + + if (VIR_ALLOC_N(iface->addrs, iface->naddrs) < 0) + goto error; + + if (VIR_STRDUP(iface->name, vm->def->nets[i]->ifname) < 0) + goto cleanup; + + if (VIR_STRDUP(iface->hwaddr, macaddr) < 0) + goto cleanup; + } + + for (j = 0; j < n_leases; j++) { + virNetworkDHCPLeasePtr lease = leases[j]; + virDomainIPAddressPtr ip_addr = &iface->addrs[j]; + + if (VIR_STRDUP(ip_addr->addr, lease->ipaddr) < 0) + goto cleanup; + + ip_addr->type = lease->type; + ip_addr->prefix = lease->prefix; + } + + for (j = 0; j < n_leases; j++) + virNetworkDHCPLeaseFree(leases[j]); + + VIR_FREE(leases); + } + + *ifaces = ifaces_ret; + ifaces_ret = NULL; + rv = ifaces_count; + + cleanup: + virObjectUnref(network); + if (leases) { + for (i = 0; i < n_leases; i++) + virNetworkDHCPLeaseFree(leases[i]); + } + VIR_FREE(leases); + + return rv; + + error: + if (ifaces_ret) { + for (i = 0; i < ifaces_count; i++) + virDomainInterfaceFree(ifaces_ret[i]); + } + VIR_FREE(ifaces_ret); + + goto cleanup; +} diff --git a/src/util/virdhcpinterfaces.h b/src/util/virdhcpinterfaces.h new file mode 100644 index 0000000..c7acb8f --- /dev/null +++ b/src/util/virdhcpinterfaces.h @@ -0,0 +1,34 @@ +/* + * virdhcpinterfaces.h: get a domains dhcp managed interfaces + * + * Copyright (C) 2016 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Daniel P. Berrange <berrange(a)redhat.com> + */ + +#ifndef __VIR_DHCP_INTERFACES_H__ +# define __VIR_DHCP_INTERFACES_H__ + +# include "internal.h" +# include "virdomainobjlist.h" + +int +virGetDHCPInterfaces(virDomainPtr dom, + virDomainObjPtr vm, + virDomainInterfacePtr **ifaces); + +#endif /* __VIR_DHCP_INTERFACES_H__ */ -- 2.7.0.rc3

2 3

[libvirt] [PATCH v2 00/21] vcpu info storage refactors - part 2
by Peter Krempa 03 Feb '16

03 Feb '16

Some of patches were pushed. The rest fixes most of the feedback that made sense that was against v1. Peter Krempa (21): cgroup: Clean up virCgroupGetPercpuStats conf: Add helper to retrieve bitmap of active vcpus for a definition cgroup: Prepare for sparse vCPU topologies in virCgroupGetPercpuStats qemu: cpu hotplug: Set vcpu state directly in the new structure qemu: Move and rename qemuProcessDetectVcpuPIDs to qemuDomainDetectVcpuPids qemu: domain: Prepare qemuDomainDetectVcpuPids for reuse qemu: Reuse qemuDomainDetectVcpuPids in cpu hot(un)plug conf: Don't copy def->cpumask into cpu pinning info conf: Split out logic to determine whether cpupin was provided conf: Store cpu pinning data in def->vcpus conf: remove unused cpu pinning helpers and data structures conf: Extract code that formats <cputune> util: bitmap: Introduce bitmap subtraction conf: Add helper to return a bitmap of active iothread ids conf: Extract code for parsing thread resource scheduler info conf: Don't store vcpusched orthogonally to other vcpu info conf: Fix how iothread scheduler info is stored qemu: vcpu: Aggregate code to set vCPU tuning qemu: vcpu: Reuse qemuProcessSetupVcpu in vcpu hotplug qemu: iothread: Aggregate code to set IOTrhead tuning qemu: iothread: Reuse qemuProcessSetupIOThread in iothread hotplug src/conf/domain_conf.c | 940 +++++++++++---------- src/conf/domain_conf.h | 45 +- src/libvirt_private.syms | 10 +- src/libxl/libxl_domain.c | 20 +- src/libxl/libxl_driver.c | 41 +- src/lxc/lxc_driver.c | 2 +- src/qemu/qemu_cgroup.c | 195 ----- src/qemu/qemu_cgroup.h | 2 - src/qemu/qemu_domain.c | 84 ++ src/qemu/qemu_domain.h | 2 + src/qemu/qemu_driver.c | 405 +++------ src/qemu/qemu_process.c | 478 ++++++----- src/qemu/qemu_process.h | 6 + src/test/test_driver.c | 45 +- src/util/virbitmap.c | 21 + src/util/virbitmap.h | 3 + src/util/vircgroup.c | 74 +- src/util/vircgroup.h | 3 +- src/vz/vz_sdk.c | 4 +- .../qemuxml2xmlout-cputune-iothreadsched.xml | 3 +- tests/virbitmaptest.c | 55 ++ tests/vircgrouptest.c | 2 +- 22 files changed, 1148 insertions(+), 1292 deletions(-) -- 2.6.2

2 43

[libvirt] [PATCH v4] qemu: Connect to guest agent iff needed
by Michal Privoznik 03 Feb '16

03 Feb '16

https://bugzilla.redhat.com/show_bug.cgi?id=1293351 Since we already have virtio channel events, we know when guest agent within guest has (dis-)connected. Instead of us blindly connecting to a socket that no one is listening to, we can just follow what qemu-ga does. This has a nice benefit that we don't need to 'guest-ping' the agent just to timeout and find out nobody is listening. The way that this commit is implemented: - don't connect in qemuProcessStart directly, defer that to event callback (which already follows the agent). - after migration is settled, before we resume vCPUs, ask qemu whether somebody is listening on the socket and if so, connect to it. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> --- diff to v3: -Move cap detection into qemuConnectAgent src/qemu/qemu_driver.c | 13 ++++++++++++- src/qemu/qemu_migration.c | 12 ++++++++++++ src/qemu/qemu_process.c | 24 ++++++++++++++++++++++++ 3 files changed, 48 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8ccf68b..3751ba6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6650,12 +6650,13 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, bool start_paused, qemuDomainAsyncJob asyncJob) { - int ret = -1; + int rc, ret = -1; virObjectEventPtr event; int intermediatefd = -1; virCommandPtr cmd = NULL; char *errbuf = NULL; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + qemuDomainObjPrivatePtr priv = vm->privateData; if ((header->version == 2) && (header->compressed != QEMU_SAVE_FORMAT_RAW)) { @@ -6710,6 +6711,16 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, goto cleanup; } + if ((rc = qemuConnectAgent(driver, vm)) < 0) { + if (rc == -2) + goto cleanup; + + VIR_WARN("Cannot connect to QEMU guest agent for %s", + vm->def->name); + virResetLastError(); + priv->agentError = true; + } + event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STARTED, VIR_DOMAIN_EVENT_STARTED_RESTORED); diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 51e7125..9678adf 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -5795,6 +5795,7 @@ qemuMigrationFinish(virQEMUDriverPtr driver, unsigned short port; unsigned long long timeReceived = 0; virObjectEventPtr event; + int rc; VIR_DEBUG("driver=%p, dconn=%p, vm=%p, cookiein=%s, cookieinlen=%d, " "cookieout=%p, cookieoutlen=%p, flags=%lx, retcode=%d", @@ -5863,6 +5864,17 @@ qemuMigrationFinish(virQEMUDriverPtr driver, if (qemuMigrationStopNBDServer(driver, vm, mig) < 0) goto endjob; + if ((rc = qemuConnectAgent(driver, vm)) < 0) { + if (rc == -2) + goto endjob; + + VIR_WARN("Cannot connect to QEMU guest agent for %s", + vm->def->name); + virResetLastError(); + priv->agentError = true; + } + + if (flags & VIR_MIGRATE_PERSIST_DEST) { if (qemuMigrationPersist(driver, vm, mig, !v3proto) < 0) { /* Hmpf. Migration was successful, but making it persistent diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 05cbda2..e1a25dd 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -77,6 +77,10 @@ VIR_LOG_INIT("qemu.qemu_process"); +static int +qemuProcessReconnectRefreshChannelVirtioState(virQEMUDriverPtr driver, + virDomainObjPtr vm); + /** * qemuProcessRemoveDomainStatus * @@ -208,6 +212,26 @@ qemuConnectAgent(virQEMUDriverPtr driver, virDomainObjPtr vm) if (!config) return 0; + if (priv->agent) + return 0; + + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VSERPORT_CHANGE) && + config->state != VIR_DOMAIN_CHR_DEVICE_STATE_CONNECTED) { + /* So qemu is capable of sending us event whenever guest agent + * (dis-)connects. And we reflect its state in config->state. Well, + * nearly. It may happen that what we think about the guest agent state + * is not accurate, e.g. right after migration. Ask qemu to be sure. */ + + if (qemuProcessReconnectRefreshChannelVirtioState(driver, vm) < 0) + goto cleanup; + + /* Now we are sure about the channel state. */ + if (config->state != VIR_DOMAIN_CHR_DEVICE_STATE_CONNECTED) { + VIR_DEBUG("Deferring connecting to guest agent"); + return 0; + } + } + if (virSecurityManagerSetDaemonSocketLabel(driver->securityManager, vm->def) < 0) { VIR_ERROR(_("Failed to set security context for agent for %s"), -- 2.4.10

2 3

[libvirt] [PATCH] qemu: Don't crash when create fails early
by Martin Kletzander 02 Feb '16

02 Feb '16

Since commit 714080791778e3dfbd484ccb3953bffd820b8ba9 we are generating socket path later than before -- when starting a domain. That makes one particular inconsistent state of a chardev, which was not possible before, currently valid. However, SELinux security driver forgot to guard the main restoring function by a check for NULL-paths. So make it no-op for NULL paths, as in the DAC driver. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1300532 Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com> --- src/security/security_selinux.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 9e986350fbb1..6a32f0a27f12 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1024,6 +1024,12 @@ virSecuritySELinuxRestoreFileLabel(virSecurityManagerPtr mgr, char *newpath = NULL; char ebuf[1024]; + /* Some paths are auto-generated, so let's be safe here and do + * nothing if nothing is needed. + */ + if (!path) + return 0; + VIR_INFO("Restoring SELinux context on '%s'", path); if (virFileResolveLink(path, &newpath) < 0) { -- 2.7.0

2 1