- Devel - libvirt List Archives

[libvirt] [PATCH] daemon: Avoid resetting errors before they are reported
by Jiri Denemark 25 Mar '11

25 Mar '11

Commit f44bfb7 was supposed to make sure no additional libvirt API (esp. *Free) is called before remoteDispatchConnError() is called on error. However, the patch missed two instances. --- daemon/remote.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 7520df3..7d0016c 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -5105,12 +5105,13 @@ remoteDispatchStoragePoolListVolumes (struct qemud_server *server ATTRIBUTE_UNUS ret->names.names_len = virStoragePoolListVolumes (pool, ret->names.names_val, args->maxnames); - virStoragePoolFree(pool); if (ret->names.names_len == -1) { VIR_FREE(ret->names.names_val); remoteDispatchConnError(rerr, conn); + virStoragePoolFree(pool); return -1; } + virStoragePoolFree(pool); return 0; } @@ -5134,11 +5135,12 @@ remoteDispatchStoragePoolNumOfVolumes (struct qemud_server *server ATTRIBUTE_UNU } ret->num = virStoragePoolNumOfVolumes (pool); - virStoragePoolFree(pool); if (ret->num == -1) { remoteDispatchConnError(rerr, conn); + virStoragePoolFree(pool); return -1; } + virStoragePoolFree(pool); return 0; } -- 1.7.4.1

2 2

[libvirt] CPU compare and baseline
by Edouard Thuleau 25 Mar '11

25 Mar '11

Hi, I try to understand the compare and baseline CPU commands with libvirt. This functions are useful for the migration ? Before migrate a domain to other host, libvirt verifies the CPU compatibility, that's right ? I've got two 64bits hosts, one with an Intel CPU : > *<cpu>* * **<arch>x86_64</arch>* * **<model>Penryn</model>* * **<vendor>Intel</vendor>* * **<topology sockets='1' cores='2' threads='1'/>* * **<feature name='xtpr'/>* * **<feature name='tm2'/>* * **<feature name='est'/>* * **<feature name='vmx'/>* * **<feature name='ds_cpl'/>* * **<feature name='monitor'/>* * **<feature name='pbe'/>* * **<feature name='tm'/>* * **<feature name='ht'/>* * **<feature name='ss'/>* * **<feature name='acpi'/>* * **<feature name='ds'/>* * **<feature name='vme'/>* *</cpu>* and one with an AMD CPU > *<cpu>* * **<arch>x86_64</arch>* * **<model>Opteron_G2</model>* * **<vendor>AMD</vendor>* * **<topology sockets='1' cores='2' threads='1'/>* * **<feature name='3dnowprefetch'/>* * **<feature name='cr8legacy'/>* * **<feature name='extapic'/>* * **<feature name='cmp_legacy'/>* * **<feature name='3dnow'/>* * **<feature name='3dnowext'/>* * **<feature name='fxsr_opt'/>* * **<feature name='mmxext'/>* * **<feature name='ht'/>* * **<feature name='vme'/>* *</cpu>* Baseline output : AMD to intel: > *$ virsh cpu-baseline cpu_gargamel.xml * *<cpu match='exact'>* * <model>Opteron_G2</model>* * <vendor>AMD</vendor>* * <feature policy='require' name='3dnowprefetch'/>* * <feature policy='require' name='cr8legacy'/>* * <feature policy='require' name='extapic'/>* * <feature policy='require' name='cmp_legacy'/>* * <feature policy='require' name='3dnow'/>* * <feature policy='require' name='3dnowext'/>* * <feature policy='require' name='fxsr_opt'/>* * <feature policy='require' name='mmxext'/>* * <feature policy='require' name='ht'/>* * <feature policy='require' name='vme'/>* *</cpu>* Intel to AMD: > *$ virsh cpu-baseline cpu_azrael.xml * *<cpu match='exact'>* * <model>Penryn</model>* * <vendor>Intel</vendor>* * <feature policy='require' name='xtpr'/>* * <feature policy='require' name='tm2'/>* * <feature policy='require' name='est'/>* * <feature policy='require' name='vmx'/>* * <feature policy='require' name='ds_cpl'/>* * <feature policy='require' name='monitor'/>* * <feature policy='require' name='pbe'/>* * <feature policy='require' name='tm'/>* * <feature policy='require' name='ht'/>* * <feature policy='require' name='ss'/>* * <feature policy='require' name='acpi'/>* * <feature policy='require' name='ds'/>* * <feature policy='require' name='vme'/>* *</cpu>* But if I try to migrate a domain from a host to another, the CPU compare says there are incompatible. I don't understand, I've done live migration manually (*migrate -d tcp: 192.168.1.10:4444*) with KVM and it's works nicely. Why it's not possible trough libvirt ? I use libvirt 0.7.5 with KVM on an Ubuntu LTS 10.04. Regards, Doude.

