- Devel - libvirt List Archives

[libvirt] [PATCH] qemu: Report errors from iohelper
by Michal Privoznik 29 Oct '12

29 Oct '12

Currently, we use iohelper when saving/restoring a domain. However, if there's some kind of error (like I/O) it is not propagated to libvirt. Since it is not qemu who is doing the actual write() it will not get error. The iohelper does. Therefore we should check for iohelper errors as it makes libvirt more user friendly. --- diff to v1: -incorporate the event loop to read iohelper's stderr src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 7 ++- src/util/virfile.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 2 + 4 files changed, 105 insertions(+), 2 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 699c9a3..68440d0 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1332,6 +1332,7 @@ virDomainListSnapshots; virFileLoopDeviceAssociate; virFileClose; virFileDirectFdFlag; +virFileWrapperFdCatchError; virFileWrapperFdClose; virFileWrapperFdFree; virFileWrapperFdNew; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8af316f..c8e97cf 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2945,6 +2945,7 @@ endjob: if (rc < 0) VIR_WARN("Unable to resume guest CPUs after save failure"); } + virFileWrapperFdCatchError(wrapperFd); } if (qemuDomainObjEndAsyncJob(driver, vm) == 0) vm = NULL; @@ -3282,9 +3283,11 @@ doCoreDump(struct qemud_driver *driver, cleanup: VIR_FORCE_CLOSE(fd); - virFileWrapperFdFree(wrapperFd); - if (ret != 0) + if (ret != 0) { + virFileWrapperFdCatchError(wrapperFd); unlink(path); + } + virFileWrapperFdFree(wrapperFd); return ret; } diff --git a/src/util/virfile.c b/src/util/virfile.c index 5b00ead..36b3420 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -135,10 +135,58 @@ virFileDirectFdFlag(void) * read-write is not supported, just a single direction. */ struct _virFileWrapperFd { virCommandPtr cmd; /* Child iohelper process to do the I/O. */ + int err_fd; /* FD to read stderr of @cmd */ + char *err_msg; /* stderr of @cmd */ + size_t err_msg_len; /* strlen of err_msg so we don't + have to compute it every time */ + int err_watch; /* ID of watch in the event loop */ }; #ifndef WIN32 /** + * virFileWrapperFdReadStdErr: + * @watch: watch ID + * @fd: the read end of pipe to iohelper's stderr + * @events: an OR-ed set of events which occurred on @fd + * @opaque: virFileWrapperFdPtr + * + * This is a callback to our eventloop which will read iohelper's + * stderr, reallocate @opaque->err_msg and copy data. + */ +static void +virFileWrapperFdReadStdErr(int watch ATTRIBUTE_UNUSED, + int fd, int events, void *opaque) +{ + virFileWrapperFdPtr wfd = (virFileWrapperFdPtr) opaque; + char ebuf[1024]; + ssize_t nread; + + if (events & VIR_EVENT_HANDLE_READABLE) { + while ((nread = saferead(fd, ebuf, sizeof(ebuf)))) { + if (nread < 0) { + if (errno != EAGAIN) + virReportSystemError(errno, "%s", + _("unable to read iohelper's stderr")); + break; + } + + if (VIR_REALLOC_N(wfd->err_msg, wfd->err_msg_len + nread + 1) < 0) { + virReportOOMError(); + return; + } + memcpy(wfd->err_msg + wfd->err_msg_len, ebuf, nread); + wfd->err_msg_len += nread; + wfd->err_msg[wfd->err_msg_len] = '\0'; + } + } + + if (events & VIR_EVENT_HANDLE_HANGUP) { + virEventRemoveHandle(watch); + wfd->err_watch = -1; + } +} + +/** * virFileWrapperFdNew: * @fd: pointer to fd to wrap * @name: name of fd, for diagnostics @@ -197,6 +245,8 @@ virFileWrapperFdNew(int *fd, const char *name, unsigned int flags) return NULL; } + ret->err_watch = -1; + mode = fcntl(*fd, F_GETFL); if (mode < 0) { @@ -229,9 +279,36 @@ virFileWrapperFdNew(int *fd, const char *name, unsigned int flags) virCommandAddArg(ret->cmd, "0"); } + /* In order to catch iohelper stderr, we must: + * - pass a FD to virCommand (-1 to auto-allocate one) + * - change iohelper's env so virLog functions print to stderr + */ + ret->err_fd = -1; + virCommandSetErrorFD(ret->cmd, &ret->err_fd); + virCommandAddEnvPair(ret->cmd, "LIBVIRT_LOG_OUTPUTS", "1:stderr"); + if (virCommandRunAsync(ret->cmd, NULL) < 0) goto error; + /* deliberately don't use virCommandNonblockingFDs here as it is all or + * nothing. And we want iohelper's stdin to block. */ + if (virSetNonBlock(ret->err_fd) < 0) { + virReportSystemError(errno, "%s", + _("Failed to set non-blocking " + "file descriptor flag")); + goto error; + } + + if ((ret->err_watch = virEventAddHandle(ret->err_fd, + VIR_EVENT_HANDLE_READABLE, + virFileWrapperFdReadStdErr, + ret, NULL)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("unable to register iohelper's " + "stderr FD in the eventloop")); + goto error; + } + if (VIR_CLOSE(pipefd[!output]) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("unable to close pipe")); goto error; @@ -280,6 +357,21 @@ virFileWrapperFdClose(virFileWrapperFdPtr wfd) return virCommandWait(wfd->cmd, NULL); } + +/** + * virFileWrapperFdCatchError: + * @wfd: fd wrapper, or NULL + * + * If iohelper reported any error VIR_WARN() about it. + */ +void +virFileWrapperFdCatchError(virFileWrapperFdPtr wfd) +{ + if (wfd->err_msg) + VIR_WARN("iohelper reports: %s", wfd->err_msg); +} + + /** * virFileWrapperFdFree: * @wfd: fd wrapper, or NULL @@ -295,6 +387,11 @@ virFileWrapperFdFree(virFileWrapperFdPtr wfd) if (!wfd) return; + VIR_FORCE_CLOSE(wfd->err_fd); + if (wfd->err_watch != -1) + virEventRemoveHandle(wfd->err_watch); + VIR_FREE(wfd->err_msg); + virCommandFree(wfd->cmd); VIR_FREE(wfd); } diff --git a/src/util/virfile.h b/src/util/virfile.h index c885b73..80daf86 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -90,6 +90,8 @@ int virFileWrapperFdClose(virFileWrapperFdPtr dfd); void virFileWrapperFdFree(virFileWrapperFdPtr dfd); +void virFileWrapperFdCatchError(virFileWrapperFdPtr dfd); + int virFileLock(int fd, bool shared, off_t start, off_t len); int virFileUnlock(int fd, off_t start, off_t len); -- 1.7.8.6

