[libvirt] [PATCH v4 0/5] Allow to make disk optional on migration
This is a 4rd version of patches [1]. Diff to v3 includes Daniel's review and some other improvements. The aim of these patches is to mark CDROM and floppy optional, thus they might get dropped during migration, domain startup, restore, etc. The granularity is set on disk level, so user can decide which cd-roms are important and which not. There are 3 levels of optionality: - mandatory - fail if missing for any reason (the default) - requisite - fail if missing on boot up, drop if missing on migrate/restore/revert - optional - drop if missing at any start attempt. To assign a disk one of these, just append 'startupPolicy' attribute to its <source>. If a domain is then eventually started (migrated, restored, whatever), we check for source path being accessible (currently only qemu driver is supported) from hypervisor's POV. This is done by virFileAccessibleAs() function. Currently, only uid+gid are considered. Ideally, this should be extended to SElinux labeling and cgroups. 1: https://www.redhat.com/archives/libvir-list/2011-October/msg00896.html Michal Privoznik (5): conf: Introduce optional startupPolicy attribute for cdrom and floppy util: Create virFileAccessibleAs function qemu: Move device alias assigning before command line construction qemu: implement startupPolicy on_missing: Emit event on disk source dropping daemon/remote.c | 39 ++++++++++ docs/formatdomain.html.in | 26 ++++++- docs/schemas/domaincommon.rng | 22 +++++- examples/domain-events/events-c/event-test.c | 28 +++++++- examples/domain-events/events-python/event-test.py | 4 + include/libvirt/libvirt.h.in | 29 +++++++ python/libvirt-override-virConnect.py | 9 ++ python/libvirt-override.c | 52 +++++++++++++ src/conf/domain_conf.c | 43 ++++++++++- src/conf/domain_conf.h | 11 +++ src/conf/domain_event.c | 61 +++++++++++++++ src/conf/domain_event.h | 9 ++ src/libvirt_private.syms | 4 + src/qemu/qemu_command.c | 5 +- src/qemu/qemu_command.h | 1 + src/qemu/qemu_domain.c | 73 ++++++++++++++++++ src/qemu/qemu_domain.h | 4 + src/qemu/qemu_driver.c | 3 + src/qemu/qemu_process.c | 7 ++ src/remote/remote_driver.c | 35 +++++++++ src/remote/remote_protocol.x | 10 ++- src/remote_protocol-structs | 6 ++ src/util/util.c | 78 ++++++++++++++++++++ src/util/util.h | 3 + tests/qemuxml2argvtest.c | 3 + .../qemuxml2xmlout-disk-cdrom-empty.xml | 31 ++++++++ tests/qemuxmlnstest.c | 3 + 27 files changed, 585 insertions(+), 14 deletions(-) create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-cdrom-empty.xml -- 1.7.3.4
This attribute says what to do with cdrom (or floppy) if the source is missing. It accepts: - mandatory - fail if missing for any reason (the default) - requisite - fail if missing on boot up, drop if missing on migrate/restore/revert - optional - drop if missing at any start attempt. However, this patch introduces only XML part of this new functionality. --- docs/formatdomain.html.in | 26 +++++++++++- docs/schemas/domaincommon.rng | 22 +++++++++- src/conf/domain_conf.c | 43 ++++++++++++++++++- src/conf/domain_conf.h | 11 +++++ src/libvirt_private.syms | 2 + .../qemuxml2xmlout-disk-cdrom-empty.xml | 31 ++++++++++++++ 6 files changed, 127 insertions(+), 8 deletions(-) create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-cdrom-empty.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 12055c2..9a74001 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -891,7 +891,7 @@ <devices> <disk type='file' snapshot='external'> <driver name="tap" type="aio" cache="default"/> - <source file='/var/lib/xen/images/fv0'/> + <source file='/var/lib/xen/images/fv0'/ startupPolicy='optional'> <target dev='hda' bus='ide'/> <boot order='2'/> <encryption type='...'> @@ -962,7 +962,29 @@ "network", the <code>source</code> may have zero or more <code>host</code> sub-elements used to specify the hosts to connect. - <span class="since">Since 0.0.3</span></dd> + <span class="since">Since 0.0.3</span> + For "file" disk type which represents cdrom or floppy + (the <code>device</code> attribute) it is possible to define + policy what to do with disk if source is not accessible. + This is done by <code>startupPolicy</code> attribute accepting + these values: + <table class="top_table"> + <tr> + <td> mandatory </td> + <td> fail if missing for any reason (the default) </td> + </tr> + <tr> + <td> requisite </td> + <td> fail if missing on boot up, + drop if missing on migrate/restore/revert </td> + </tr> + <tr> + <td> optional </td> + <td> drop if missing at any start attempt </td> + </tr> + </table> + <span class="since">Since 0.9.7</span> + </dd> <dt><code>target</code></dt> <dd>The <code>target</code> element controls the bus / device under which the disk is exposed to the guest diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index cd1a067..08cc64f 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -665,6 +665,17 @@ </interleave> </element> </define> + + <define name="startupPolicy"> + <attribute name="startupPolicy"> + <choice> + <value>mandatory</value> + <value>requisite</value> + <value>optional</value> + </choice> + </attribute> + </define> + <!-- A disk description can be either of type file or block The name of the attribute on the source element depends on the type @@ -692,9 +703,14 @@ <interleave> <optional> <element name="source"> - <attribute name="file"> - <ref name="absFilePath"/> - </attribute> + <optional> + <attribute name="file"> + <ref name="absFilePath"/> + </attribute> + </optional> + <optional> + <ref name="startupPolicy"/> + </optional> <empty/> </element> </optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5959593..87095ff 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -578,6 +578,12 @@ VIR_ENUM_IMPL(virDomainNumatuneMemMode, VIR_DOMAIN_NUMATUNE_MEM_LAST, "preferred", "interleave"); +VIR_ENUM_IMPL(virDomainStartupPolicy, VIR_DOMAIN_STARTUP_POLICY_LAST, + "default", + "mandatory", + "requisite", + "optional"); + #define virDomainReportError(code, ...) \ virReportErrorHelper(VIR_FROM_DOMAIN, code, __FILE__, \ __FUNCTION__, __LINE__, __VA_ARGS__) @@ -2319,6 +2325,7 @@ virDomainDiskDefParseXML(virCapsPtr caps, char *devaddr = NULL; virStorageEncryptionPtr encryption = NULL; char *serial = NULL; + char *startupPolicy = NULL; if (VIR_ALLOC(def) < 0) { virReportOOMError(); @@ -2347,6 +2354,7 @@ virDomainDiskDefParseXML(virCapsPtr caps, switch (def->type) { case VIR_DOMAIN_DISK_TYPE_FILE: source = virXMLPropString(cur, "file"); + startupPolicy = virXMLPropString(cur, "startupPolicy"); break; case VIR_DOMAIN_DISK_TYPE_BLOCK: source = virXMLPropString(cur, "dev"); @@ -2646,6 +2654,27 @@ virDomainDiskDefParseXML(virCapsPtr caps, goto error; } + if (startupPolicy) { + int i; + + if ((i = virDomainStartupPolicyTypeFromString(startupPolicy)) < 0) { + virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown startupPolicy value '%s'"), + startupPolicy); + goto error; + } + + if (def->device != VIR_DOMAIN_DISK_DEVICE_CDROM && + def->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY) { + virDomainReportError(VIR_ERR_INVALID_ARG, + _("Setting disk %s is allowed only for " + "cdrom or floppy"), + startupPolicy); + goto error; + } + def->startupPolicy = i; + } + def->src = source; source = NULL; def->dst = target; @@ -2701,6 +2730,7 @@ cleanup: VIR_FREE(devaddr); VIR_FREE(serial); virStorageEncryptionFree(encryption); + VIR_FREE(startupPolicy); return def; @@ -9176,6 +9206,7 @@ virDomainDiskDefFormat(virBufferPtr buf, const char *iomode = virDomainDiskIoTypeToString(def->iomode); const char *ioeventfd = virDomainIoEventFdTypeToString(def->ioeventfd); const char *event_idx = virDomainVirtioEventIdxTypeToString(def->event_idx); + const char *startupPolicy = virDomainStartupPolicyTypeToString(def->startupPolicy); if (!type) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, @@ -9234,11 +9265,17 @@ virDomainDiskDefFormat(virBufferPtr buf, virBufferAsprintf(buf, "/>\n"); } - if (def->src || def->nhosts > 0) { + if (def->src || def->nhosts > 0 || + def->startupPolicy) { switch (def->type) { case VIR_DOMAIN_DISK_TYPE_FILE: - virBufferEscapeString(buf, " <source file='%s'/>\n", - def->src); + virBufferAsprintf(buf," <source"); + if (def->src) + virBufferEscapeString(buf, " file='%s'", def->src); + if (def->startupPolicy) + virBufferEscapeString(buf, " startupPolicy='%s'", + startupPolicy); + virBufferAsprintf(buf, "/>\n"); break; case VIR_DOMAIN_DISK_TYPE_BLOCK: virBufferEscapeString(buf, " <source dev='%s'/>\n", diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 2119b5a..88853fc 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -269,6 +269,15 @@ enum virDomainSnapshotState { VIR_DOMAIN_DISK_SNAPSHOT = VIR_DOMAIN_LAST, }; +enum virDomainStartupPolicy { + VIR_DOMAIN_STARTUP_POLICY_DEFAULT = 0, + VIR_DOMAIN_STARTUP_POLICY_MANDATORY, + VIR_DOMAIN_STARTUP_POLICY_REQUISITE, + VIR_DOMAIN_STARTUP_POLICY_OPTIONAL, + + VIR_DOMAIN_STARTUP_POLICY_LAST +}; + /* Stores the virtual disk configuration */ typedef struct _virDomainDiskDef virDomainDiskDef; typedef virDomainDiskDef *virDomainDiskDefPtr; @@ -292,6 +301,7 @@ struct _virDomainDiskDef { int ioeventfd; int event_idx; int snapshot; /* enum virDomainDiskSnapshot */ + int startupPolicy; /* enum virDomainStartupPolicy */ unsigned int readonly : 1; unsigned int shared : 1; unsigned int transient : 1; @@ -1935,4 +1945,5 @@ VIR_ENUM_DECL(virDomainTimerTrack) VIR_ENUM_DECL(virDomainTimerTickpolicy) VIR_ENUM_DECL(virDomainTimerMode) +VIR_ENUM_DECL(virDomainStartupPolicy) #endif /* __DOMAIN_CONF_H */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index dedbd16..b83fd3f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -427,6 +427,8 @@ virDomainSnapshotUpdateRelations; virDomainSoundDefFree; virDomainSoundModelTypeFromString; virDomainSoundModelTypeToString; +virDomainStartupPolicyTypeFromString; +virDomainStartupPolicyTypeToString; virDomainStateReasonFromString; virDomainStateReasonToString; virDomainStateTypeFromString; diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-cdrom-empty.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-cdrom-empty.xml new file mode 100644 index 0000000..9299da3 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-cdrom-empty.xml @@ -0,0 +1,31 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219100</memory> + <currentMemory>219100</currentMemory> + <vcpu>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' unit='0'/> + </disk> + <disk type='file' device='cdrom'> + <target dev='hdc' bus='ide'/> + <source startupPolicy='optional'/> + <readonly/> + <address type='drive' controller='0' bus='1' unit='0'/> + </disk> + <controller type='ide' index='0'/> + <memballoon model='virtio'/> + </devices> +</domain> -- 1.7.3.4
On Thu, Oct 20, 2011 at 04:52:48PM +0200, Michal Privoznik wrote:
This attribute says what to do with cdrom (or floppy) if the source is missing. It accepts: - mandatory - fail if missing for any reason (the default) - requisite - fail if missing on boot up, drop if missing on migrate/restore/revert - optional - drop if missing at any start attempt.
However, this patch introduces only XML part of this new functionality. --- docs/formatdomain.html.in | 26 +++++++++++- docs/schemas/domaincommon.rng | 22 +++++++++- src/conf/domain_conf.c | 43 ++++++++++++++++++- src/conf/domain_conf.h | 11 +++++ src/libvirt_private.syms | 2 + .../qemuxml2xmlout-disk-cdrom-empty.xml | 31 ++++++++++++++ 6 files changed, 127 insertions(+), 8 deletions(-) create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-cdrom-empty.xml
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
This function checks if a given path is accessible under given uid and gid. --- src/util/util.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/util/util.h | 3 ++ 2 files changed, 81 insertions(+), 0 deletions(-) diff --git a/src/util/util.c b/src/util/util.c index 01146f5..2020415 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -671,6 +671,72 @@ virFileIsExecutable(const char *file) } #ifndef WIN32 +/* Check that a file is accessible under certain + * user & gid. + * @mode can be R_OK, W_OK, X_OK or F_OK. + * see 'man accesss' for more details. + * Returns 0 on success, -errno on fail, + * >0 on signaled child. + */ +int +virFileAccessibleAs(const char *path, int mode, + uid_t uid, gid_t gid) +{ + pid_t pid; + int waitret, status, ret = 0; + int forkRet; + + forkRet = virFork(&pid); + + if (pid < 0) { + return -1; + } + + if (pid) { /* parent */ + do { + if ((waitret = waitpid(pid, &status, 0)) < 0) { + ret = -errno; + virKillProcess(pid, SIGKILL); + return ret; + } + } while (!WIFEXITED(status) && !WIFSIGNALED(status)); + + /* child actually returns positive value + * anded by 0xFF (see man waitpid @ WEXITSTATUS) + */ + + if (WIFSIGNALED(status)) + return WTERMSIG(status); + + return -WEXITSTATUS(status); + } + + /* child. + * Return positive value here. Parent + * will change it to negative one. */ + + if (forkRet < 0) { + ret = 1; + goto childerror; + } + + if (virSetUIDGID(uid, gid) < 0) { + ret = errno; + goto childerror; + } + + if (access(path, mode) < 0) + ret = errno; + +childerror: + if ((ret & 0xFF) != ret) { + VIR_WARN("unable to pass desired return value %d", ret); + ret = 0xFF; + } + + _exit(ret); +} + /* return -errno on failure, or 0 on success */ static int virFileOpenAsNoFork(const char *path, int openflags, mode_t mode, @@ -993,6 +1059,18 @@ childerror: #else /* WIN32 */ +int +virFileAccessibleAs(const char *path ATTRIBUTE_UNUSED, + int mode ATTRIBUTE_UNUSED, + uid_t uid ATTRIBUTE_UNUSED, + git_t gid ATTRIBUTE_UNUSED) +{ + virUtilError(VIR_ERR_INTERNAL_ERROR, + "%s", _("virFileAccessibleAs is not implemented for WIN32")); + + return -ENOSYS; +} + /* return -errno on failure, or 0 on success */ int virFileOpenAs(const char *path ATTRIBUTE_UNUSED, int openflags ATTRIBUTE_UNUSED, diff --git a/src/util/util.h b/src/util/util.h index c55e852..e869f1b 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -93,6 +93,9 @@ enum { VIR_FILE_OPEN_AS_UID = (1 << 0), VIR_FILE_OPEN_FORCE_PERMS = (1 << 1), }; +int virFileAccessibleAs(const char *path, int mode, + uid_t uid, gid_t gid) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; int virFileOpenAs(const char *path, int openflags, mode_t mode, uid_t uid, gid_t gid, unsigned int flags) -- 1.7.3.4
On Thu, Oct 20, 2011 at 04:52:49PM +0200, Michal Privoznik wrote:
This function checks if a given path is accessible under given uid and gid. --- src/util/util.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/util/util.h | 3 ++ 2 files changed, 81 insertions(+), 0 deletions(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 10/20/2011 08:52 AM, Michal Privoznik wrote:
This function checks if a given path is accessible under given uid and gid. --- src/util/util.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/util/util.h | 3 ++ 2 files changed, 81 insertions(+), 0 deletions(-)
diff --git a/src/util/util.c b/src/util/util.c index 01146f5..2020415 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -671,6 +671,72 @@ virFileIsExecutable(const char *file) }
#ifndef WIN32 +/* Check that a file is accessible under certain + * user& gid. + * @mode can be R_OK, W_OK, X_OK or F_OK.
More accurately: @mode can be F_OK, or a bitwise combination of R_OK, W_OK, and X_OK.
+ * see 'man accesss' for more details.
s/accesss/access/
+ * Returns 0 on success, -errno on fail, + *>0 on signaled child. + */ +int +virFileAccessibleAs(const char *path, int mode, + uid_t uid, gid_t gid) +{ + pid_t pid; + int waitret, status, ret = 0; + int forkRet; + + forkRet = virFork(&pid);
Should you take a shortcut that if uid and gid match geteuid()/getegid(), then we avoid the fork()? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
This function checks if a given path is accessible under given uid and gid. --- src/util/util.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/util/util.h | 3 ++ 2 files changed, 86 insertions(+), 0 deletions(-) diff --git a/src/util/util.c b/src/util/util.c index af27344..7343485 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -671,6 +671,77 @@ virFileIsExecutable(const char *file) } #ifndef WIN32 +/* Check that a file is accessible under certain + * user & gid. + * @mode can be F_OK, or a bitwise combination of R_OK, W_OK, and X_OK. + * see 'man access' for more details. + * Returns 0 on success, -errno on fail, + * >0 on signaled child. + */ +int +virFileAccessibleAs(const char *path, int mode, + uid_t uid, gid_t gid) +{ + pid_t pid = 0; + int waitret, status, ret = 0; + int forkRet = 0; + bool do_fork = (uid != getuid() || gid != getgid()); + + if (do_fork) + forkRet = virFork(&pid); + + if (pid < 0) { + return -1; + } + + if (pid) { /* parent */ + do { + if ((waitret = waitpid(pid, &status, 0)) < 0) { + ret = -errno; + virKillProcess(pid, SIGKILL); + return ret; + } + } while (!WIFEXITED(status) && !WIFSIGNALED(status)); + + /* child actually returns positive value + * anded by 0xFF (see man waitpid @ WEXITSTATUS) + */ + + if (WIFSIGNALED(status)) + return WTERMSIG(status); + + return -WEXITSTATUS(status); + } + + /* child. + * Return positive value here. Parent + * will change it to negative one. */ + + if (forkRet < 0) { + ret = 1; + goto childerror; + } + + if (do_fork && virSetUIDGID(uid, gid) < 0) { + ret = errno; + goto childerror; + } + + if (access(path, mode) < 0) + ret = errno; + +childerror: + if (do_fork && (ret & 0xFF) != ret) { + VIR_WARN("unable to pass desired return value %d", ret); + ret = 0xFF; + } + + if (do_fork) + _exit(ret); + + return -ret; +} + /* return -errno on failure, or 0 on success */ static int virFileOpenAsNoFork(const char *path, int openflags, mode_t mode, @@ -993,6 +1064,18 @@ childerror: #else /* WIN32 */ +int +virFileAccessibleAs(const char *path ATTRIBUTE_UNUSED, + int mode ATTRIBUTE_UNUSED, + uid_t uid ATTRIBUTE_UNUSED, + git_t gid ATTRIBUTE_UNUSED) +{ + virUtilError(VIR_ERR_INTERNAL_ERROR, + "%s", _("virFileAccessibleAs is not implemented for WIN32")); + + return -ENOSYS; +} + /* return -errno on failure, or 0 on success */ int virFileOpenAs(const char *path ATTRIBUTE_UNUSED, int openflags ATTRIBUTE_UNUSED, diff --git a/src/util/util.h b/src/util/util.h index c55e852..e869f1b 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -93,6 +93,9 @@ enum { VIR_FILE_OPEN_AS_UID = (1 << 0), VIR_FILE_OPEN_FORCE_PERMS = (1 << 1), }; +int virFileAccessibleAs(const char *path, int mode, + uid_t uid, gid_t gid) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; int virFileOpenAs(const char *path, int openflags, mode_t mode, uid_t uid, gid_t gid, unsigned int flags) -- 1.7.3.4
On 10/21/2011 07:12 AM, Michal Privoznik wrote:
This function checks if a given path is accessible under given uid and gid. --- src/util/util.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/util/util.h | 3 ++ 2 files changed, 86 insertions(+), 0 deletions(-)
diff --git a/src/util/util.c b/src/util/util.c index af27344..7343485 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -671,6 +671,77 @@ virFileIsExecutable(const char *file) }
#ifndef WIN32 +/* Check that a file is accessible under certain + * user& gid. + * @mode can be F_OK, or a bitwise combination of R_OK, W_OK, and X_OK. + * see 'man access' for more details. + * Returns 0 on success, -errno on fail, + *>0 on signaled child.
That's awkward. How about just stating: Returns 0 on success, -1 on fail with errno set.
+ */ +int +virFileAccessibleAs(const char *path, int mode, + uid_t uid, gid_t gid) +{ + pid_t pid = 0; + int waitret, status, ret = 0; + int forkRet = 0; + bool do_fork = (uid != getuid() || gid != getgid()); + + if (do_fork) + forkRet = virFork(&pid);
The control flow is a bit hard to follow. While it looks correct, I think it would be easier to read as follows (it also matches my proposal to return -1 with errno set, when encountering error): if (uid == getuid() && gid == getgid()) return access(path, mode); forkRet = virFork(&pid); ... code without reference to do_fork ...
+ if (pid) { /* parent */ + do { + if ((waitret = waitpid(pid,&status, 0))< 0) { + ret = -errno; + virKillProcess(pid, SIGKILL); + return ret; + } + } while (!WIFEXITED(status)&& !WIFSIGNALED(status));
Use virPidWait() (from command.h), not this hand-rolled do/while loop. In particular, your loop will only ever execute exactly once, since status will always satisfy either WIFEXITED or WIFSIGNALED if waitpid() was successful; and you fail to handle EINTR as a reason to retry waitpid. Returning a positive value for a signaled child is awkward; I would just treat that as failure, by setting errno to EIO and returning -1. The likelihood of the child dying by signal is super-slim, not to mention that I don't see how the client can behave any better by knowing that the child process died than if it just reacted as if access were denied in the first place.
@@ -993,6 +1064,18 @@ childerror:
#else /* WIN32 */
+int +virFileAccessibleAs(const char *path ATTRIBUTE_UNUSED, + int mode ATTRIBUTE_UNUSED, + uid_t uid ATTRIBUTE_UNUSED, + git_t gid ATTRIBUTE_UNUSED) +{ + virUtilError(VIR_ERR_INTERNAL_ERROR, + "%s", _("virFileAccessibleAs is not implemented for WIN32")); + + return -ENOSYS;
Rather than failing with -ENOSYS, can we at least try access(path, mode), ignoring uid and gid? It may have false negatives (fail in cases where a different uid would succeed), but is more likely to be useful than just blindly declaring failure. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
This function checks if a given path is accessible under given uid and gid. --- src/util/util.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/util/util.h | 3 ++ 2 files changed, 75 insertions(+), 0 deletions(-) diff --git a/src/util/util.c b/src/util/util.c index af27344..f21e929 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -671,6 +671,66 @@ virFileIsExecutable(const char *file) } #ifndef WIN32 +/* Check that a file is accessible under certain + * user & gid. + * @mode can be F_OK, or a bitwise combination of R_OK, W_OK, and X_OK. + * see 'man access' for more details. + * Returns 0 on success, -1 on fail with errno set. + */ +int +virFileAccessibleAs(const char *path, int mode, + uid_t uid, gid_t gid) +{ + pid_t pid = 0; + int status, ret = 0; + int forkRet = 0; + + if (uid == getuid() && + gid == getgid()) + return access(path, mode); + + forkRet = virFork(&pid); + + if (pid < 0) { + return -1; + } + + if (pid) { /* parent */ + if (virPidWait(pid, &status) < 0) { + /* virPidWait() already + * reported error */ + return -1; + } + + return -(status & 0xFF); + } + + /* child. + * Return positive value here. Parent + * will change it to negative one. */ + + if (forkRet < 0) { + ret = 1; + goto childerror; + } + + if (virSetUIDGID(uid, gid) < 0) { + ret = 1; + goto childerror; + } + + if (access(path, mode) < 0) + ret = 1; + +childerror: + if ((ret & 0xFF) != ret) { + VIR_WARN("unable to pass desired return value %d", ret); + ret = 0xFF; + } + + _exit(ret); +} + /* return -errno on failure, or 0 on success */ static int virFileOpenAsNoFork(const char *path, int openflags, mode_t mode, @@ -993,6 +1053,18 @@ childerror: #else /* WIN32 */ +int +virFileAccessibleAs(const char *path, + int mode, + uid_t uid ATTRIBUTE_UNUSED, + git_t gid ATTRIBUTE_UNUSED) +{ + + VIR_WARN("Ignoring uid/gid due to WIN32"); + + return access(path, mode); +} + /* return -errno on failure, or 0 on success */ int virFileOpenAs(const char *path ATTRIBUTE_UNUSED, int openflags ATTRIBUTE_UNUSED, diff --git a/src/util/util.h b/src/util/util.h index c55e852..e869f1b 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -93,6 +93,9 @@ enum { VIR_FILE_OPEN_AS_UID = (1 << 0), VIR_FILE_OPEN_FORCE_PERMS = (1 << 1), }; +int virFileAccessibleAs(const char *path, int mode, + uid_t uid, gid_t gid) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; int virFileOpenAs(const char *path, int openflags, mode_t mode, uid_t uid, gid_t gid, unsigned int flags) -- 1.7.3.4
On 10/24/2011 04:23 AM, Michal Privoznik wrote:
This function checks if a given path is accessible under given uid and gid. --- src/util/util.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/util/util.h | 3 ++ 2 files changed, 75 insertions(+), 0 deletions(-)
+ + if (pid) { /* parent */ + if (virPidWait(pid,&status)< 0) { + /* virPidWait() already + * reported error */ + return -1; + } + + return -(status& 0xFF);
Per the method docs, I think this has to be: errno = status; return -1;
+ } + + /* child. + * Return positive value here. Parent + * will change it to negative one. */ + + if (forkRet< 0) { + ret = 1;
ret = errno;
+ goto childerror; + } + + if (virSetUIDGID(uid, gid)< 0) { + ret = 1;
ret = errno;
+ goto childerror; + } + + if (access(path, mode)< 0) + ret = 1;
ret = errno;
+ +childerror: + if ((ret& 0xFF) != ret) { + VIR_WARN("unable to pass desired return value %d", ret); + ret = 0xFF; + } + + _exit(ret);
That way, your exit status is 0 on success, and an errno value on error. ACK - I'm comfortable with you making those changes and pushing, without having to see v7. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
This patch is rather cosmetic as it only moves device alias assignation from command line construction just before that. However, it is needed in connotation of previous and next patch. --- src/qemu/qemu_command.c | 5 +---- src/qemu/qemu_command.h | 1 + src/qemu/qemu_driver.c | 3 +++ src/qemu/qemu_process.c | 3 +++ tests/qemuxml2argvtest.c | 3 +++ tests/qemuxmlnstest.c | 3 +++ 6 files changed, 14 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 30c0be6..0c5bfab 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -656,7 +656,7 @@ qemuAssignDeviceControllerAlias(virDomainControllerDefPtr controller) } -static int +int qemuAssignDeviceAliases(virDomainDefPtr def, virBitmapPtr qemuCaps) { int i; @@ -3284,9 +3284,6 @@ qemuBuildCommandLine(virConnectPtr conn, bool usblegacy = false; uname_normalize(&ut); - if (qemuAssignDeviceAliases(def, qemuCaps) < 0) - return NULL; - virUUIDFormat(def->uuid, uuid); emulator = def->emulator; diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 00e58a2..d34df8f 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -187,6 +187,7 @@ int qemuDomainPCIAddressReleaseSlot(qemuDomainPCIAddressSetPtr addrs, int slot); void qemuDomainPCIAddressSetFree(qemuDomainPCIAddressSetPtr addrs); int qemuAssignDevicePCISlots(virDomainDefPtr def, qemuDomainPCIAddressSetPtr addrs); +int qemuAssignDeviceAliases(virDomainDefPtr def, virBitmapPtr qemuCaps); int qemuDomainNetVLAN(virDomainNetDefPtr def); int qemuAssignDeviceNetAlias(virDomainDefPtr def, virDomainNetDefPtr net, int idx); int qemuAssignDeviceDiskAlias(virDomainDiskDefPtr def, virBitmapPtr qemuCaps); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 84ef4dd..3599ca3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4611,6 +4611,9 @@ static char *qemuDomainXMLToNative(virConnectPtr conn, if (qemuProcessPrepareMonitorChr(driver, &monConfig, def->name) < 0) goto cleanup; + if (qemuAssignDeviceAliases(def, qemuCaps) < 0) + goto cleanup; + if (!(cmd = qemuBuildCommandLine(conn, driver, def, &monConfig, false, qemuCaps, NULL, -1, NULL, VIR_VM_OP_NO_OP))) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index a7fe86c..c0f6fd4 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3015,6 +3015,9 @@ int qemuProcessStart(virConnectPtr conn, priv->persistentAddrs = 0; } + if (qemuAssignDeviceAliases(vm->def, priv->qemuCaps) < 0) + goto cleanup; + VIR_DEBUG("Building emulator command line"); if (!(cmd = qemuBuildCommandLine(conn, driver, vm->def, priv->monConfig, priv->monJSON != 0, priv->qemuCaps, diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index a7ae925..a64f45e 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -118,6 +118,9 @@ static int testCompareXMLToArgvFiles(const char *xml, qemuCapsSet(extraFlags, QEMU_CAPS_PCI_MULTIBUS); } + if (qemuAssignDeviceAliases(vmdef, extraFlags) < 0) + goto fail; + if (!(cmd = qemuBuildCommandLine(conn, &driver, vmdef, &monitor_chr, json, extraFlags, migrateFrom, migrateFd, NULL, diff --git a/tests/qemuxmlnstest.c b/tests/qemuxmlnstest.c index 03d5b03..b13b409 100644 --- a/tests/qemuxmlnstest.c +++ b/tests/qemuxmlnstest.c @@ -118,6 +118,9 @@ static int testCompareXMLToArgvFiles(const char *xml, qemuCapsSet(extraFlags, QEMU_CAPS_PCI_MULTIBUS); } + if (qemuAssignDeviceAliases(vmdef, extraFlags) < 0) + goto fail; + if (!(cmd = qemuBuildCommandLine(conn, &driver, vmdef, &monitor_chr, json, extraFlags, migrateFrom, migrateFd, NULL, -- 1.7.3.4
On Thu, Oct 20, 2011 at 04:52:50PM +0200, Michal Privoznik wrote:
This patch is rather cosmetic as it only moves device alias assignation from command line construction just before that. However, it is needed in connotation of previous and next patch. --- src/qemu/qemu_command.c | 5 +---- src/qemu/qemu_command.h | 1 + src/qemu/qemu_driver.c | 3 +++ src/qemu/qemu_process.c | 3 +++ tests/qemuxml2argvtest.c | 3 +++ tests/qemuxmlnstest.c | 3 +++ 6 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 30c0be6..0c5bfab 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -656,7 +656,7 @@ qemuAssignDeviceControllerAlias(virDomainControllerDefPtr controller) }
-static int +int qemuAssignDeviceAliases(virDomainDefPtr def, virBitmapPtr qemuCaps) { int i; @@ -3284,9 +3284,6 @@ qemuBuildCommandLine(virConnectPtr conn, bool usblegacy = false; uname_normalize(&ut);
- if (qemuAssignDeviceAliases(def, qemuCaps) < 0) - return NULL; - virUUIDFormat(def->uuid, uuid);
emulator = def->emulator; diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 00e58a2..d34df8f 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -187,6 +187,7 @@ int qemuDomainPCIAddressReleaseSlot(qemuDomainPCIAddressSetPtr addrs, int slot); void qemuDomainPCIAddressSetFree(qemuDomainPCIAddressSetPtr addrs); int qemuAssignDevicePCISlots(virDomainDefPtr def, qemuDomainPCIAddressSetPtr addrs);
+int qemuAssignDeviceAliases(virDomainDefPtr def, virBitmapPtr qemuCaps); int qemuDomainNetVLAN(virDomainNetDefPtr def); int qemuAssignDeviceNetAlias(virDomainDefPtr def, virDomainNetDefPtr net, int idx); int qemuAssignDeviceDiskAlias(virDomainDiskDefPtr def, virBitmapPtr qemuCaps); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 84ef4dd..3599ca3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4611,6 +4611,9 @@ static char *qemuDomainXMLToNative(virConnectPtr conn, if (qemuProcessPrepareMonitorChr(driver, &monConfig, def->name) < 0) goto cleanup;
+ if (qemuAssignDeviceAliases(def, qemuCaps) < 0) + goto cleanup; + if (!(cmd = qemuBuildCommandLine(conn, driver, def, &monConfig, false, qemuCaps, NULL, -1, NULL, VIR_VM_OP_NO_OP))) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index a7fe86c..c0f6fd4 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3015,6 +3015,9 @@ int qemuProcessStart(virConnectPtr conn, priv->persistentAddrs = 0; }
+ if (qemuAssignDeviceAliases(vm->def, priv->qemuCaps) < 0) + goto cleanup; + VIR_DEBUG("Building emulator command line"); if (!(cmd = qemuBuildCommandLine(conn, driver, vm->def, priv->monConfig, priv->monJSON != 0, priv->qemuCaps, diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index a7ae925..a64f45e 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -118,6 +118,9 @@ static int testCompareXMLToArgvFiles(const char *xml, qemuCapsSet(extraFlags, QEMU_CAPS_PCI_MULTIBUS); }
+ if (qemuAssignDeviceAliases(vmdef, extraFlags) < 0) + goto fail; + if (!(cmd = qemuBuildCommandLine(conn, &driver, vmdef, &monitor_chr, json, extraFlags, migrateFrom, migrateFd, NULL, diff --git a/tests/qemuxmlnstest.c b/tests/qemuxmlnstest.c index 03d5b03..b13b409 100644 --- a/tests/qemuxmlnstest.c +++ b/tests/qemuxmlnstest.c @@ -118,6 +118,9 @@ static int testCompareXMLToArgvFiles(const char *xml, qemuCapsSet(extraFlags, QEMU_CAPS_PCI_MULTIBUS); }
+ if (qemuAssignDeviceAliases(vmdef, extraFlags) < 0) + goto fail; + if (!(cmd = qemuBuildCommandLine(conn, &driver, vmdef, &monitor_chr, json, extraFlags, migrateFrom, migrateFd, NULL,
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
This patch implements on_missing feature in qemu driver. Upon qemu startup process an accessibility of CDROMs and floppy disks is checked. The source might get dropped if unavailable and on_missing is set accordingly. No event is emit thought. Look for follow up patch. --- src/qemu/qemu_domain.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 4 +++ src/qemu/qemu_process.c | 10 +++++-- 3 files changed, 77 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index b202ba7..7fbdfa1 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1608,3 +1608,69 @@ qemuDomainSetFakeReboot(struct qemud_driver *driver, if (virDomainSaveStatus(driver->caps, driver->stateDir, vm) < 0) VIR_WARN("Failed to save status on vm %s", vm->def->name); } + +int +qemuDomainCheckDiskPresence(struct qemud_driver *driver, + virDomainObjPtr vm, + bool start_with_state) +{ + int ret = -1; + int i; + int accessRet; + virDomainDiskDefPtr disk; + char uuid[VIR_UUID_STRING_BUFLEN] ATTRIBUTE_UNUSED; + + virUUIDFormat(vm->def->uuid, uuid); + + for (i = 0; i < vm->def->ndisks; i++) { + disk = vm->def->disks[i]; + + if (!disk->startupPolicy || !disk->src) + continue; + + if ((accessRet = virFileAccessibleAs(disk->src, F_OK, + driver->user, + driver->group)) >= 0) { + /* disk accessible or virFileAccessibleAs() + * terminated with signal*/ + continue; + } + + switch ((enum virDomainStartupPolicy) disk->startupPolicy) { + case VIR_DOMAIN_STARTUP_POLICY_OPTIONAL: + break; + + case VIR_DOMAIN_STARTUP_POLICY_MANDATORY: + virReportSystemError(-accessRet, + _("cannot access file '%s'"), + disk->src); + goto cleanup; + break; + + case VIR_DOMAIN_STARTUP_POLICY_REQUISITE: + if (!start_with_state) { + virReportSystemError(-accessRet, + _("cannot access file '%s'"), + disk->src); + goto cleanup; + } + break; + + case VIR_DOMAIN_STARTUP_POLICY_DEFAULT: + case VIR_DOMAIN_STARTUP_POLICY_LAST: + /* this should never happen */ + break; + } + + VIR_DEBUG("Droping disk '%s' on domain '%s' (UUID '%s') " + "due to not accessible source '%s'", + disk->dst, vm->def->name, uuid, disk->src); + + VIR_FREE(disk->src); + } + + ret = 0; + +cleanup: + return ret; +} diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index d9f323c..e8bcab3 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -280,4 +280,8 @@ void qemuDomainSetFakeReboot(struct qemud_driver *driver, bool qemuDomainJobAllowed(qemuDomainObjPrivatePtr priv, enum qemuDomainJob job); + +int qemuDomainCheckDiskPresence(struct qemud_driver *driver, + virDomainObjPtr vm, + bool start_with_state); #endif /* __QEMU_DOMAIN_H__ */ diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index c0f6fd4..5731223 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2880,6 +2880,13 @@ int qemuProcessStart(virConnectPtr conn, NULL) < 0) goto cleanup; + if (qemuAssignDeviceAliases(vm->def, priv->qemuCaps) < 0) + goto cleanup; + + VIR_DEBUG("Checking for CDROM and floppy presence"); + if (qemuDomainCheckDiskPresence(driver, vm, migrateFrom != NULL) < 0) + goto cleanup; + /* If you are using a SecurityDriver with dynamic labelling, then generate a security label for isolation */ VIR_DEBUG("Generating domain security label (if required)"); @@ -3015,9 +3022,6 @@ int qemuProcessStart(virConnectPtr conn, priv->persistentAddrs = 0; } - if (qemuAssignDeviceAliases(vm->def, priv->qemuCaps) < 0) - goto cleanup; - VIR_DEBUG("Building emulator command line"); if (!(cmd = qemuBuildCommandLine(conn, driver, vm->def, priv->monConfig, priv->monJSON != 0, priv->qemuCaps, -- 1.7.3.4
On Thu, Oct 20, 2011 at 04:52:51PM +0200, Michal Privoznik wrote:
This patch implements on_missing feature in qemu driver. Upon qemu startup process an accessibility of CDROMs and floppy disks is checked. The source might get dropped if unavailable and on_missing is set accordingly. No event is emit thought. Look for follow up patch. --- src/qemu/qemu_domain.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 4 +++ src/qemu/qemu_process.c | 10 +++++-- 3 files changed, 77 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index b202ba7..7fbdfa1 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1608,3 +1608,69 @@ qemuDomainSetFakeReboot(struct qemud_driver *driver, if (virDomainSaveStatus(driver->caps, driver->stateDir, vm) < 0) VIR_WARN("Failed to save status on vm %s", vm->def->name); } + +int +qemuDomainCheckDiskPresence(struct qemud_driver *driver, + virDomainObjPtr vm, + bool start_with_state) +{ + int ret = -1; + int i; + int accessRet; + virDomainDiskDefPtr disk; + char uuid[VIR_UUID_STRING_BUFLEN] ATTRIBUTE_UNUSED; + + virUUIDFormat(vm->def->uuid, uuid); + + for (i = 0; i < vm->def->ndisks; i++) { + disk = vm->def->disks[i]; + + if (!disk->startupPolicy || !disk->src) + continue; + + if ((accessRet = virFileAccessibleAs(disk->src, F_OK, + driver->user, + driver->group)) >= 0) { + /* disk accessible or virFileAccessibleAs() + * terminated with signal*/ + continue; + } + + switch ((enum virDomainStartupPolicy) disk->startupPolicy) { + case VIR_DOMAIN_STARTUP_POLICY_OPTIONAL: + break; + + case VIR_DOMAIN_STARTUP_POLICY_MANDATORY: + virReportSystemError(-accessRet, + _("cannot access file '%s'"), + disk->src); + goto cleanup; + break; + + case VIR_DOMAIN_STARTUP_POLICY_REQUISITE: + if (!start_with_state) { + virReportSystemError(-accessRet, + _("cannot access file '%s'"), + disk->src); + goto cleanup; + } + break; + + case VIR_DOMAIN_STARTUP_POLICY_DEFAULT: + case VIR_DOMAIN_STARTUP_POLICY_LAST: + /* this should never happen */ + break; + } + + VIR_DEBUG("Droping disk '%s' on domain '%s' (UUID '%s') " + "due to not accessible source '%s'", + disk->dst, vm->def->name, uuid, disk->src); + + VIR_FREE(disk->src); + } + + ret = 0; + +cleanup: + return ret; +} diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index d9f323c..e8bcab3 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -280,4 +280,8 @@ void qemuDomainSetFakeReboot(struct qemud_driver *driver,
bool qemuDomainJobAllowed(qemuDomainObjPrivatePtr priv, enum qemuDomainJob job); + +int qemuDomainCheckDiskPresence(struct qemud_driver *driver, + virDomainObjPtr vm, + bool start_with_state); #endif /* __QEMU_DOMAIN_H__ */ diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index c0f6fd4..5731223 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2880,6 +2880,13 @@ int qemuProcessStart(virConnectPtr conn, NULL) < 0) goto cleanup;
+ if (qemuAssignDeviceAliases(vm->def, priv->qemuCaps) < 0) + goto cleanup; + + VIR_DEBUG("Checking for CDROM and floppy presence"); + if (qemuDomainCheckDiskPresence(driver, vm, migrateFrom != NULL) < 0) + goto cleanup; + /* If you are using a SecurityDriver with dynamic labelling, then generate a security label for isolation */ VIR_DEBUG("Generating domain security label (if required)"); @@ -3015,9 +3022,6 @@ int qemuProcessStart(virConnectPtr conn, priv->persistentAddrs = 0; }
- if (qemuAssignDeviceAliases(vm->def, priv->qemuCaps) < 0) - goto cleanup; - VIR_DEBUG("Building emulator command line"); if (!(cmd = qemuBuildCommandLine(conn, driver, vm->def, priv->monConfig, priv->monJSON != 0, priv->qemuCaps,
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
If a disk source gets dropped because it is not accessible, mgmt application might want to be informed about this. Therefore we need to emit an event. The event presented in this patch is however a bit superset of what written above. The reason is simple: an intention to be easily expanded, e.g. on 'user ejected disk in guest' events. Therefore, callback gets source string and disk alias (which should be unique among a domain) and reason (an integer); --- daemon/remote.c | 39 +++++++++++++ examples/domain-events/events-c/event-test.c | 28 +++++++++- examples/domain-events/events-python/event-test.py | 4 + include/libvirt/libvirt.h.in | 29 +++++++++ python/libvirt-override-virConnect.py | 9 +++ python/libvirt-override.c | 52 +++++++++++++++++ src/conf/domain_event.c | 61 ++++++++++++++++++++ src/conf/domain_event.h | 9 +++ src/libvirt_private.syms | 2 + src/qemu/qemu_domain.c | 7 ++ src/qemu/qemu_process.c | 14 ++-- src/remote/remote_driver.c | 35 +++++++++++ src/remote/remote_protocol.x | 10 +++- src/remote_protocol-structs | 6 ++ 14 files changed, 296 insertions(+), 9 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 550bed4..f49eb48 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -451,6 +451,44 @@ static int remoteRelayDomainEventControlError(virConnectPtr conn ATTRIBUTE_UNUSE } +static int remoteRelayDomainEventDiskEject(virConnectPtr conn ATTRIBUTE_UNUSED, + virDomainPtr dom, + const char *srcPath, + const char *devAlias, + int reason, + void *opaque) +{ + virNetServerClientPtr client = opaque; + remote_domain_event_disk_eject_msg data; + + if (!client) + return -1; + + VIR_DEBUG("Relaying domain %s %d disk eject %s %s %d", + dom->name, dom->id, srcPath, devAlias, reason); + + /* build return data */ + memset(&data, 0, sizeof data); + data.srcPath = strdup(srcPath); + data.devAlias = strdup(devAlias); + if (!data.srcPath || !data.devAlias) + goto mem_error; + data.reason = reason; + + make_nonnull_domain(&data.dom, dom); + + remoteDispatchDomainEventSend(client, remoteProgram, + REMOTE_PROC_DOMAIN_EVENT_DISK_EJECT, + (xdrproc_t)xdr_remote_domain_event_disk_eject_msg, &data); + + return 0; + +mem_error: + virReportOOMError(); + return -1; +} + + static virConnectDomainEventGenericCallback domainEventCallbacks[] = { VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventLifecycle), VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventReboot), @@ -461,6 +499,7 @@ static virConnectDomainEventGenericCallback domainEventCallbacks[] = { VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventIOErrorReason), VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventControlError), VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventBlockJob), + VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventDiskEject), }; verify(ARRAY_CARDINALITY(domainEventCallbacks) == VIR_DOMAIN_EVENT_ID_LAST); diff --git a/examples/domain-events/events-c/event-test.c b/examples/domain-events/events-c/event-test.c index 6a3ed26..8d1b23f 100644 --- a/examples/domain-events/events-c/event-test.c +++ b/examples/domain-events/events-c/event-test.c @@ -285,6 +285,24 @@ static int myDomainEventControlErrorCallback(virConnectPtr conn ATTRIBUTE_UNUSED } +const char *diskEjectReasonStrings[] = { + "startupPolicy", /* 0 */ + /* add new reason here */ +}; +static int myDomainEventDiskEjectCallback(virConnectPtr conn ATTRIBUTE_UNUSED, + virDomainPtr dom, + const char *srcPath, + const char *devAlias, + int reason, + void *opaque ATTRIBUTE_UNUSED) +{ + printf("%s EVENT: Domain %s(%d) disk eject srcPath: %s devAlias: %s reason: %s\n", + __func__, virDomainGetName(dom), virDomainGetID(dom), + srcPath, devAlias, diskEjectReasonStrings[reason]); + return 0; +} + + static void myFreeFunc(void *opaque) { char *str = opaque; @@ -319,6 +337,7 @@ int main(int argc, char **argv) int callback6ret = -1; int callback7ret = -1; int callback8ret = -1; + int callback9ret = -1; struct sigaction action_stop; memset(&action_stop, 0, sizeof action_stop); @@ -382,6 +401,11 @@ int main(int argc, char **argv) VIR_DOMAIN_EVENT_ID_CONTROL_ERROR, VIR_DOMAIN_EVENT_CALLBACK(myDomainEventControlErrorCallback), strdup("callback control error"), myFreeFunc); + callback9ret = virConnectDomainEventRegisterAny(dconn, + NULL, + VIR_DOMAIN_EVENT_ID_DISK_EJECT, + VIR_DOMAIN_EVENT_CALLBACK(myDomainEventDiskEjectCallback), + strdup("disk eject"), myFreeFunc); if ((callback1ret != -1) && (callback2ret != -1) && @@ -389,7 +413,8 @@ int main(int argc, char **argv) (callback4ret != -1) && (callback5ret != -1) && (callback6ret != -1) && - (callback7ret != -1)) { + (callback7ret != -1) && + (callback9ret != -1)) { while (run) { if (virEventRunDefaultImpl() < 0) { virErrorPtr err = virGetLastError(); @@ -406,6 +431,7 @@ int main(int argc, char **argv) virConnectDomainEventDeregisterAny(dconn, callback5ret); virConnectDomainEventDeregisterAny(dconn, callback6ret); virConnectDomainEventDeregisterAny(dconn, callback7ret); + virConnectDomainEventDeregisterAny(dconn, callback9ret); if (callback8ret != -1) virConnectDomainEventDeregisterAny(dconn, callback8ret); } diff --git a/examples/domain-events/events-python/event-test.py b/examples/domain-events/events-python/event-test.py index 4df9b98..b7d3d6e 100644 --- a/examples/domain-events/events-python/event-test.py +++ b/examples/domain-events/events-python/event-test.py @@ -469,6 +469,9 @@ def myDomainEventIOErrorCallback(conn, dom, srcpath, devalias, action, opaque): def myDomainEventGraphicsCallback(conn, dom, phase, localAddr, remoteAddr, authScheme, subject, opaque): print "myDomainEventGraphicsCallback: Domain %s(%s) %d %s" % (dom.name(), dom.ID(), phase, authScheme) +def myDomainEventDiskEjectCallback(conn, dom, srcPath, devAlias, reason, opaque): + print "myDomainEventDiskEjectCallback: Domain %s(%s) disk eject srcPath: %s devAlias: %s reason: %s" % ( + dom.name(), dom.ID(), srcPath, devAlias, reason) def usage(out=sys.stderr): print >>out, "usage: "+os.path.basename(sys.argv[0])+" [-hdl] [uri]" print >>out, " uri will default to qemu:///system" @@ -526,6 +529,7 @@ def main(): vc.domainEventRegisterAny(None, libvirt.VIR_DOMAIN_EVENT_ID_IO_ERROR, myDomainEventIOErrorCallback, None) vc.domainEventRegisterAny(None, libvirt.VIR_DOMAIN_EVENT_ID_WATCHDOG, myDomainEventWatchdogCallback, None) vc.domainEventRegisterAny(None, libvirt.VIR_DOMAIN_EVENT_ID_GRAPHICS, myDomainEventGraphicsCallback, None) + vc.domainEventRegisterAny(None, libvirt.VIR_DOMAIN_EVENT_ID_DISK_EJECT, myDomainEventDiskEjectCallback, None) # The rest of your app would go here normally, but for sake # of demo we'll just go to sleep. The other option is to diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 361881a..66b718c 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2996,6 +2996,34 @@ typedef void (*virConnectDomainEventBlockJobCallback)(virConnectPtr conn, void *opaque); /** + * virConnectDomainEventDiskEjectReason: + * + * The reason describing why this callback is called + */ +typedef enum { + VIR_DOMAIN_DISK_EJECT_MISSING_ON_START = 0, +} virConnectDomainEventDiskEjectReason; + +/** + * virConnectDomainEventDiskEjectCallback: + * @conn: connection object + * @dom: domain on which the event occurred + * @target: target which changed status + * @reason: reason why this callback was called; any of + * virConnectDomainEventDiskEjectReason + * @opaque: application specified data + * + * The callback signature to use when registering for an event of type + * VIR_DOMAIN_EVENT_ID_IO_ERROR with virConnectDomainEventRegisterAny() + */ +typedef void (*virConnectDomainEventDiskEjectCallback)(virConnectPtr conn, + virDomainPtr dom, + const char *srcPath, + const char *devAlias, + int reason, + void *opaque); + +/** * VIR_DOMAIN_EVENT_CALLBACK: * * Used to cast the event specific callback into the generic one @@ -3014,6 +3042,7 @@ typedef enum { VIR_DOMAIN_EVENT_ID_IO_ERROR_REASON = 6, /* virConnectDomainEventIOErrorReasonCallback */ VIR_DOMAIN_EVENT_ID_CONTROL_ERROR = 7, /* virConnectDomainEventGenericCallback */ VIR_DOMAIN_EVENT_ID_BLOCK_JOB = 8, /* virConnectDomainEventBlockJobCallback */ + VIR_DOMAIN_EVENT_ID_DISK_EJECT = 9, /* virConnectDomainEventDiskEjectCallback */ /* * NB: this enum value will increase over time as new events are diff --git a/python/libvirt-override-virConnect.py b/python/libvirt-override-virConnect.py index 65b5342..504fef2 100644 --- a/python/libvirt-override-virConnect.py +++ b/python/libvirt-override-virConnect.py @@ -125,6 +125,15 @@ except AttributeError: pass + def _dispatchDomainEventDiskEjectCallback(self, dom, srcPath, devAlias, reason, cbData): + """Dispatches event to python user domain diskEject event callbacks + """ + cb = cbData["cb"] + opaque = cbData["opaque"] + + cb(self, virDomain(self, _obj=dom), srcPath, devAlias, reason, opaque) + return 0; + def domainEventDeregisterAny(self, callbackID): """Removes a Domain Event Callback. De-registering for a domain callback will disable delivery of this event type """ diff --git a/python/libvirt-override.c b/python/libvirt-override.c index 523c03b..7317b9e 100644 --- a/python/libvirt-override.c +++ b/python/libvirt-override.c @@ -4374,6 +4374,55 @@ libvirt_virConnectDomainEventBlockJobCallback(virConnectPtr conn ATTRIBUTE_UNUSE return ret; } +static int +libvirt_virConnectDomainEventDiskEjectCallback(virConnectPtr conn ATTRIBUTE_UNUSED, + virDomainPtr dom, + const char *srcPath, + const char *devAlias, + int reason, + void *opaque) +{ + PyObject *pyobj_cbData = (PyObject*)opaque; + PyObject *pyobj_dom; + PyObject *pyobj_ret; + PyObject *pyobj_conn; + PyObject *dictKey; + int ret = -1; + + LIBVIRT_ENSURE_THREAD_STATE; + /* Create a python instance of this virDomainPtr */ + virDomainRef(dom); + + pyobj_dom = libvirt_virDomainPtrWrap(dom); + Py_INCREF(pyobj_cbData); + + dictKey = libvirt_constcharPtrWrap("conn"); + pyobj_conn = PyDict_GetItem(pyobj_cbData, dictKey); + Py_DECREF(dictKey); + + /* Call the Callback Dispatcher */ + pyobj_ret = PyObject_CallMethod(pyobj_conn, + (char*)"_dispatchDomainEventDiskEjectCallback", + (char*)"OssiO", + pyobj_dom, + srcPath, devAlias, + reason, pyobj_cbData); + + Py_DECREF(pyobj_cbData); + Py_DECREF(pyobj_dom); + + if(!pyobj_ret) { + DEBUG("%s - ret:%p\n", __FUNCTION__, pyobj_ret); + PyErr_Print(); + } else { + Py_DECREF(pyobj_ret); + ret = 0; + } + + LIBVIRT_RELEASE_THREAD_STATE; + return ret; +} + static PyObject * libvirt_virConnectDomainEventRegisterAny(ATTRIBUTE_UNUSED PyObject * self, PyObject * args) @@ -4431,6 +4480,9 @@ libvirt_virConnectDomainEventRegisterAny(ATTRIBUTE_UNUSED PyObject * self, case VIR_DOMAIN_EVENT_ID_BLOCK_JOB: cb = VIR_DOMAIN_EVENT_CALLBACK(libvirt_virConnectDomainEventBlockJobCallback); break; + case VIR_DOMAIN_EVENT_ID_DISK_EJECT: + cb = VIR_DOMAIN_EVENT_CALLBACK(libvirt_virConnectDomainEventDiskEjectCallback); + break; } if (!cb) { diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c index f712c34..a85a7ee 100644 --- a/src/conf/domain_event.c +++ b/src/conf/domain_event.c @@ -88,6 +88,11 @@ struct _virDomainEvent { int type; int status; } blockJob; + struct { + char *srcPath; + char *devAlias; + int reason; + } diskEject; } data; }; @@ -509,6 +514,11 @@ void virDomainEventFree(virDomainEventPtr event) case VIR_DOMAIN_EVENT_ID_BLOCK_JOB: VIR_FREE(event->data.blockJob.path); break; + + case VIR_DOMAIN_EVENT_ID_DISK_EJECT: + VIR_FREE(event->data.diskEject.srcPath); + VIR_FREE(event->data.diskEject.devAlias); + break; } VIR_FREE(event->dom.name); @@ -961,6 +971,49 @@ virDomainEventPtr virDomainEventControlErrorNewFromObj(virDomainObjPtr obj) return ev; } +static virDomainEventPtr +virDomainEventDiskEjectNew(int id, const char *name, + unsigned char *uuid, const char *srcPath, + const char *devAlias, int reason) +{ + virDomainEventPtr ev = + virDomainEventNewInternal(VIR_DOMAIN_EVENT_ID_DISK_EJECT, + id, name, uuid); + + if (ev) { + if (!(ev->data.diskEject.srcPath = strdup(srcPath)) || + !(ev->data.diskEject.devAlias = strdup(devAlias))) + goto error; + + ev->data.diskEject.reason = reason; + } + + return ev; + +error: + virReportOOMError(); + virDomainEventFree(ev); + return NULL; +} + +virDomainEventPtr virDomainEventDiskEjectNewFromObj(virDomainObjPtr obj, + const char *srcPath, + const char *devAlias, + int reason) +{ + return virDomainEventDiskEjectNew(obj->def->id, obj->def->name, + obj->def->uuid, srcPath, + devAlias, reason); +} + +virDomainEventPtr virDomainEventDiskEjectNewFromDom(virDomainPtr dom, + const char *srcPath, + const char *devAlias, + int reason) +{ + return virDomainEventDiskEjectNew(dom->id, dom->name, dom->uuid, + srcPath, devAlias, reason); +} /** * virDomainEventQueuePop: @@ -1104,6 +1157,14 @@ void virDomainEventDispatchDefaultFunc(virConnectPtr conn, cbopaque); break; + case VIR_DOMAIN_EVENT_ID_DISK_EJECT: + ((virConnectDomainEventDiskEjectCallback)cb)(conn, dom, + event->data.diskEject.srcPath, + event->data.diskEject.devAlias, + event->data.diskEject.reason, + cbopaque); + break; + default: VIR_WARN("Unexpected event ID %d", event->eventID); break; diff --git a/src/conf/domain_event.h b/src/conf/domain_event.h index 08930ed..e8aaef9 100644 --- a/src/conf/domain_event.h +++ b/src/conf/domain_event.h @@ -179,6 +179,15 @@ virDomainEventPtr virDomainEventBlockJobNewFromDom(virDomainPtr dom, int type, int status); +virDomainEventPtr virDomainEventDiskEjectNewFromObj(virDomainObjPtr obj, + const char *srcPath, + const char *devAlias, + int reason); +virDomainEventPtr virDomainEventDiskEjectNewFromDom(virDomainPtr dom, + const char *srcPath, + const char *devAlias, + int reason); + int virDomainEventQueuePush(virDomainEventQueuePtr evtQueue, virDomainEventPtr event); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b83fd3f..a4e49b0 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -478,6 +478,8 @@ virDomainEventCallbackListRemoveConn; virDomainEventCallbackListRemoveID; virDomainEventControlErrorNewFromDom; virDomainEventControlErrorNewFromObj; +virDomainEventDiskEjectNewFromDom; +virDomainEventDiskEjectNewFromObj; virDomainEventDispatch; virDomainEventDispatchDefaultFunc; virDomainEventFree; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 7fbdfa1..3e2a237 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -35,6 +35,7 @@ #include "ignore-value.h" #include "uuid.h" #include "virfile.h" +#include "domain_event.h" #include <sys/time.h> #include <fcntl.h> @@ -1619,6 +1620,7 @@ qemuDomainCheckDiskPresence(struct qemud_driver *driver, int accessRet; virDomainDiskDefPtr disk; char uuid[VIR_UUID_STRING_BUFLEN] ATTRIBUTE_UNUSED; + virDomainEventPtr event = NULL; virUUIDFormat(vm->def->uuid, uuid); @@ -1666,6 +1668,11 @@ qemuDomainCheckDiskPresence(struct qemud_driver *driver, "due to not accessible source '%s'", disk->dst, vm->def->name, uuid, disk->src); + event = virDomainEventDiskEjectNewFromObj(vm, disk->src, disk->info.alias, + VIR_DOMAIN_DISK_EJECT_MISSING_ON_START); + if (event) + qemuDomainEventQueue(driver, event); + VIR_FREE(disk->src); } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 5731223..18c98c6 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2880,13 +2880,6 @@ int qemuProcessStart(virConnectPtr conn, NULL) < 0) goto cleanup; - if (qemuAssignDeviceAliases(vm->def, priv->qemuCaps) < 0) - goto cleanup; - - VIR_DEBUG("Checking for CDROM and floppy presence"); - if (qemuDomainCheckDiskPresence(driver, vm, migrateFrom != NULL) < 0) - goto cleanup; - /* If you are using a SecurityDriver with dynamic labelling, then generate a security label for isolation */ VIR_DEBUG("Generating domain security label (if required)"); @@ -2956,6 +2949,13 @@ int qemuProcessStart(virConnectPtr conn, &priv->qemuCaps) < 0) goto cleanup; + if (qemuAssignDeviceAliases(vm->def, priv->qemuCaps) < 0) + goto cleanup; + + VIR_DEBUG("Checking for CDROM and floppy presence"); + if (qemuDomainCheckDiskPresence(driver, vm, migrateFrom != NULL) < 0) + goto cleanup; + VIR_DEBUG("Setting up domain cgroup (if required)"); if (qemuSetupCgroup(driver, vm) < 0) goto cleanup; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 1dea327..fa2ed57 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -228,6 +228,11 @@ remoteDomainBuildEventBlockJob(virNetClientProgramPtr prog, virNetClientPtr client, void *evdata, void *opaque); +static void +remoteDomainBuildEventDiskEject(virNetClientProgramPtr prog, + virNetClientPtr client, + void *evdata, void *opaque); + static virNetClientProgramEvent remoteDomainEvents[] = { { REMOTE_PROC_DOMAIN_EVENT_RTC_CHANGE, remoteDomainBuildEventRTCChange, @@ -265,6 +270,10 @@ static virNetClientProgramEvent remoteDomainEvents[] = { remoteDomainBuildEventBlockJob, sizeof(remote_domain_event_block_job_msg), (xdrproc_t)xdr_remote_domain_event_block_job_msg }, + { REMOTE_PROC_DOMAIN_EVENT_DISK_EJECT, + remoteDomainBuildEventDiskEject, + sizeof(remote_domain_event_disk_eject_msg), + (xdrproc_t)xdr_remote_domain_event_disk_eject_msg }, }; enum virDrvOpenRemoteFlags { @@ -3327,6 +3336,32 @@ remoteDomainBuildEventControlError(virNetClientProgramPtr prog ATTRIBUTE_UNUSED, } +static void +remoteDomainBuildEventDiskEject(virNetClientProgramPtr prog ATTRIBUTE_UNUSED, + virNetClientPtr client ATTRIBUTE_UNUSED, + void *evdata, void *opaque) +{ + virConnectPtr conn = opaque; + struct private_data *priv = conn->privateData; + remote_domain_event_disk_eject_msg *msg = evdata; + virDomainPtr dom; + virDomainEventPtr event = NULL; + + dom = get_nonnull_domain(conn, msg->dom); + if (!dom) + return; + + event = virDomainEventDiskEjectNewFromDom(dom, + msg->srcPath, + msg->devAlias, + msg->reason); + + virDomainFree(dom); + + remoteDomainEventQueue(priv, event); +} + + static virDrvOpenStatus ATTRIBUTE_NONNULL (1) remoteSecretOpen(virConnectPtr conn, virConnectAuthPtr auth, unsigned int flags) diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index f95253e..63c4581 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -2010,6 +2010,13 @@ struct remote_domain_event_block_job_msg { int status; }; +struct remote_domain_event_disk_eject_msg { + remote_nonnull_domain dom; + remote_nonnull_string srcPath; + remote_nonnull_string devAlias; + int reason; +}; + struct remote_domain_managed_save_args { remote_nonnull_domain dom; unsigned int flags; @@ -2546,7 +2553,8 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_SNAPSHOT_GET_PARENT = 244, /* autogen autogen priority:high */ REMOTE_PROC_DOMAIN_RESET = 245, /* autogen autogen */ REMOTE_PROC_DOMAIN_SNAPSHOT_NUM_CHILDREN = 246, /* autogen autogen priority:high */ - REMOTE_PROC_DOMAIN_SNAPSHOT_LIST_CHILDREN_NAMES = 247 /* autogen autogen priority:high */ + REMOTE_PROC_DOMAIN_SNAPSHOT_LIST_CHILDREN_NAMES = 247, /* autogen autogen priority:high */ + REMOTE_PROC_DOMAIN_EVENT_DISK_EJECT = 248 /* skipgen skipgen */ /* * Notice how the entries are grouped in sets of 10 ? diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index 7894441..06a237e 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -1509,6 +1509,12 @@ struct remote_domain_event_block_job_msg { int type; int status; }; +struct remote_domain_event_disk_eject_msg { + remote_nonnull_domain dom; + remote_nonnull_string srcPath; + remote_nonnull_string devAlias; + int reason; +}; struct remote_domain_managed_save_args { remote_nonnull_domain dom; u_int flags; -- 1.7.3.4
On Thu, Oct 20, 2011 at 04:52:52PM +0200, Michal Privoznik wrote:
If a disk source gets dropped because it is not accessible, mgmt application might want to be informed about this. Therefore we need to emit an event. The event presented in this patch is however a bit superset of what written above. The reason is simple: an intention to be easily expanded, e.g. on 'user ejected disk in guest' events. Therefore, callback gets source string and disk alias (which should be unique among a domain) and reason (an integer);
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 361881a..66b718c 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2996,6 +2996,34 @@ typedef void (*virConnectDomainEventBlockJobCallback)(virConnectPtr conn, void *opaque);
/** + * virConnectDomainEventDiskEjectReason: + * + * The reason describing why this callback is called + */ +typedef enum { + VIR_DOMAIN_DISK_EJECT_MISSING_ON_START = 0, +} virConnectDomainEventDiskEjectReason; + +/** + * virConnectDomainEventDiskEjectCallback: + * @conn: connection object + * @dom: domain on which the event occurred + * @target: target which changed status + * @reason: reason why this callback was called; any of + * virConnectDomainEventDiskEjectReason + * @opaque: application specified data + * + * The callback signature to use when registering for an event of type + * VIR_DOMAIN_EVENT_ID_IO_ERROR with virConnectDomainEventRegisterAny() + */ +typedef void (*virConnectDomainEventDiskEjectCallback)(virConnectPtr conn, + virDomainPtr dom, + const char *srcPath, + const char *devAlias, + int reason, + void *opaque);
It suddenly occurs to me that this event will also be relevant if we insert new media. So instead of 'srcPath', we should have 'oldSrcPath' and 'newSrcPath', either one of which may be NULL depending on the scenarios involved. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Thu, Oct 20, 2011 at 16:32:55 +0100, Daniel P. Berrange wrote:
On Thu, Oct 20, 2011 at 04:52:52PM +0200, Michal Privoznik wrote: ...
+/** + * virConnectDomainEventDiskEjectCallback: + * @conn: connection object + * @dom: domain on which the event occurred + * @target: target which changed status + * @reason: reason why this callback was called; any of + * virConnectDomainEventDiskEjectReason + * @opaque: application specified data + * + * The callback signature to use when registering for an event of type + * VIR_DOMAIN_EVENT_ID_IO_ERROR with virConnectDomainEventRegisterAny() + */ +typedef void (*virConnectDomainEventDiskEjectCallback)(virConnectPtr conn, + virDomainPtr dom, + const char *srcPath, + const char *devAlias, + int reason, + void *opaque);
It suddenly occurs to me that this event will also be relevant if we insert new media. So instead of 'srcPath', we should have 'oldSrcPath' and 'newSrcPath', either one of which may be NULL depending on the scenarios involved.
In that case I think we should call the event DiskChange instead of DiskEject. Jirka
On Fri, Oct 21, 2011 at 08:48:48AM +0200, Jiri Denemark wrote:
On Thu, Oct 20, 2011 at 16:32:55 +0100, Daniel P. Berrange wrote:
On Thu, Oct 20, 2011 at 04:52:52PM +0200, Michal Privoznik wrote: ...
+/** + * virConnectDomainEventDiskEjectCallback: + * @conn: connection object + * @dom: domain on which the event occurred + * @target: target which changed status + * @reason: reason why this callback was called; any of + * virConnectDomainEventDiskEjectReason + * @opaque: application specified data + * + * The callback signature to use when registering for an event of type + * VIR_DOMAIN_EVENT_ID_IO_ERROR with virConnectDomainEventRegisterAny() + */ +typedef void (*virConnectDomainEventDiskEjectCallback)(virConnectPtr conn, + virDomainPtr dom, + const char *srcPath, + const char *devAlias, + int reason, + void *opaque);
It suddenly occurs to me that this event will also be relevant if we insert new media. So instead of 'srcPath', we should have 'oldSrcPath' and 'newSrcPath', either one of which may be NULL depending on the scenarios involved.
In that case I think we should call the event DiskChange instead of DiskEject.
Agreed, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
If a disk source gets dropped because it is not accessible, mgmt application might want to be informed about this. Therefore we need to emit an event. The event presented in this patch is however a bit superset of what written above. The reason is simple: an intention to be easily expanded, e.g. on 'user ejected disk in guest' events. Therefore, callback gets source string and disk alias (which should be unique among a domain) and reason (an integer); --- daemon/remote.c | 51 +++++++++++++ examples/domain-events/events-c/event-test.c | 29 +++++++- examples/domain-events/events-python/event-test.py | 4 + include/libvirt/libvirt.h.in | 36 +++++++++ python/libvirt-override-virConnect.py | 9 +++ python/libvirt-override.c | 53 ++++++++++++++ src/conf/domain_event.c | 76 ++++++++++++++++++++ src/conf/domain_event.h | 11 +++ src/libvirt_private.syms | 2 + src/qemu/qemu_domain.c | 7 ++ src/qemu/qemu_process.c | 14 ++-- src/remote/remote_driver.c | 36 +++++++++ src/remote/remote_protocol.x | 11 +++- src/remote_protocol-structs | 7 ++ 14 files changed, 337 insertions(+), 9 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 550bed4..9d70163 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -451,6 +451,56 @@ static int remoteRelayDomainEventControlError(virConnectPtr conn ATTRIBUTE_UNUSE } +static int remoteRelayDomainEventDiskChange(virConnectPtr conn ATTRIBUTE_UNUSED, + virDomainPtr dom, + const char *oldSrcPath, + const char *newSrcPath, + const char *devAlias, + int reason, + void *opaque) +{ + virNetServerClientPtr client = opaque; + remote_domain_event_disk_change_msg data; + char **oldSrcPath_p = NULL, **newSrcPath_p = NULL; + + if (!client) + return -1; + + VIR_DEBUG("Relaying domain %s %d disk change %s %s %s %d", + dom->name, dom->id, oldSrcPath, newSrcPath, devAlias, reason); + + /* build return data */ + memset(&data, 0, sizeof data); + if (oldSrcPath && + ((VIR_ALLOC(oldSrcPath_p) < 0) || + !(*oldSrcPath_p = strdup(oldSrcPath)))) + goto mem_error; + + if (newSrcPath && + ((VIR_ALLOC(newSrcPath_p) < 0) || + !(*newSrcPath_p = strdup(newSrcPath)))) + goto mem_error; + + data.oldSrcPath = oldSrcPath_p; + data.newSrcPath = newSrcPath_p; + if (!(data.devAlias = strdup(devAlias))) + goto mem_error; + data.reason = reason; + + make_nonnull_domain(&data.dom, dom); + + remoteDispatchDomainEventSend(client, remoteProgram, + REMOTE_PROC_DOMAIN_EVENT_DISK_CHANGE, + (xdrproc_t)xdr_remote_domain_event_disk_change_msg, &data); + + return 0; + +mem_error: + virReportOOMError(); + return -1; +} + + static virConnectDomainEventGenericCallback domainEventCallbacks[] = { VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventLifecycle), VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventReboot), @@ -461,6 +511,7 @@ static virConnectDomainEventGenericCallback domainEventCallbacks[] = { VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventIOErrorReason), VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventControlError), VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventBlockJob), + VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventDiskChange), }; verify(ARRAY_CARDINALITY(domainEventCallbacks) == VIR_DOMAIN_EVENT_ID_LAST); diff --git a/examples/domain-events/events-c/event-test.c b/examples/domain-events/events-c/event-test.c index 6a3ed26..7c99222 100644 --- a/examples/domain-events/events-c/event-test.c +++ b/examples/domain-events/events-c/event-test.c @@ -285,6 +285,25 @@ static int myDomainEventControlErrorCallback(virConnectPtr conn ATTRIBUTE_UNUSED } +const char *diskChangeReasonStrings[] = { + "startupPolicy", /* 0 */ + /* add new reason here */ +}; +static int myDomainEventDiskChangeCallback(virConnectPtr conn ATTRIBUTE_UNUSED, + virDomainPtr dom, + const char *oldSrcPath, + const char *newSrcPath, + const char *devAlias, + int reason, + void *opaque ATTRIBUTE_UNUSED) +{ + printf("%s EVENT: Domain %s(%d) disk change oldSrcPath: %s newSrcPath: %s devAlias: %s reason: %s\n", + __func__, virDomainGetName(dom), virDomainGetID(dom), + oldSrcPath, newSrcPath, devAlias, diskChangeReasonStrings[reason]); + return 0; +} + + static void myFreeFunc(void *opaque) { char *str = opaque; @@ -319,6 +338,7 @@ int main(int argc, char **argv) int callback6ret = -1; int callback7ret = -1; int callback8ret = -1; + int callback9ret = -1; struct sigaction action_stop; memset(&action_stop, 0, sizeof action_stop); @@ -382,6 +402,11 @@ int main(int argc, char **argv) VIR_DOMAIN_EVENT_ID_CONTROL_ERROR, VIR_DOMAIN_EVENT_CALLBACK(myDomainEventControlErrorCallback), strdup("callback control error"), myFreeFunc); + callback9ret = virConnectDomainEventRegisterAny(dconn, + NULL, + VIR_DOMAIN_EVENT_ID_DISK_CHANGE, + VIR_DOMAIN_EVENT_CALLBACK(myDomainEventDiskChangeCallback), + strdup("disk change"), myFreeFunc); if ((callback1ret != -1) && (callback2ret != -1) && @@ -389,7 +414,8 @@ int main(int argc, char **argv) (callback4ret != -1) && (callback5ret != -1) && (callback6ret != -1) && - (callback7ret != -1)) { + (callback7ret != -1) && + (callback9ret != -1)) { while (run) { if (virEventRunDefaultImpl() < 0) { virErrorPtr err = virGetLastError(); @@ -406,6 +432,7 @@ int main(int argc, char **argv) virConnectDomainEventDeregisterAny(dconn, callback5ret); virConnectDomainEventDeregisterAny(dconn, callback6ret); virConnectDomainEventDeregisterAny(dconn, callback7ret); + virConnectDomainEventDeregisterAny(dconn, callback9ret); if (callback8ret != -1) virConnectDomainEventDeregisterAny(dconn, callback8ret); } diff --git a/examples/domain-events/events-python/event-test.py b/examples/domain-events/events-python/event-test.py index 4df9b98..9628f6e 100644 --- a/examples/domain-events/events-python/event-test.py +++ b/examples/domain-events/events-python/event-test.py @@ -469,6 +469,9 @@ def myDomainEventIOErrorCallback(conn, dom, srcpath, devalias, action, opaque): def myDomainEventGraphicsCallback(conn, dom, phase, localAddr, remoteAddr, authScheme, subject, opaque): print "myDomainEventGraphicsCallback: Domain %s(%s) %d %s" % (dom.name(), dom.ID(), phase, authScheme) +def myDomainEventDiskChangeCallback(conn, dom, oldSrcPath, newSrcPath, devAlias, reason, opaque): + print "myDomainEventDiskChangeCallback: Domain %s(%s) disk change oldSrcPath: %s newSrcPath: %s devAlias: %s reason: %s" % ( + dom.name(), dom.ID(), oldSrcPath, newSrcPath, devAlias, reason) def usage(out=sys.stderr): print >>out, "usage: "+os.path.basename(sys.argv[0])+" [-hdl] [uri]" print >>out, " uri will default to qemu:///system" @@ -526,6 +529,7 @@ def main(): vc.domainEventRegisterAny(None, libvirt.VIR_DOMAIN_EVENT_ID_IO_ERROR, myDomainEventIOErrorCallback, None) vc.domainEventRegisterAny(None, libvirt.VIR_DOMAIN_EVENT_ID_WATCHDOG, myDomainEventWatchdogCallback, None) vc.domainEventRegisterAny(None, libvirt.VIR_DOMAIN_EVENT_ID_GRAPHICS, myDomainEventGraphicsCallback, None) + vc.domainEventRegisterAny(None, libvirt.VIR_DOMAIN_EVENT_ID_DISK_CHANGE, myDomainEventDiskChangeCallback, None) # The rest of your app would go here normally, but for sake # of demo we'll just go to sleep. The other option is to diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 361881a..e036060 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2996,6 +2996,41 @@ typedef void (*virConnectDomainEventBlockJobCallback)(virConnectPtr conn, void *opaque); /** + * virConnectDomainEventDisChangeReason: + * + * The reason describing why this callback is called + */ +typedef enum { + VIR_DOMAIN_DISK_CHANGE_MISSING_ON_START = 0, /* oldSrcPath is set */ +} virConnectDomainEventDiskChangeReason; + +/** + * virConnectDomainEventDiskChangeCallback: + * @conn: connection object + * @dom: domain on which the event occurred + * @oldSrcPath: old source path + * @newSrcPath: new source path + * @reason: reason why this callback was called; any of + * virConnectDomainEventDiskChangeReason + * @opaque: application specified data + * + * This callback occurs when disk gets changed. However, + * not all @reason mush cause both @oldSrcPath and @newSrcPath + * to be non-NULL. Please see virConnectDomainEventDiskChangeReason + * for more details. + * + * The callback signature to use when registering for an event of type + * VIR_DOMAIN_EVENT_ID_IO_ERROR with virConnectDomainEventRegisterAny() + */ +typedef void (*virConnectDomainEventDiskChangeCallback)(virConnectPtr conn, + virDomainPtr dom, + const char *oldSrcPath, + const char *newSrcPath, + const char *devAlias, + int reason, + void *opaque); + +/** * VIR_DOMAIN_EVENT_CALLBACK: * * Used to cast the event specific callback into the generic one @@ -3014,6 +3049,7 @@ typedef enum { VIR_DOMAIN_EVENT_ID_IO_ERROR_REASON = 6, /* virConnectDomainEventIOErrorReasonCallback */ VIR_DOMAIN_EVENT_ID_CONTROL_ERROR = 7, /* virConnectDomainEventGenericCallback */ VIR_DOMAIN_EVENT_ID_BLOCK_JOB = 8, /* virConnectDomainEventBlockJobCallback */ + VIR_DOMAIN_EVENT_ID_DISK_CHANGE = 9, /* virConnectDomainEventDiskChangeCallback */ /* * NB: this enum value will increase over time as new events are diff --git a/python/libvirt-override-virConnect.py b/python/libvirt-override-virConnect.py index 65b5342..b908b32 100644 --- a/python/libvirt-override-virConnect.py +++ b/python/libvirt-override-virConnect.py @@ -125,6 +125,15 @@ except AttributeError: pass + def _dispatchDomainEventDiskChangeCallback(self, dom, oldSrcPath, newSrcPath, devAlias, reason, cbData): + """Dispatches event to python user domain diskChange event callbacks + """ + cb = cbData["cb"] + opaque = cbData["opaque"] + + cb(self, virDomain(self, _obj=dom), oldSrcPath, newSrcPath, devAlias, reason, opaque) + return 0; + def domainEventDeregisterAny(self, callbackID): """Removes a Domain Event Callback. De-registering for a domain callback will disable delivery of this event type """ diff --git a/python/libvirt-override.c b/python/libvirt-override.c index 523c03b..1759bae 100644 --- a/python/libvirt-override.c +++ b/python/libvirt-override.c @@ -4374,6 +4374,56 @@ libvirt_virConnectDomainEventBlockJobCallback(virConnectPtr conn ATTRIBUTE_UNUSE return ret; } +static int +libvirt_virConnectDomainEventDiskChangeCallback(virConnectPtr conn ATTRIBUTE_UNUSED, + virDomainPtr dom, + const char *oldSrcPath, + const char *newSrcPath, + const char *devAlias, + int reason, + void *opaque) +{ + PyObject *pyobj_cbData = (PyObject*)opaque; + PyObject *pyobj_dom; + PyObject *pyobj_ret; + PyObject *pyobj_conn; + PyObject *dictKey; + int ret = -1; + + LIBVIRT_ENSURE_THREAD_STATE; + /* Create a python instance of this virDomainPtr */ + virDomainRef(dom); + + pyobj_dom = libvirt_virDomainPtrWrap(dom); + Py_INCREF(pyobj_cbData); + + dictKey = libvirt_constcharPtrWrap("conn"); + pyobj_conn = PyDict_GetItem(pyobj_cbData, dictKey); + Py_DECREF(dictKey); + + /* Call the Callback Dispatcher */ + pyobj_ret = PyObject_CallMethod(pyobj_conn, + (char*)"_dispatchDomainEventDiskChangeCallback", + (char*)"OsssiO", + pyobj_dom, + oldSrcPath, newSrcPath, + devAlias, reason, pyobj_cbData); + + Py_DECREF(pyobj_cbData); + Py_DECREF(pyobj_dom); + + if(!pyobj_ret) { + DEBUG("%s - ret:%p\n", __FUNCTION__, pyobj_ret); + PyErr_Print(); + } else { + Py_DECREF(pyobj_ret); + ret = 0; + } + + LIBVIRT_RELEASE_THREAD_STATE; + return ret; +} + static PyObject * libvirt_virConnectDomainEventRegisterAny(ATTRIBUTE_UNUSED PyObject * self, PyObject * args) @@ -4431,6 +4481,9 @@ libvirt_virConnectDomainEventRegisterAny(ATTRIBUTE_UNUSED PyObject * self, case VIR_DOMAIN_EVENT_ID_BLOCK_JOB: cb = VIR_DOMAIN_EVENT_CALLBACK(libvirt_virConnectDomainEventBlockJobCallback); break; + case VIR_DOMAIN_EVENT_ID_DISK_CHANGE: + cb = VIR_DOMAIN_EVENT_CALLBACK(libvirt_virConnectDomainEventDiskChangeCallback); + break; } if (!cb) { diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c index f712c34..a04b9d0 100644 --- a/src/conf/domain_event.c +++ b/src/conf/domain_event.c @@ -88,6 +88,12 @@ struct _virDomainEvent { int type; int status; } blockJob; + struct { + char *oldSrcPath; + char *newSrcPath; + char *devAlias; + int reason; + } diskChange; } data; }; @@ -509,6 +515,12 @@ void virDomainEventFree(virDomainEventPtr event) case VIR_DOMAIN_EVENT_ID_BLOCK_JOB: VIR_FREE(event->data.blockJob.path); break; + + case VIR_DOMAIN_EVENT_ID_DISK_CHANGE: + VIR_FREE(event->data.diskChange.oldSrcPath); + VIR_FREE(event->data.diskChange.newSrcPath); + VIR_FREE(event->data.diskChange.devAlias); + break; } VIR_FREE(event->dom.name); @@ -961,6 +973,61 @@ virDomainEventPtr virDomainEventControlErrorNewFromObj(virDomainObjPtr obj) return ev; } +static virDomainEventPtr +virDomainEventDiskChangeNew(int id, const char *name, + unsigned char *uuid, + const char *oldSrcPath, + const char *newSrcPath, + const char *devAlias, int reason) +{ + virDomainEventPtr ev = + virDomainEventNewInternal(VIR_DOMAIN_EVENT_ID_DISK_CHANGE, + id, name, uuid); + + if (ev) { + if (!(ev->data.diskChange.devAlias = strdup(devAlias))) + goto error; + + if (oldSrcPath && + !(ev->data.diskChange.oldSrcPath = strdup(oldSrcPath))) + goto error; + + if (newSrcPath && + !(ev->data.diskChange.newSrcPath = strdup(newSrcPath))) + goto error; + + ev->data.diskChange.reason = reason; + } + + return ev; + +error: + virReportOOMError(); + virDomainEventFree(ev); + return NULL; +} + +virDomainEventPtr virDomainEventDiskChangeNewFromObj(virDomainObjPtr obj, + const char *oldSrcPath, + const char *newSrcPath, + const char *devAlias, + int reason) +{ + return virDomainEventDiskChangeNew(obj->def->id, obj->def->name, + obj->def->uuid, oldSrcPath, + newSrcPath, devAlias, reason); +} + +virDomainEventPtr virDomainEventDiskChangeNewFromDom(virDomainPtr dom, + const char *oldSrcPath, + const char *newSrcPath, + const char *devAlias, + int reason) +{ + return virDomainEventDiskChangeNew(dom->id, dom->name, dom->uuid, + oldSrcPath, newSrcPath, + devAlias, reason); +} /** * virDomainEventQueuePop: @@ -1104,6 +1171,15 @@ void virDomainEventDispatchDefaultFunc(virConnectPtr conn, cbopaque); break; + case VIR_DOMAIN_EVENT_ID_DISK_CHANGE: + ((virConnectDomainEventDiskChangeCallback)cb)(conn, dom, + event->data.diskChange.oldSrcPath, + event->data.diskChange.newSrcPath, + event->data.diskChange.devAlias, + event->data.diskChange.reason, + cbopaque); + break; + default: VIR_WARN("Unexpected event ID %d", event->eventID); break; diff --git a/src/conf/domain_event.h b/src/conf/domain_event.h index 08930ed..3ba418e 100644 --- a/src/conf/domain_event.h +++ b/src/conf/domain_event.h @@ -179,6 +179,17 @@ virDomainEventPtr virDomainEventBlockJobNewFromDom(virDomainPtr dom, int type, int status); +virDomainEventPtr virDomainEventDiskChangeNewFromObj(virDomainObjPtr obj, + const char *oldSrcPath, + const char *newSrcPath, + const char *devAlias, + int reason); +virDomainEventPtr virDomainEventDiskChangeNewFromDom(virDomainPtr dom, + const char *oldSrcPath, + const char *newSrcPath, + const char *devAlias, + int reason); + int virDomainEventQueuePush(virDomainEventQueuePtr evtQueue, virDomainEventPtr event); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 163b481..f06527c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -481,6 +481,8 @@ virDomainEventCallbackListRemoveConn; virDomainEventCallbackListRemoveID; virDomainEventControlErrorNewFromDom; virDomainEventControlErrorNewFromObj; +virDomainEventDiskChangeNewFromDom; +virDomainEventDiskChangeNewFromObj; virDomainEventDispatch; virDomainEventDispatchDefaultFunc; virDomainEventFree; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 7fbdfa1..198ebcc 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -35,6 +35,7 @@ #include "ignore-value.h" #include "uuid.h" #include "virfile.h" +#include "domain_event.h" #include <sys/time.h> #include <fcntl.h> @@ -1619,6 +1620,7 @@ qemuDomainCheckDiskPresence(struct qemud_driver *driver, int accessRet; virDomainDiskDefPtr disk; char uuid[VIR_UUID_STRING_BUFLEN] ATTRIBUTE_UNUSED; + virDomainEventPtr event = NULL; virUUIDFormat(vm->def->uuid, uuid); @@ -1666,6 +1668,11 @@ qemuDomainCheckDiskPresence(struct qemud_driver *driver, "due to not accessible source '%s'", disk->dst, vm->def->name, uuid, disk->src); + event = virDomainEventDiskChangeNewFromObj(vm, disk->src, NULL, disk->info.alias, + VIR_DOMAIN_DISK_CHANGE_MISSING_ON_START); + if (event) + qemuDomainEventQueue(driver, event); + VIR_FREE(disk->src); } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 5731223..18c98c6 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2880,13 +2880,6 @@ int qemuProcessStart(virConnectPtr conn, NULL) < 0) goto cleanup; - if (qemuAssignDeviceAliases(vm->def, priv->qemuCaps) < 0) - goto cleanup; - - VIR_DEBUG("Checking for CDROM and floppy presence"); - if (qemuDomainCheckDiskPresence(driver, vm, migrateFrom != NULL) < 0) - goto cleanup; - /* If you are using a SecurityDriver with dynamic labelling, then generate a security label for isolation */ VIR_DEBUG("Generating domain security label (if required)"); @@ -2956,6 +2949,13 @@ int qemuProcessStart(virConnectPtr conn, &priv->qemuCaps) < 0) goto cleanup; + if (qemuAssignDeviceAliases(vm->def, priv->qemuCaps) < 0) + goto cleanup; + + VIR_DEBUG("Checking for CDROM and floppy presence"); + if (qemuDomainCheckDiskPresence(driver, vm, migrateFrom != NULL) < 0) + goto cleanup; + VIR_DEBUG("Setting up domain cgroup (if required)"); if (qemuSetupCgroup(driver, vm) < 0) goto cleanup; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 1dea327..e98ebd7 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -228,6 +228,11 @@ remoteDomainBuildEventBlockJob(virNetClientProgramPtr prog, virNetClientPtr client, void *evdata, void *opaque); +static void +remoteDomainBuildEventDiskChange(virNetClientProgramPtr prog, + virNetClientPtr client, + void *evdata, void *opaque); + static virNetClientProgramEvent remoteDomainEvents[] = { { REMOTE_PROC_DOMAIN_EVENT_RTC_CHANGE, remoteDomainBuildEventRTCChange, @@ -265,6 +270,10 @@ static virNetClientProgramEvent remoteDomainEvents[] = { remoteDomainBuildEventBlockJob, sizeof(remote_domain_event_block_job_msg), (xdrproc_t)xdr_remote_domain_event_block_job_msg }, + { REMOTE_PROC_DOMAIN_EVENT_DISK_CHANGE, + remoteDomainBuildEventDiskChange, + sizeof(remote_domain_event_disk_change_msg), + (xdrproc_t)xdr_remote_domain_event_disk_change_msg }, }; enum virDrvOpenRemoteFlags { @@ -3327,6 +3336,33 @@ remoteDomainBuildEventControlError(virNetClientProgramPtr prog ATTRIBUTE_UNUSED, } +static void +remoteDomainBuildEventDiskChange(virNetClientProgramPtr prog ATTRIBUTE_UNUSED, + virNetClientPtr client ATTRIBUTE_UNUSED, + void *evdata, void *opaque) +{ + virConnectPtr conn = opaque; + struct private_data *priv = conn->privateData; + remote_domain_event_disk_change_msg *msg = evdata; + virDomainPtr dom; + virDomainEventPtr event = NULL; + + dom = get_nonnull_domain(conn, msg->dom); + if (!dom) + return; + + event = virDomainEventDiskChangeNewFromDom(dom, + msg->oldSrcPath ? *msg->oldSrcPath : NULL, + msg->newSrcPath ? *msg->newSrcPath : NULL, + msg->devAlias, + msg->reason); + + virDomainFree(dom); + + remoteDomainEventQueue(priv, event); +} + + static virDrvOpenStatus ATTRIBUTE_NONNULL (1) remoteSecretOpen(virConnectPtr conn, virConnectAuthPtr auth, unsigned int flags) diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index f95253e..d135653 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -2010,6 +2010,14 @@ struct remote_domain_event_block_job_msg { int status; }; +struct remote_domain_event_disk_change_msg { + remote_nonnull_domain dom; + remote_string oldSrcPath; + remote_string newSrcPath; + remote_nonnull_string devAlias; + int reason; +}; + struct remote_domain_managed_save_args { remote_nonnull_domain dom; unsigned int flags; @@ -2546,7 +2554,8 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_SNAPSHOT_GET_PARENT = 244, /* autogen autogen priority:high */ REMOTE_PROC_DOMAIN_RESET = 245, /* autogen autogen */ REMOTE_PROC_DOMAIN_SNAPSHOT_NUM_CHILDREN = 246, /* autogen autogen priority:high */ - REMOTE_PROC_DOMAIN_SNAPSHOT_LIST_CHILDREN_NAMES = 247 /* autogen autogen priority:high */ + REMOTE_PROC_DOMAIN_SNAPSHOT_LIST_CHILDREN_NAMES = 247, /* autogen autogen priority:high */ + REMOTE_PROC_DOMAIN_EVENT_DISK_CHANGE = 248 /* skipgen skipgen */ /* * Notice how the entries are grouped in sets of 10 ? diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index 7894441..8e8b231 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -1509,6 +1509,13 @@ struct remote_domain_event_block_job_msg { int type; int status; }; +struct remote_domain_event_disk_change_msg { + remote_nonnull_domain dom; + remote_string oldSrcPath; + remote_string newSrcPath; + remote_nonnull_string devAlias; + int reason; +}; struct remote_domain_managed_save_args { remote_nonnull_domain dom; u_int flags; -- 1.7.3.4
On Fri, Oct 21, 2011 at 03:13:30PM +0200, Michal Privoznik wrote:
If a disk source gets dropped because it is not accessible, mgmt application might want to be informed about this. Therefore we need to emit an event. The event presented in this patch is however a bit superset of what written above. The reason is simple: an intention to be easily expanded, e.g. on 'user ejected disk in guest' events. Therefore, callback gets source string and disk alias (which should be unique among a domain) and reason (an integer); --- daemon/remote.c | 51 +++++++++++++ examples/domain-events/events-c/event-test.c | 29 +++++++- examples/domain-events/events-python/event-test.py | 4 + include/libvirt/libvirt.h.in | 36 +++++++++ python/libvirt-override-virConnect.py | 9 +++ python/libvirt-override.c | 53 ++++++++++++++ src/conf/domain_event.c | 76 ++++++++++++++++++++ src/conf/domain_event.h | 11 +++ src/libvirt_private.syms | 2 + src/qemu/qemu_domain.c | 7 ++ src/qemu/qemu_process.c | 14 ++-- src/remote/remote_driver.c | 36 +++++++++ src/remote/remote_protocol.x | 11 +++- src/remote_protocol-structs | 7 ++ 14 files changed, 337 insertions(+), 9 deletions(-)
ACK, looks good now Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 10/21/2011 07:13 AM, Michal Privoznik wrote:
If a disk source gets dropped because it is not accessible, mgmt application might want to be informed about this. Therefore we need to emit an event. The event presented in this patch is however a bit superset of what written above. The reason is simple: an intention to be easily expanded, e.g. on 'user ejected disk in guest' events. Therefore, callback gets source string and disk alias (which should be unique among a domain) and reason (an integer);
The guaranteed unique string is disk->dst, which is required by the schema and parser to be present for all disks (from <target dev='...'> in the XML). While disk aliases are also unique, they are not part of the user's XML, and correspond only to what we create under the hood to pass to qemu. I wonder if we should go with the disk target name, rather than the disk alias. That would also match what I did for snapshots - a <domainsnapshot> prefers listing disk->dst anywhere on output where a unique name is needed.
/** + * virConnectDomainEventDisChangeReason: + * + * The reason describing why this callback is called + */ +typedef enum { + VIR_DOMAIN_DISK_CHANGE_MISSING_ON_START = 0, /* oldSrcPath is set */ +} virConnectDomainEventDiskChangeReason; + +/** + * virConnectDomainEventDiskChangeCallback: + * @conn: connection object + * @dom: domain on which the event occurred + * @oldSrcPath: old source path + * @newSrcPath: new source path + * @reason: reason why this callback was called; any of + * virConnectDomainEventDiskChangeReason + * @opaque: application specified data + * + * This callback occurs when disk gets changed. However, + * not all @reason mush cause both @oldSrcPath and @newSrcPath
s/mush/will/
+++ b/src/conf/domain_event.c @@ -88,6 +88,12 @@ struct _virDomainEvent { int type; int status; } blockJob; + struct { + char *oldSrcPath; + char *newSrcPath; + char *devAlias;
See for example where domain_conf.c lists _virDomainSnapshotDiskDef uses char *name as its mapping back to <target dev='...'>. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 20.10.2011 16:52, Michal Privoznik wrote:
This is a 4rd version of patches [1]. Diff to v3 includes Daniel's review and some other improvements.
The aim of these patches is to mark CDROM and floppy optional, thus they might get dropped during migration, domain startup, restore, etc. The granularity is set on disk level, so user can decide which cd-roms are important and which not.
There are 3 levels of optionality: - mandatory - fail if missing for any reason (the default) - requisite - fail if missing on boot up, drop if missing on migrate/restore/revert - optional - drop if missing at any start attempt.
To assign a disk one of these, just append 'startupPolicy' attribute to its <source>.
If a domain is then eventually started (migrated, restored, whatever), we check for source path being accessible (currently only qemu driver is supported) from hypervisor's POV. This is done by virFileAccessibleAs() function. Currently, only uid+gid are considered. Ideally, this should be extended to SElinux labeling and cgroups.
1: https://www.redhat.com/archives/libvir-list/2011-October/msg00896.html
Michal Privoznik (5): conf: Introduce optional startupPolicy attribute for cdrom and floppy util: Create virFileAccessibleAs function qemu: Move device alias assigning before command line construction qemu: implement startupPolicy on_missing: Emit event on disk source dropping
daemon/remote.c | 39 ++++++++++ docs/formatdomain.html.in | 26 ++++++- docs/schemas/domaincommon.rng | 22 +++++- examples/domain-events/events-c/event-test.c | 28 +++++++- examples/domain-events/events-python/event-test.py | 4 + include/libvirt/libvirt.h.in | 29 +++++++ python/libvirt-override-virConnect.py | 9 ++ python/libvirt-override.c | 52 +++++++++++++ src/conf/domain_conf.c | 43 ++++++++++- src/conf/domain_conf.h | 11 +++ src/conf/domain_event.c | 61 +++++++++++++++ src/conf/domain_event.h | 9 ++ src/libvirt_private.syms | 4 + src/qemu/qemu_command.c | 5 +- src/qemu/qemu_command.h | 1 + src/qemu/qemu_domain.c | 73 ++++++++++++++++++ src/qemu/qemu_domain.h | 4 + src/qemu/qemu_driver.c | 3 + src/qemu/qemu_process.c | 7 ++ src/remote/remote_driver.c | 35 +++++++++ src/remote/remote_protocol.x | 10 ++- src/remote_protocol-structs | 6 ++ src/util/util.c | 78 ++++++++++++++++++++ src/util/util.h | 3 + tests/qemuxml2argvtest.c | 3 + .../qemuxml2xmlout-disk-cdrom-empty.xml | 31 ++++++++ tests/qemuxmlnstest.c | 3 + 27 files changed, 585 insertions(+), 14 deletions(-) create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-cdrom-empty.xml
Thanks, pushed.
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Jiri Denemark -
Michal Privoznik