3 4

[libvirt] [PATCH] command: properly diagnose process exit via signal
by Eric Blake 25 Mar '11

25 Mar '11

Child processes don't always reach _exit(); if they die from a signal, then any messages should still be accurate. Most users either expect a 0 status (thankfully, if status==0, then WIFEXITED(status) is true and WEXITSTATUS(status)==0 for all known platforms) or were filtering on WIFEXITED before printing a status, but a few were missing this check. Additionally, nwfilter_ebiptables_driver was making an assumption that works on Linux (where WEXITSTATUS shifts and WTERMSIG just masks) but fails on other platforms (where WEXITSTATUS just masks and WTERMSIG shifts). * src/util/command.h (virCommandTranslateStatus): New helper. * src/libvirt_private.syms (command.h): Export it. * src/util/command.c (virCommandTranslateStatus): New function. (virCommandWait): Use it to also diagnose status from signals. * src/security/security_apparmor.c (load_profile): Likewise. * src/storage/storage_backend.c (virStorageBackendQEMUImgBackingFormat): Likewise. * src/util/util.c (virExecDaemonize, virRunWithHook) (virFileOperation, virDirCreate): Likewise. * daemon/remote.c (remoteDispatchAuthPolkit): Likewise. * src/nwfilter/nwfilter_ebiptables_driver.c (ebiptablesExecCLI): Likewise. --- In response to my finding here: https://www.redhat.com/archives/libvir-list/2011-March/msg01029.html > status includes normal exit and signals. This should probably use > WIFEXITED and WEXITSTATUS to avoid printing values shifted by 8. For > that matter, I just noticed that virCommandWait should probably be more > careful in how it interprets status. daemon/remote.c | 11 +++++++- src/libvirt_private.syms | 1 + src/nwfilter/nwfilter_ebiptables_driver.c | 16 ++++++++++--- src/security/security_apparmor.c | 22 ++++++++++++------ src/storage/storage_backend.c | 18 ++++++--------- src/util/command.c | 34 ++++++++++++++++++++++++++-- src/util/command.h | 7 +++++- src/util/util.c | 27 ++++++++++------------ 8 files changed, 92 insertions(+), 44 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index f410982..2c54721 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -60,6 +60,7 @@ #include "uuid.h" #include "network.h" #include "libvirt/libvirt-qemu.h" +#include "command.h" #define VIR_FROM_THIS VIR_FROM_REMOTE #define REMOTE_DEBUG(fmt, ...) VIR_DEBUG(fmt, __VA_ARGS__) @@ -4363,8 +4364,14 @@ remoteDispatchAuthPolkit (struct qemud_server *server, goto authfail; } if (status != 0) { - VIR_ERROR(_("Policy kit denied action %s from pid %d, uid %d, result: %d"), - action, callerPid, callerUid, status); + char *tmp = virCommandTranslateStatus(status); + if (!tmp) { + virReportOOMError(); + goto authfail; + } + VIR_ERROR(_("Policy kit denied action %s from pid %d, uid %d: %s"), + action, callerPid, callerUid, tmp); + VIR_FREE(tmp); goto authdeny; } PROBE(CLIENT_AUTH_ALLOW, "fd=%d, auth=%d, username=%s", diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 55be36e..d2f98fc 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -123,6 +123,7 @@ virCommandSetPreExecHook; virCommandSetWorkingDirectory; virCommandToString; virCommandTransferFD; +virCommandTranslateStatus; virCommandWait; virCommandWriteArgLog; diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c b/src/nwfilter/nwfilter_ebiptables_driver.c index 2ec5b02..75fddfb 100644 --- a/src/nwfilter/nwfilter_ebiptables_driver.c +++ b/src/nwfilter/nwfilter_ebiptables_driver.c @@ -1,6 +1,7 @@ /* * nwfilter_ebiptables_driver.c: driver for ebtables/iptables on tap devices * + * Copyright (C) 2011 Red Hat, Inc. * Copyright (C) 2010 IBM Corp. * Copyright (C) 2010 Stefan Berger * @@ -2558,7 +2559,7 @@ err_exit: * ebiptablesExecCLI: * @buf : pointer to virBuffer containing the string with the commands to * execute. - * @status: Pointer to an integer for returning the status of the + * @status: Pointer to an integer for returning the WEXITSTATUS of the * commands executed via the script the was run. * * Returns 0 in case of success, != 0 in case of an error. The returned @@ -2587,7 +2588,7 @@ ebiptablesExecCLI(virBufferPtr buf, cmds = virBufferContentAndReset(buf); - VIR_DEBUG("%s", cmds); + VIR_DEBUG("%s", NULLSTR(cmds)); if (!cmds) return 0; @@ -2606,9 +2607,16 @@ ebiptablesExecCLI(virBufferPtr buf, virMutexUnlock(&execCLIMutex); - *status >>= 8; + if (rc == 0) { + if (WIFEXITED(*status)) { + *status = WEXITSTATUS(*status); + } else { + rc = -1; + *status = 1; + } + } - VIR_DEBUG("rc = %d, status = %d",rc, *status); + VIR_DEBUG("rc = %d, status = %d", rc, *status); unlink(filename); diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index deb4181..3edc680 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -1,5 +1,6 @@ /* * AppArmor security driver for libvirt + * Copyright (C) 2011 Red Hat, Inc. * Copyright (C) 2009-2010 Canonical Ltd. * * This library is free software; you can redistribute it and/or @@ -36,6 +37,7 @@ #include "hostusb.h" #include "files.h" #include "configmake.h" +#include "command.h" #define VIR_FROM_THIS VIR_FROM_SECURITY #define SECURITY_APPARMOR_VOID_DOI "0" @@ -217,15 +219,19 @@ load_profile(virSecurityManagerPtr mgr, VIR_FORCE_CLOSE(pipefd[1]); rc = 0; - rewait: - if (waitpid(child, &status, 0) != child) { - if (errno == EINTR) - goto rewait; - + while ((ret = waitpid(child, &status, 0)) < 0 && errno == EINTR); + if (ret < 0) { + virReportSystemError(errno, + _("Failed to reap virt-aa-helper pid %lu"), + (unsigned long)child); + rc = -1; + } else if (status) { + char *str = virCommandTranslateStatus(status); virSecurityReportError(VIR_ERR_INTERNAL_ERROR, - _("Unexpected exit status from virt-aa-helper " - "%d pid %lu"), - WEXITSTATUS(status), (unsigned long)child); + _("Unexpected status from virt-aa-helper " + "pid %lu: %s"), + (unsigned long)child, NULLSTR(str)); + VIR_FREE(str); rc = -1; } diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index fc08c68..c6c16c8 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -56,6 +56,7 @@ #include "storage_backend.h" #include "logging.h" #include "files.h" +#include "command.h" #if WITH_STORAGE_LVM # include "storage_backend_logical.h" @@ -631,18 +632,13 @@ static int virStorageBackendQEMUImgBackingFormat(const char *qemuimg) cleanup: VIR_FREE(help); VIR_FORCE_CLOSE(newstdout); -rewait: if (child) { - if (waitpid(child, &status, 0) != child) { - if (errno == EINTR) - goto rewait; - - VIR_ERROR(_("Unexpected exit status from qemu %d pid %lu"), - WEXITSTATUS(status), (unsigned long)child); - } - if (WEXITSTATUS(status) != 0) { - VIR_WARN("Unexpected exit status '%d', qemu probably failed", - WEXITSTATUS(status)); + while (waitpid(child, &status, 0) == -1 && errno == EINTR); + if (status) { + tmp = virCommandTranslateStatus(status); + VIR_WARN("Unexpected status, qemu probably failed: %s", + NULLSTR(tmp)); + VIR_FREE(tmp); } } diff --git a/src/util/command.c b/src/util/command.c index 22f350b..89f82c7 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -798,6 +798,26 @@ virCommandToString(virCommandPtr cmd) /* + * Translate an exit status into a malloc'd string. Generic helper + * for virCommandRun and virCommandWait status argument, as well as + * raw waitpid and older virRun status. + */ +char * +virCommandTranslateStatus(int status) +{ + char *buf; + if (WIFEXITED(status)) { + virAsprintf(&buf, _("exit status %d"), WEXITSTATUS(status)); + } else if (WIFSIGNALED(status)) { + virAsprintf(&buf, _("fatal signal %d"), WTERMSIG(status)); + } else { + virAsprintf(&buf, _("invalid value %d"), status); + } + return buf; +} + + +/* * Manage input and output to the child process. */ static int @@ -958,6 +978,7 @@ virCommandRun(virCommandPtr cmd, int *exitstatus) struct stat st; bool string_io; bool async_io = false; + char *str; if (!cmd ||cmd->has_error == ENOMEM) { virReportOOMError(); @@ -1048,9 +1069,14 @@ virCommandRun(virCommandPtr cmd, int *exitstatus) if (virCommandWait(cmd, exitstatus) < 0) ret = -1; - VIR_DEBUG("Result stdout: '%s' stderr: '%s'", + str = (exitstatus ? virCommandTranslateStatus(*exitstatus) + : (char *) "status 0"); + VIR_DEBUG("Result %s, stdout: '%s' stderr: '%s'", + NULLSTR(str), cmd->outbuf ? NULLSTR(*cmd->outbuf) : "(null)", cmd->errbuf ? NULLSTR(*cmd->errbuf) : "(null)"); + if (exitstatus) + VIR_FREE(str); /* Reset any capturing, in case caller runs * this identical command again */ @@ -1226,10 +1252,12 @@ virCommandWait(virCommandPtr cmd, int *exitstatus) if (exitstatus == NULL) { if (status != 0) { char *str = virCommandToString(cmd); + char *st = virCommandTranslateStatus(status); virCommandError(VIR_ERR_INTERNAL_ERROR, - _("Child process (%s) exited with status %d."), - str ? str : cmd->args[0], WEXITSTATUS(status)); + _("Child process (%s) status unexpected: %s"), + str ? str : cmd->args[0], NULLSTR(st)); VIR_FREE(str); + VIR_FREE(st); return -1; } } else { diff --git a/src/util/command.h b/src/util/command.h index 59d0ee3..0e890d0 100644 --- a/src/util/command.h +++ b/src/util/command.h @@ -1,7 +1,7 @@ /* * command.h: Child command execution * - * Copyright (C) 2010 Red Hat, Inc. + * Copyright (C) 2010-2011 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -273,5 +273,10 @@ int virCommandWait(virCommandPtr cmd, */ void virCommandFree(virCommandPtr cmd); +/* + * Translate an exit status into a malloc'd string. + */ +char *virCommandTranslateStatus(int exitstatus) ATTRIBUTE_RETURN_CHECK; + #endif /* __VIR_COMMAND_H__ */ diff --git a/src/util/util.c b/src/util/util.c index 1e4e2ab..4301b00 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -76,6 +76,7 @@ #include "threads.h" #include "verify.h" #include "files.h" +#include "command.h" #ifndef NSIG # define NSIG 32 @@ -832,9 +833,11 @@ int virExecDaemonize(const char *const*argv, errno == EINTR); if (childstat != 0) { + char *str = virCommandTranslateStatus(childstat); virUtilError(VIR_ERR_INTERNAL_ERROR, - _("Intermediate daemon process exited with status %d."), - WEXITSTATUS(childstat)); + _("Intermediate daemon process status unexpected: %s"), + NULLSTR(str)); + VIR_FREE(str); ret = -2; } @@ -903,13 +906,12 @@ virRunWithHook(const char *const*argv, if (status == NULL) { errno = EINVAL; - if (WIFEXITED(exitstatus) && WEXITSTATUS(exitstatus) != 0) { + if (exitstatus) { + char *str = virCommandTranslateStatus(exitstatus); virUtilError(VIR_ERR_INTERNAL_ERROR, - _("'%s' exited with non-zero status %d and " - "signal %d: %s"), argv_str, - WIFEXITED(exitstatus) ? WEXITSTATUS(exitstatus) : 0, - WIFSIGNALED(exitstatus) ? WTERMSIG(exitstatus) : 0, - (errbuf ? errbuf : "")); + _("'%s' exited unexpectedly: %s"), + argv_str, NULLSTR(str)); + VIR_FREE(str); goto error; } } else { @@ -1510,8 +1512,7 @@ int virFileOperation(const char *path, int openflags, mode_t mode, path); goto parenterror; } - ret = -WEXITSTATUS(status); - if (!WIFEXITED(status) || (ret == -EACCES)) { + if (!WIFEXITED(status) || (ret = -WEXITSTATUS(status)) == -EACCES) { /* fall back to the simpler method, which works better in * some cases */ return virFileOperationNoFork(path, openflags, mode, uid, gid, @@ -1630,15 +1631,11 @@ int virDirCreate(const char *path, mode_t mode, path); goto parenterror; } - ret = -WEXITSTATUS(status); - if (!WIFEXITED(status) || (ret == -EACCES)) { + if (!WIFEXITED(status) || (ret = -WEXITSTATUS(status)) == -EACCES) { /* fall back to the simpler method, which works better in * some cases */ return virDirCreateNoFork(path, mode, uid, gid, flags); } - if (ret < 0) { - goto parenterror; - } parenterror: return ret; } -- 1.7.4

3 17

[libvirt] About to enter freeze for 0.9.0, on Monday
by Daniel Veillard 25 Mar '11

25 Mar '11

As posted previously I was thinking of entering freeze today, but we have a number of patchsets which are relatively advanced but don't have a final version, like Dan's patches and support of persistent device by Kame. So I suggest to extend the development Friday and over the week-end if needed but we will enter Freeze Monday morning. Then depending on feedback on release candidate, I will either push 0.9.0 on the 1st April or following Monday, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

1 0

[libvirt] [PATCH] fix the check of the output of monitor command 'device_add'
by Wen Congyang 25 Mar '11

25 Mar '11

Hotpluging host usb device by text mode will fail, because the monitor command 'device_add' outputs 'husb: using...' if it succeeds, but we think the command should not output anything. Signed-off-by: Wen Congyang <wency(a)cn.fujitsu.com> --- src/qemu/qemu_monitor_text.c | 10 +++++++++- 1 files changed, 9 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index 91ecc8b..75b2995 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -2283,7 +2283,15 @@ int qemuMonitorTextAddDevice(qemuMonitorPtr mon, goto cleanup; } - /* If the command succeeds, no output is sent. So + /* If the host device is hotpluged first time, qemu will output + * husb: using %s file-system with %s if the command succeeds. + */ + if (STRPREFIX(reply, "husb: using")) { + ret = 0; + goto cleanup; + } + + /* Otherwise, if the command succeeds, no output is sent. So * any non-empty string shows an error */ if (STRNEQ(reply, "")) { qemuReportError(VIR_ERR_OPERATION_FAILED, -- 1.7.1

2 2

[libvirt] cgroup memoryinfo handling
by Zvi Dubitzky 24 Mar '11

24 Mar '11

Hi 1) Daniel Smith of IBM wrote (2008) code for implementing 'virsh schedinfo' in src/util/cgroup.c He also added some code for cgroup memory subsystem (util/cgroup.c/ virCgroupSetMemoryHardLimit () etc.), I see also src/qemu_driver.c/qemuDomainSetMemoryParameters() But that seems not being invoked . I am looking at libvirt-0.8.8 2) Is anybody handling/completing this piece of code and are you libvirt guys interested in us completing it because we might find usage for that kind of 'virsh meminfo' command using cgroups . thanks Zvi Dubitzky Email:dubi@il.ibm.com

2 3

[libvirt] [PATCH] unlock the monitor when unwatching the monitor
by Wen Congyang 24 Mar '11

24 Mar '11

Steps to reproduce this bug: # virsh qemu-monitor-command domain 'cpu_set 2 online' --hmp The domain has 2 cpus, and we try to set the third cpu online. The qemu crashes, and this command will hang. The reason is that the refs is not 1 when we unwatch the monitor. We lock the monitor, but we do not unlock it. So virCondWait() will be blocked. Signed-off-by: Wen Congyang <wency(a)cn.fujitsu.com> --- src/qemu/qemu_monitor.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index da38096..dc08594 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -239,7 +239,8 @@ qemuMonitorUnwatch(void *monitor) qemuMonitorPtr mon = monitor; qemuMonitorLock(mon); - qemuMonitorUnref(mon); + if (qemuMonitorUnref(mon) > 0) + qemuMonitorUnlock(mon); } static int -- 1.7.1

5 8

[libvirt] ANNOUNCE: virt-manager 0.8.7 and virtinst 0.500.6 released
by Cole Robinson 24 Mar '11

24 Mar '11

I'm happy to announce two new releases: virt-manager 0.8.7: virt-manager is a desktop application for managing KVM and Xen virtual machines via libvirt. virtinst 0.500.6: virtinst is a collection of command line tools for provisioning libvirt virtual machines, including virt-install and virt-clone. The releases can be downloaded from: http://virt-manager.org/download.html The direct download links are: http://virt-manager.org/download/sources/virt-manager/virt-manager-0.8.7.ta… http://virt-manager.org/download/sources/virtinst/virtinst-0.500.6.tar.gz Here are the highlights for the past 2 releases (since I neglected to send out a release announcement for the previous round :) ) The virt-manager release includes: virt-manager 0.8.7: - Allow renaming an offline VM - Spice password support (Marc-André Lureau) - Allow editting NIC <virtualport> settings (Gerhard Stenzel) - Allow enabling/disabling individual CPU features - Allow easily changing graphics type between VNC and SPICE for existing VM - Allow easily changing network source device for existing VM virt-manager 0.8.6: - SPICE support (requires spice-gtk) (Marc-André Lureau) - Option to configure CPU model - Option to configure CPU topology - Save and migration cancellation (Wen Congyang) - Save and migration progress reporting - Option to enable bios boot menu - Option to configure direct kernel/initrd boot virtinst 0.500.6: - virt-install: --graphics spice now enables spicevmc and qxl - virt-install: New --disk parameter io=native|threads virtinst 0.500.5: - New virt-install --cpu option for configuring CPU model/features - virt-install --vcpus option can not specify topology and maxvcpus - New virt-install --graphics option to unify --vnc, --sdl, spice config - New virt-install --print-xml option to skip install and print XML Thanks to everyone who has contributed to this release through testing, bug reporting, submitting patches, and otherwise sending in feedback! Thanks, Cole

1 0

[libvirt] [PATCHv6 1/3] libvirt/qemu - support persistent modification of devices.
by KAMEZAWA Hiroyuki 24 Mar '11

24 Mar '11

Thank you for comments. This is a rebased/fixed one. = >From 5858b9d6b33a9d917576f6179be13a306272ff45 Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com> Date: Wed, 23 Mar 2011 17:45:12 +0900 Subject: [PATCHv6 1/3] libvirt/qemu - support persistent modification of devices. Now, qemudDomainAttachDeviceFlags() and qemudDomainDetachDeviceFlags() doesn't support VIR_DOMAIN_DEVICE_MODIFY_CONFIG. By this, virsh's at(de)tatch-device --persistent cannot modify qemu config. (Xen allows it.) This patch is a base patch for adding support of devices in step by step manner. Following patches will add some device support. Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com> Changelog v5->v6 - fixed Indentation - clean up. Changelog v4->v5: - fixed error codes at reporting Error. - fixed unlock code after calling qemuDomainObjBeginJobWithDriver() - fixed virDomainFindByUUID() to be virDomainFindByName() - fixed spelling, indent, etc.. - removed unnecessary checks. --- src/qemu/qemu_driver.c | 137 +++++++++++++++++++++++++++++++++++++++++++----- 1 files changed, 124 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6f296c9..468361b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4140,16 +4140,124 @@ cleanup: return ret; } -static int qemudDomainAttachDeviceFlags(virDomainPtr dom, - const char *xml, - unsigned int flags) { - if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) { - qemuReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("cannot modify the persistent configuration of a domain")); +/* + * Attach a device given by XML, the change will be persistent + * and domain XML definition file is updated. + */ +static int qemuDomainAttachDevicePersistent(virDomainDefPtr vmdef, + virDomainDeviceDefPtr newdev) +{ + + switch(newdev->type) { + default: + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Sorry, the device is not supported for now")); + return -1; + } + + return 0; +} + +static int qemuDomainDetachDevicePersistent(virDomainDefPtr vmdef, + virDomainDeviceDefPtr device) +{ + switch(device->type) { + default: + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Sorry, the device is not supported for now")); return -1; } + return 0; +} + +static int qemuDomainModifyDevicePersistent(virDomainPtr dom, + const char *xml, + unsigned int attach, unsigned int flags) +{ + struct qemud_driver *driver; + virDomainDeviceDefPtr device; + virDomainDefPtr vmdef; + virDomainObjPtr vm; + int ret = -1; + + /* + * When both of MODIFY_CONFIG and MODIFY_LIVE is specified at the same time, + * return error for now. We should support this later. + */ + if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) { + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("Now, cannot modify alive domain and its definition " + "at the same time.")); + return -1; + } + + driver = dom->conn->privateData; + qemuDriverLock(driver); + vm = virDomainFindByName(&driver->domains, dom->name); + if (!vm) { + qemuReportError(VIR_ERR_NO_DOMAIN, _("no domain with name : '%s'"), + dom->name); + goto unlock_out; + } + + if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0) + goto unlock_out; + + if (virDomainObjIsActive(vm)) { + /* + * For now, just allow updating inactive domains. Further development + * will allow updating both active domain and its config file at + * the same time. + */ + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("unsupported to update active domain's definition.")); + goto endjob; + } + + vmdef = virDomainObjGetPersistentDef(driver->caps, vm); - return qemudDomainAttachDevice(dom, xml); + if (!vmdef) + goto endjob; + + device = virDomainDeviceDefParse(driver->caps, + vmdef, xml, VIR_DOMAIN_XML_INACTIVE); + if (!device) + goto endjob; + + if (attach) + ret = qemuDomainAttachDevicePersistent(vmdef, device); + else + ret = qemuDomainDetachDevicePersistent(vmdef, device); + + if (!ret) /* save the result */ + ret = virDomainSaveConfig(driver->configDir, vmdef); + + virDomainDeviceDefFree(device); + +endjob: + if (qemuDomainObjEndJob(vm) == 0) + vm = NULL; +unlock_out: + if (vm) + virDomainObjUnlock(vm); + qemuDriverUnlock(driver); + return ret; +} + +static int qemudDomainAttachDeviceFlags(virDomainPtr dom, + const char *xml, + unsigned int flags) +{ + if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) + return qemuDomainModifyDevicePersistent(dom, xml, 1, flags); + + if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) + return qemudDomainAttachDevice(dom, xml); + + qemuReportError(VIR_ERR_INVALID_ARG, + _("bad flag: %x not supported"), flags); + + return -1; } @@ -4364,13 +4472,16 @@ cleanup: static int qemudDomainDetachDeviceFlags(virDomainPtr dom, const char *xml, unsigned int flags) { - if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) { - qemuReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("cannot modify the persistent configuration of a domain")); - return -1; - } - return qemudDomainDetachDevice(dom, xml); + if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) + return qemuDomainModifyDevicePersistent(dom, xml, 0, flags); + + if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) + return qemudDomainDetachDevice(dom, xml); + + qemuReportError(VIR_ERR_INVALID_ARG, + _("bad flag: %x not supported now"), flags); + return -1; } static int qemudDomainGetAutostart(virDomainPtr dom, -- 1.7.4.1

2 8

[libvirt] [PATCH] update domain status forcibly even if attach a device failed
by Wen Congyang 24 Mar '11

24 Mar '11

Steps to reproduce this bug: 1. virsh attach-disk domain --source diskimage --target sdb --sourcetype file --driver qemu --subdriver qcow2 error: Failed to attach disk error: operation failed: adding scsi-disk,bus=scsi0.0,scsi-id=1,drive=drive-scsi0-0-1,id=scsi0-0-1 device failed: Property 'scsi-disk.drive' can't find value 'drive-scsi0-0-1' 2. service libvirtd restart Stopping libvirtd daemon: [ OK ] Starting libvirtd daemon: [ OK ] 3. virsh attach-disk domain --source diskimage --target sdb --sourcetype file --driver qemu --subdriver raw error: Failed to attach disk error: operation failed: adding lsi,id=scsi0,bus=pci.0,addr=0x6 device failed: Duplicate ID 'scsi0' for device The reason is that we create a new scsi controller but we do not update /var/run/libvirt/qemu/domain.xml. Signed-off-by: Wen Congyang <wency(a)cn.fujitsu.com> --- src/qemu/qemu_driver.c | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 82a2210..5dff64a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6638,7 +6638,11 @@ static int qemudDomainAttachDevice(virDomainPtr dom, goto endjob; } - if (!ret && virDomainSaveStatus(driver->caps, driver->stateDir, vm) < 0) + /* update domain status forcibly because the domain status may be changed + * even if we attach the device failed. For example, a new controller may + * be created. + */ + if (virDomainSaveStatus(driver->caps, driver->stateDir, vm) < 0) ret = -1; endjob: -- 1.7.1

2 6