2 4

[libvirt] [PATCH 0/2] v3: dnsmasq conf-file and interface= patches
by Gene Czarcinski 29 Oct '12

29 Oct '12

I am sorry about the clutter with my multiple patch submissions. I am just trynig to get it right so that, the next time, I will not make so many stupid mistakes. Gene Czarcinski (2): v3: put dnsmasq parameters into a file instead of the command line v3: add dnsmasq interface= parameter so bind-interfaces works src/network/bridge_driver.c | 183 ++++++++++++++------- src/network/bridge_driver.h | 8 +- tests/networkxml2argvdata/isolated-network.argv | 25 ++- .../networkxml2argvdata/nat-network-dns-hosts.argv | 15 +- .../nat-network-dns-srv-record-minimal.argv | 36 ++-- .../nat-network-dns-srv-record.argv | 36 ++-- .../nat-network-dns-txt-record.argv | 30 ++-- tests/networkxml2argvdata/nat-network.argv | 28 ++-- tests/networkxml2argvdata/netboot-network.argv | 29 ++-- .../networkxml2argvdata/netboot-proxy-network.argv | 26 ++- tests/networkxml2argvdata/routed-network.argv | 13 +- tests/networkxml2argvtest.c | 44 +---- 12 files changed, 278 insertions(+), 195 deletions(-) -- 1.7.11.7

2 19

[libvirt] libvirt can not get right stats of a rbd pool
by yue 29 Oct '12

29 Oct '12

Allocation exceed Capacity ,but Available is not 0. #virsh pool-info 2361a6d4-0edc-3534-87ae-e7ee09199921 Name: 2361a6d4-0edc-3534-87ae-e7ee09199921 UUID: 2361a6d4-0edc-3534-87ae-e7ee09199921 State: running Persistent: yes Autostart: no Capacity: 285.57 GiB Allocation: 489.89 GiB Available: 230.59 GiB

1 0

Re: [libvirt] [Xen-devel] libxl drivers for libvirt?
by Jim Fehlig 29 Oct '12

29 Oct '12

Ian Campbell wrote: > On Fri, 2012-10-26 at 14:03 +0100, George Dunlap wrote: > >> On 24/10/12 19:11, Jim Fehlig wrote: >> >>>> * What kind of Xen support for libxl is in the libvirt development >>>> branch, and do you have an idea when full support for 4.2 (at least, >>>> including migration, suspend/resume, &c) might be available? >>>> >>> Nothing has changed in git master over what is available in 0.10.2, but >>> we are now starting to pick up this work. Our priorities are to first >>> get the libxl driver compiling against 4.2 and all of the existing >>> functionality that works with 4.1 working with 4.2, followed by closing >>> the feature gap with the legacy xen driver, and finally closing the >>> feature gap with the qemu driver where it makes sense. This is >>> obviously quite a bit of work and any help would be appreciated :). >>> >>> BTW, we don't have any motivation to add features to the 4.1 version of >>> the libvirt libxl driver. >>> >> That sounds like a good plan. For 4.1, xend is still the default >> toolstack, and libxl is a "tech preview"; for 4.2, xl is the default >> toolstack, and (if I understand correctly) we're officially going to be >> supporting the libxl interface in a backwards-compatible way from here >> forward. >> >> It seems to me that if you have trouble supporting both libxl 4.2 and >> libxl 4.1, that it might be better just to drop 4.1 support and focus on >> 4.2. Ian Campbell / Jackson: Thoughts? >> CC'ing libvirt community to get their input. > > That's what I would do if I were them ;-) > I tend to agree that this might be the best approach. The libxl in Xen 4.1 is essentially a one-off and I question whether we should pollute the libvirt libxl driver code with support for a dead, tech preview interface. Xend is still the primary toolstack in Xen 4.1.x, and libvirt has a stable, functional driver for this toolstack that can (should) be used in Xen 4.1.x deployments. Is anyone using the libvirt libxl driver with Xen 4.1 for anything other than development or experimentation? > Another option would be to fork into 4.1 and 4.2+ versions and to stop > adding new stuff to the 4.1 copy. That has its own downsides though. > I'd like to hear what the libvirt community thinks about only supporting Xen >= 4.2 in the libxl driver. Regards, Jim

2 1

[libvirt] libxl driver doesn't build with xen 4.2
by Cole Robinson 29 Oct '12

29 Oct '12

Hi Jim, The libvirt libxl driver doesn't build with xen 4.2. There was a small thread about it on fedora's virt list: http://lists.fedoraproject.org/pipermail/virt/2012-October/003446.html I saw that you had a few xen 4.2 patches land recently, and I'm just curious if you were aware of the libxl issues/possibly had any patches brewing. Currently in fedora we've disabled building the libxl driver so the xen guys can pull in their latest bits. Thanks, Cole

2 2

Re: [libvirt] Can we run guest OS without using NAT and iptables?
by Stefan Hajnoczi 29 Oct '12

29 Oct '12

On Mon, Oct 29, 2012 at 12:55:43PM +0530, freak 62 wrote: > Can we run guest o.s. on KVM without enabling NAT and iptables? > > The reason to do this is , I wanted to disable conntrack module > from my system and to disable that I must have to delete iptable and > NAT. > > I am getting the following message, when I start guest o.s. on > KVM (iptable and NAT disabled): > > Error starting domain: internal error 'Network default' is not active. > > Is their any way to run guest o.s. with NAT disabled? or Is their > any way to disable conntrack module and still can use KVM to run guest > OS ? > > I am using Ubuntu 10.04 This is a libvirt question since libvirt sets up the networking configuration. You can try a different network config either using the virt-manager GUI tool or by editing the network XML, which is documented here: http://libvirt.org/formatnetwork.html CCed libvirt mailing list. Stefan

2 1

[libvirt] [PATCH] daemon: Fix LIBVIRT_DEBUG=1 default output
by Cole Robinson 29 Oct '12

29 Oct '12

This commit changes the behavior of LIBVIRT_DEBUG=1 libvirtd: $ git show 7022b09111d4322d21396a70d58320a9ad773962 commit 7022b09111d4322d21396a70d58320a9ad773962 Author: Daniel P. Berrange <berrange(a)redhat.com> Date: Thu Sep 27 13:13:09 2012 +0100 Automatically enable systemd journal logging Probe to see if the systemd journal is accessible, and if so enable logging to the journal by default, rather than stderr (current default under systemd). Previously 'LIBVIRT_DEBUG=1 /usr/sbin/libvirtd' would show all debug output to stderr, now it send debug output to the journal. Only use the journal by default if running in daemon mode, or if stdin is _not_ a tty. This should make libvirtd launched from systemd use the journal, but preserve the old behavior in most situations. --- daemon/libvirtd.c | 9 ++++++--- src/util/logging.c | 2 ++ 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index d2e0a04..279a9d1 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -636,10 +636,13 @@ daemonSetupLogging(struct daemonConfig *config, virLogParseOutputs(config->log_outputs); /* - * If no defined outputs, then first try to direct it - * to the systemd journal (if it exists).... + * If no defined outputs, and either running + * as daemon or not on a tty, then first try + * to direct it to the systemd journal + * (if it exists).... */ - if (virLogGetNbOutputs() == 0) { + if (virLogGetNbOutputs() == 0 && + (godaemon || isatty(STDIN_FILENO) != 1)) { char *tmp; if (access("/run/systemd/journal/socket", W_OK) >= 0) { if (virAsprintf(&tmp, "%d:journald", virLogGetDefaultPriority()) < 0) diff --git a/src/util/logging.c b/src/util/logging.c index 9a8bba1..dd43842 100644 --- a/src/util/logging.c +++ b/src/util/logging.c @@ -1247,6 +1247,8 @@ virLogParseOutputs(const char *outputs) if (cur == NULL) return -1; + VIR_DEBUG("outputs=%s", outputs); + virSkipSpaces(&cur); while (*cur != 0) { prio= virParseNumber(&cur); -- 1.7.11.7

3 6

[libvirt] [PATCH] [DRAFT] handle qemu event GUEST_PANICKED
by Hu Tao 29 Oct '12

29 Oct '12

This is for early review, comments are welcome! qemu is adding a method to detect guest panic, and a new event GUEST_PANICKED[1]. This patch adds support for GUEST_PANICKED. But I have two questions: - to react to GUEST_PANICKED, can xml element <on_crash> be used as the action to take, or do I have to invent another xml element to configure the action? - Currently libvirt relies on watchdog to detect guest panic, but this way is not that reliable(there has to be a watchdog device, the watchdog itself has to be working when panic occurs), so it seems to be appropriate to remove this method from libvirt as there is a better one. But, will the removal cause any problems, such as backward compatibility, user confusion, etc.? [1] http://lists.nongnu.org/archive/html/qemu-devel/2012-10/msg04361.html --- src/qemu/qemu_command.c | 31 ++++++++++++++++++++----------- src/qemu/qemu_monitor.c | 10 ++++++++++ src/qemu/qemu_monitor.h | 3 +++ src/qemu/qemu_monitor_json.c | 9 +++++++++ src/qemu/qemu_process.c | 9 +++++++++ 5 files changed, 51 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 898c4c0..0c3f9ff 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4300,6 +4300,9 @@ qemuBuildMachineArgStr(virCommandPtr cmd, const virDomainDefPtr def, qemuCapsPtr caps) { + virBuffer params = VIR_BUFFER_INITIALIZER; + char *tmp = NULL; + /* This should *never* be NULL, since we always provide * a machine in the capabilities data for QEMU. So this * check is just here as a safety in case the unexpected @@ -4307,29 +4310,35 @@ qemuBuildMachineArgStr(virCommandPtr cmd, if (!def->os.machine) return 0; - if (!def->mem.dump_core) { - /* if no parameter to the machine type is needed, we still use - * '-M' to keep the most of the compatibility with older versions. - */ - virCommandAddArgList(cmd, "-M", def->os.machine, NULL); - } else { + virBufferAsprintf(&params, ",enable_pv_event=on"); + + if (def->mem.dump_core) { if (!qemuCapsGet(caps, QEMU_CAPS_DUMP_GUEST_CORE)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("dump-guest-core is not available " " with this QEMU binary")); return -1; } + virBufferAsprintf(&params,",%s", + virDomainMemDumpTypeToString(def->mem.dump_core)); + } - /* However, in case there is a parameter to be added, we need to + tmp = virBufferContentAndReset(&params); + if (!tmp || strlen(tmp) == 0) { + /* if no parameter to the machine type is needed, we still use + * '-M' to keep the most of the compatibility with older versions. + */ + virCommandAddArgList(cmd, "-M", def->os.machine, NULL); + } else { + /* However, in case there are parameters to be added, we need to * use the "-machine" parameter because qemu is not parsing the * "-M" correctly */ virCommandAddArg(cmd, "-machine"); - virCommandAddArgFormat(cmd, - "%s,dump-guest-core=%s", - def->os.machine, - virDomainMemDumpTypeToString(def->mem.dump_core)); + virCommandAddArgFormat(cmd, "%s%s", def->os.machine, tmp); } + VIR_FREE(tmp); + return 0; } diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 2d9c44c..8ab7ed2 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1146,6 +1146,16 @@ int qemuMonitorEmitBalloonChange(qemuMonitorPtr mon, } +int qemuMonitorEmitPanic(qemuMonitorPtr mon) +{ + int ret = -1; + VIR_DEBUG("mon=%p", mon); + + QEMU_MONITOR_CALLBACK(mon, ret, domainPanic, mon->vm); + return ret; +} + + int qemuMonitorSetCapabilities(qemuMonitorPtr mon) { int ret; diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 8856d9f..ea0a474 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -138,6 +138,8 @@ struct _qemuMonitorCallbacks { unsigned long long actual); int (*domainPMSuspendDisk)(qemuMonitorPtr mon, virDomainObjPtr vm); + int (*domainPanic)(qemuMonitorPtr mon, + virDomainObjPtr vm); }; char *qemuMonitorEscapeArg(const char *in); @@ -216,6 +218,7 @@ int qemuMonitorEmitBlockJob(qemuMonitorPtr mon, int qemuMonitorEmitBalloonChange(qemuMonitorPtr mon, unsigned long long actual); int qemuMonitorEmitPMSuspendDisk(qemuMonitorPtr mon); +int qemuMonitorEmitPanic(qemuMonitorPtr mon); int qemuMonitorStartCPUs(qemuMonitorPtr mon, virConnectPtr conn); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index e495c0a..0adbefc 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -71,6 +71,8 @@ static void qemuMonitorJSONHandleBlockJobCompleted(qemuMonitorPtr mon, virJSONVa static void qemuMonitorJSONHandleBlockJobCanceled(qemuMonitorPtr mon, virJSONValuePtr data); static void qemuMonitorJSONHandleBalloonChange(qemuMonitorPtr mon, virJSONValuePtr data); static void qemuMonitorJSONHandlePMSuspendDisk(qemuMonitorPtr mon, virJSONValuePtr data); +static void qemuMonitorJSONHandlePanic(qemuMonitorPtr mon, virJSONValuePtr data); + typedef struct { const char *type; @@ -83,6 +85,7 @@ static qemuEventHandler eventHandlers[] = { { "BLOCK_JOB_CANCELLED", qemuMonitorJSONHandleBlockJobCanceled, }, { "BLOCK_JOB_COMPLETED", qemuMonitorJSONHandleBlockJobCompleted, }, { "DEVICE_TRAY_MOVED", qemuMonitorJSONHandleTrayChange, }, + { "GUEST_PANICKED", qemuMonitorJSONHandlePanic, }, { "POWERDOWN", qemuMonitorJSONHandlePowerdown, }, { "RESET", qemuMonitorJSONHandleReset, }, { "RTC_CHANGE", qemuMonitorJSONHandleRTCChange, }, @@ -4417,3 +4420,9 @@ cleanup: virJSONValueFree(reply); return ret; } + +static void qemuMonitorJSONHandlePanic(qemuMonitorPtr mon, + virJSONValuePtr data ATTRIBUTE_UNUSED) +{ + qemuMonitorEmitPanic(mon); +} diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 969e3ce..d3b37e0 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1235,6 +1235,14 @@ qemuProcessHandlePMSuspendDisk(qemuMonitorPtr mon ATTRIBUTE_UNUSED, return 0; } +static int +qemuProcessHandlePanic(qemuMonitorPtr mon ATTRIBUTE_UNUSED, + virDomainObjPtr vm ATTRIBUTE_UNUSED) +{ + VIR_WARN("guest panicked."); + return 0; +} + static qemuMonitorCallbacks monitorCallbacks = { .destroy = qemuProcessHandleMonitorDestroy, @@ -1254,6 +1262,7 @@ static qemuMonitorCallbacks monitorCallbacks = { .domainPMSuspend = qemuProcessHandlePMSuspend, .domainBalloonChange = qemuProcessHandleBalloonChange, .domainPMSuspendDisk = qemuProcessHandlePMSuspendDisk, + .domainPanic = qemuProcessHandlePanic, }; static int -- 1.7.10.2

2 1

[libvirt] [PATCH] util: Re-format literal strings in virXMLEmitWarning
by Peter Krempa 29 Oct '12

29 Oct '12

And drop a stray space at the end of the first line of the warning. --- Yes, some lines exceed 80 columns. --- src/util/xml.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/util/xml.c b/src/util/xml.c index dad9227..30323b3 100644 --- a/src/util/xml.c +++ b/src/util/xml.c @@ -796,13 +796,15 @@ static int virXMLEmitWarning(int fd, const char *cmd) { size_t len; - const char *prologue = "\n\n"; + const char *prologue = + "\n\n"; if (fd < 0 || !cmd) { errno = EINVAL; -- 1.7.12.4

2 2

[libvirt] [PATCH v3] xml: print uuids in the warning
by Ján Tomko 29 Oct '12

29 Oct '12

In the XML warning, we print a virsh command line that can be used to edit that XML. This patch prints UUIDs if the entity name contains special characters (like shell metacharacters, or "--" that would break parsing of the XML comment). If the entity doesn't have a UUID, just print the virsh command that can be used to edit it. --- As opposed to previous versions, this doesn't use virBuffer and it doesn't do any shell escaping at all. The name parameter can be null now. commit 9b704ab8235af010b1fda4886201aab02098b969 xml: omit domain name from comment if it contains double hyphen only checked the 'name' parameter of virXMLEmitWarning. virXMLSaveFile invocation when creating a snapshot put the domain name in 'cmd', which means that snapshot XMLs of a domain with "--" in the name still can't be parsed by libvirt. --- src/conf/domain_conf.c | 6 +++++- src/conf/network_conf.c | 6 +++++- src/conf/nwfilter_conf.c | 12 ++++++++++-- src/conf/storage_conf.c | 6 +++++- src/libvirt_private.syms | 1 + src/parallels/parallels_storage.c | 3 +-- src/qemu/qemu_domain.c | 9 +-------- src/util/xml.c | 18 +++++++++++++----- src/util/xml.h | 1 + 9 files changed, 42 insertions(+), 20 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 90ec9aa..7bf0282 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14288,6 +14288,7 @@ int virDomainSaveXML(const char *configDir, virDomainDefPtr def, const char *xml) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; char *configFile = NULL; int ret = -1; @@ -14301,7 +14302,10 @@ int virDomainSaveXML(const char *configDir, goto cleanup; } - ret = virXMLSaveFile(configFile, def->name, "edit", xml); + virUUIDFormat(def->uuid, uuidstr); + ret = virXMLSaveFile(configFile, + virXMLPickShellSafeComment(def->name, uuidstr), "edit", + xml); cleanup: VIR_FREE(configFile); diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 8976f2a..a55339d 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -1978,6 +1978,7 @@ int virNetworkSaveXML(const char *configDir, virNetworkDefPtr def, const char *xml) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; char *configFile = NULL; int ret = -1; @@ -1991,7 +1992,10 @@ int virNetworkSaveXML(const char *configDir, goto cleanup; } - ret = virXMLSaveFile(configFile, def->name, "net-edit", xml); + virUUIDFormat(def->uuid, uuidstr); + ret = virXMLSaveFile(configFile, + virXMLPickShellSafeComment(def->name, uuidstr), + "net-edit", xml); cleanup: VIR_FREE(configFile); diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index 27dbee8..b6d5d40 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -2717,6 +2717,7 @@ int virNWFilterSaveXML(const char *configDir, virNWFilterDefPtr def, const char *xml) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; char *configFile = NULL; int ret = -1; @@ -2730,7 +2731,10 @@ int virNWFilterSaveXML(const char *configDir, goto cleanup; } - ret = virXMLSaveFile(configFile, def->name, "nwfilter-edit", xml); + virUUIDFormat(def->uuid, uuidstr); + ret = virXMLSaveFile(configFile, + virXMLPickShellSafeComment(def->name, uuidstr), + "nwfilter-edit", xml); cleanup: VIR_FREE(configFile); @@ -3151,6 +3155,7 @@ virNWFilterObjSaveDef(virNWFilterDriverStatePtr driver, virNWFilterObjPtr nwfilter, virNWFilterDefPtr def) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; char *xml; int ret; @@ -3174,7 +3179,10 @@ virNWFilterObjSaveDef(virNWFilterDriverStatePtr driver, return -1; } - ret = virXMLSaveFile(nwfilter->configFile, def->name, "nwfilter-edit", xml); + virUUIDFormat(def->uuid, uuidstr); + ret = virXMLSaveFile(nwfilter->configFile, + virXMLPickShellSafeComment(def->name, uuidstr), + "nwfilter-edit", xml); VIR_FREE(xml); return ret; diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 5d9e61a..9a765d8 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1637,6 +1637,7 @@ virStoragePoolObjSaveDef(virStorageDriverStatePtr driver, virStoragePoolObjPtr pool, virStoragePoolDefPtr def) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; char *xml; int ret = -1; @@ -1666,7 +1667,10 @@ virStoragePoolObjSaveDef(virStorageDriverStatePtr driver, return -1; } - ret = virXMLSaveFile(pool->configFile, def->name, "pool-edit", xml); + virUUIDFormat(def->uuid, uuidstr); + ret = virXMLSaveFile(pool->configFile, + virXMLPickShellSafeComment(def->name, uuidstr), + "pool-edit", xml); VIR_FREE(xml); return ret; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 8212726..02ca2c7 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1819,6 +1819,7 @@ virURIParse; # xml.h virXMLChildElementCount; virXMLParseHelper; +virXMLPickShellSafeComment; virXMLPropString; virXMLSaveFile; virXPathBoolean; diff --git a/src/parallels/parallels_storage.c b/src/parallels/parallels_storage.c index 76d885c..71f3f89 100644 --- a/src/parallels/parallels_storage.c +++ b/src/parallels/parallels_storage.c @@ -973,8 +973,7 @@ parallelsStorageVolumeDefine(virStoragePoolObjPtr pool, if (!xml_path) goto cleanup; - if (virXMLSaveFile(xml_path, privvol->name, - "volume-create", xmldesc)) { + if (virXMLSaveFile(xml_path, NULL, "volume-create", xmldesc)) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("Can't create file with volume description")); goto cleanup; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 4e6a5e9..d01e366 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1599,7 +1599,6 @@ qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm, char *snapDir = NULL; char *snapFile = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; - char *tmp; virUUIDFormat(vm->def->uuid, uuidstr); newxml = virDomainSnapshotDefFormat(uuidstr, snapshot->def, @@ -1622,13 +1621,7 @@ qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm, goto cleanup; } - if (virAsprintf(&tmp, "snapshot-edit %s", vm->def->name) < 0) { - virReportOOMError(); - goto cleanup; - } - - ret = virXMLSaveFile(snapFile, snapshot->def->name, tmp, newxml); - VIR_FREE(tmp); + ret = virXMLSaveFile(snapFile, NULL, "snapshot-edit", newxml); cleanup: VIR_FREE(snapFile); diff --git a/src/util/xml.c b/src/util/xml.c index f3dc256..dad9227 100644 --- a/src/util/xml.c +++ b/src/util/xml.c @@ -780,6 +780,16 @@ error: goto cleanup; } +const char *virXMLPickShellSafeComment(const char *str1, const char *str2) +{ + if(str1 && !strpbrk(str1, "\r\t\n !\"#$&'()*;<>?[\\]^`{|}~") && + !strstr(str1, "--")) + return str1; + if(str2 && !strpbrk(str2, "\r\t\n !\"#$&'()*;<>?[\\]^`{|}~") && + !strstr(str2, "--")) + return str2; + return NULL; +} static int virXMLEmitWarning(int fd, const char *name, @@ -794,7 +804,7 @@ OVERWRITTEN AND LOST. Changes to this xml configuration should be made using:\n\ or other application using the libvirt API.\n\ -->\n\n"; - if (fd < 0 || !name || !cmd) { + if (fd < 0 || !cmd) { errno = EINVAL; return -1; } @@ -807,9 +817,7 @@ or other application using the libvirt API.\n\ if (safewrite(fd, cmd, len) != len) return -1; - /* Omit the domain name if it contains a double hyphen - * because they aren't allowed in XML comments */ - if (!strstr(name, "--")) { + if (name) { if (safewrite(fd, " ", 1) != 1) return -1; @@ -837,7 +845,7 @@ virXMLRewriteFile(int fd, void *opaque) { struct virXMLRewriteFileData *data = opaque; - if (data->warnName && data->warnCommand) { + if (data->warnCommand) { if (virXMLEmitWarning(fd, data->warnName, data->warnCommand) < 0) return -1; } diff --git a/src/util/xml.h b/src/util/xml.h index a3750fa..c3b05df 100644 --- a/src/util/xml.h +++ b/src/util/xml.h @@ -61,6 +61,7 @@ xmlDocPtr virXMLParseHelper(int domcode, const char *url, xmlXPathContextPtr *pctxt); +const char *virXMLPickShellSafeComment(const char *str1, const char *str2); /** * virXMLParse: * @filename: file to parse, or NULL for string parsing -- 1.7.8.6

2 1