[libvirt] [PATCH 0/5] Support for marking domains as "tainted" (v2)
There are various configurations for virtual domains that we allow, but do not wish to actively support in production environments. The support situation is similar to that of binary only or non-GPL kernel modules, so borrow the kernel's idea of "tainting". When an undesirable configuration is used on a running VM, set a suitable taint flag and log a warning. OS distro bug triagers can see these warnings and decide whether to support users filing bugs in this scenarios In v2: - Split up the 3rd patch into 3 pieces - Add a dedicated API for appending a formatted string to the domain logfile (optionally opening the log if not already opened) - Use 'enum virDomainTaintFlags' for parameters instead of 'int'
Some configuration setups for guests are allowed, but strongly discouraged and unsupportable in production systems. Introduce a concept of 'tainting' to virDomainObjPtr to allow such setups to be identified. Drivers can then log warnings at suitable times * src/conf/domain_conf.c, src/conf/domain_conf.h: Declare taint flags and add parsing/formatting of domain status XML --- src/conf/domain_conf.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 16 ++++++++++++++ src/libvirt_private.syms | 3 ++ 3 files changed, 70 insertions(+), 0 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2a681d9..8fe375f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -50,6 +50,13 @@ #define VIR_FROM_THIS VIR_FROM_DOMAIN +VIR_ENUM_IMPL(virDomainTaint, VIR_DOMAIN_TAINT_LAST, + "custom-argv", + "custom-monitor", + "high-privileges", + "shell-scripts", + "disk-probing"); + VIR_ENUM_IMPL(virDomainVirt, VIR_DOMAIN_VIRT_LAST, "qemu", "kqemu", @@ -510,6 +517,20 @@ virDomainObjPtr virDomainFindByName(const virDomainObjListPtr doms, return obj; } + +bool virDomainObjTaint(virDomainObjPtr obj, + enum virDomainTaintFlags taint) +{ + int flag = (1 << taint); + + if (obj->taint & flag) + return false; + + obj->taint |= flag; + return true; +} + + static void virDomainGraphicsAuthDefClear(virDomainGraphicsAuthDefPtr def) { @@ -6250,6 +6271,8 @@ static virDomainObjPtr virDomainObjParseXML(virCapsPtr caps, xmlNodePtr config; xmlNodePtr oldnode; virDomainObjPtr obj; + xmlNodePtr *nodes = NULL; + int i, n; if (!(obj = virDomainObjNew(caps))) return NULL; @@ -6288,6 +6311,26 @@ static virDomainObjPtr virDomainObjParseXML(virCapsPtr caps, } obj->pid = (pid_t)val; + if ((n = virXPathNodeSet("./taint", ctxt, &nodes)) < 0) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("failed to parse taint flags")); + goto error; + } + for (i = 0 ; i < n ; i++) { + char *str = virXMLPropString(nodes[i], "flag"); + if (str) { + int flag = virDomainTaintTypeFromString(str); + VIR_FREE(str); + if (flag < 0) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown taint flag %s"), str); + goto error; + } + virDomainObjTaint(obj, flag); + } + } + VIR_FREE(nodes); + if (caps->privateDataXMLParse && ((caps->privateDataXMLParse)(ctxt, obj->privateData)) < 0) goto error; @@ -6297,6 +6340,7 @@ static virDomainObjPtr virDomainObjParseXML(virCapsPtr caps, error: /* obj was never shared, so unref should return 0 */ ignore_value(virDomainObjUnref(obj)); + VIR_FREE(nodes); return NULL; } @@ -8454,11 +8498,18 @@ static char *virDomainObjFormat(virCapsPtr caps, { char *config_xml = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; + int i; virBufferVSprintf(&buf, "<domstatus state='%s' pid='%d'>\n", virDomainStateTypeToString(obj->state), obj->pid); + for (i = 0 ; i < VIR_DOMAIN_TAINT_LAST ; i++) { + if (obj->taint & (1 << i)) + virBufferVSprintf(&buf, " <taint flag='%s'/>\n", + virDomainTaintTypeToString(i)); + } + if (caps->privateDataXMLFormat && ((caps->privateDataXMLFormat)(&buf, obj->privateData)) < 0) goto error; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 1dadf98..a0f820c 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1182,6 +1182,16 @@ struct _virDomainDef { virDomainXMLNamespace ns; }; +enum virDomainTaintFlags { + VIR_DOMAIN_TAINT_CUSTOM_ARGV, /* Custom ARGV passthrough from XML */ + VIR_DOMAIN_TAINT_CUSTOM_MONITOR, /* Custom monitor commands issued */ + VIR_DOMAIN_TAINT_HIGH_PRIVILEGES, /* Running with undesirably high privileges */ + VIR_DOMAIN_TAINT_SHELL_SCRIPTS, /* Network configuration using opaque shell scripts */ + VIR_DOMAIN_TAINT_DISK_PROBING, /* Relying on potentially unsafe disk format probing */ + + VIR_DOMAIN_TAINT_LAST +}; + /* Guest VM runtime state */ typedef struct _virDomainObj virDomainObj; typedef virDomainObj *virDomainObjPtr; @@ -1204,6 +1214,8 @@ struct _virDomainObj { void *privateData; void (*privateDataFreeFunc)(void *); + + int taint; }; typedef struct _virDomainObjList virDomainObjList; @@ -1231,6 +1243,8 @@ virDomainObjPtr virDomainFindByUUID(const virDomainObjListPtr doms, virDomainObjPtr virDomainFindByName(const virDomainObjListPtr doms, const char *name); +bool virDomainObjTaint(virDomainObjPtr obj, + enum virDomainTaintFlags taint); void virDomainGraphicsDefFree(virDomainGraphicsDefPtr def); void virDomainInputDefFree(virDomainInputDefPtr def); @@ -1429,6 +1443,8 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, typedef const char* (*virLifecycleToStringFunc)(int type); typedef int (*virLifecycleFromStringFunc)(const char *type); +VIR_ENUM_DECL(virDomainTaint) + VIR_ENUM_DECL(virDomainVirt) VIR_ENUM_DECL(virDomainBoot) VIR_ENUM_DECL(virDomainFeature) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 1b22be6..508b044 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -300,6 +300,7 @@ virDomainObjListNumOfDomains; virDomainObjLock; virDomainObjRef; virDomainObjSetDefTransient; +virDomainObjTaint; virDomainObjUnlock; virDomainObjUnref; virDomainRemoveInactive; @@ -324,6 +325,8 @@ virDomainSoundModelTypeFromString; virDomainSoundModelTypeToString; virDomainStateTypeFromString; virDomainStateTypeToString; +virDomainTaintTypeFromString; +virDomainTaintTypeToString; virDomainTimerModeTypeFromString; virDomainTimerModeTypeToString; virDomainTimerNameTypeFromString; -- 1.7.4.4
On 05/05/2011 05:51 AM, Daniel P. Berrange wrote:
Some configuration setups for guests are allowed, but strongly discouraged and unsupportable in production systems. Introduce a concept of 'tainting' to virDomainObjPtr to allow such setups to be identified. Drivers can then log warnings at suitable times
* src/conf/domain_conf.c, src/conf/domain_conf.h: Declare taint flags and add parsing/formatting of domain status XML
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Wire up logging of VM taintint to the QEMU driver - If running QEMU as root user/group or without capabilities being cleared - If passing custom QEMU command line args - If issuing custom QEMU monitor commands - If using a network interface config with an associated shell script - If using a disk config relying on format probing The warnings, per-VM appear in the main libvirtd logs 11:56:17.571: 10832: warning : qemuDomainObjTaint:712 : Domain id=1 name='l2' uuid=c7a3edbd-edaf-9455-926a-d65c16db1802 is tainted: high-privileges 11:56:17.571: 10832: warning : qemuDomainObjTaint:712 : Domain id=1 name='l2' uuid=c7a3edbd-edaf-9455-926a-d65c16db1802 is tainted: disk-probing The taint flags are reset when the VM is stopped. * src/qemu/qemu_domain.c, src/qemu/qemu_domain.h: Helper APIs for logging taint warnings * src/qemu/qemu_driver.c: Log tainting with custom QEMU monitor commands and disk/net hotplug with unsupported configs * src/qemu/qemu_process.c: Log tainting at startup based on unsupported configs --- src/qemu/qemu_domain.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 14 +++++++++- src/qemu/qemu_driver.c | 8 ++--- src/qemu/qemu_process.c | 4 ++- 4 files changed, 82 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 3033ff5..938c063 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -33,6 +33,7 @@ #include "event.h" #include "cpu/cpu.h" #include "ignore-value.h" +#include "uuid.h" #include <sys/time.h> @@ -742,3 +743,65 @@ cleanup: virCPUDefFree(cpu); return ret; } + +void qemuDomainObjTaint(struct qemud_driver *driver ATTRIBUTE_UNUSED, + virDomainObjPtr obj, + enum virDomainTaintFlags taint) +{ + if (virDomainObjTaint(obj, taint)) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(obj->def->uuid, uuidstr); + + VIR_WARN("Domain id=%d name='%s' uuid=%s is tainted: %s", + obj->def->id, + obj->def->name, + uuidstr, + virDomainTaintTypeToString(taint)); + } +} + + +void qemuDomainObjCheckTaint(struct qemud_driver *driver, + virDomainObjPtr obj) +{ + int i; + + if (!driver->clearEmulatorCapabilities || + driver->user == 0 || + driver->group == 0) + qemuDomainObjTaint(driver, obj, VIR_DOMAIN_TAINT_HIGH_PRIVILEGES); + + if (obj->def->namespaceData) { + qemuDomainCmdlineDefPtr qemucmd = obj->def->namespaceData; + if (qemucmd->num_args || qemucmd->num_env) + qemuDomainObjTaint(driver, obj, VIR_DOMAIN_TAINT_CUSTOM_ARGV); + } + + for (i = 0 ; i < obj->def->ndisks ; i++) + qemuDomainObjCheckDiskTaint(driver, obj, obj->def->disks[i]); + + for (i = 0 ; i < obj->def->nnets ; i++) + qemuDomainObjCheckNetTaint(driver, obj, obj->def->nets[i]); +} + + +void qemuDomainObjCheckDiskTaint(struct qemud_driver *driver, + virDomainObjPtr obj, + virDomainDiskDefPtr disk) +{ + if (!disk->driverType && + driver->allowDiskFormatProbing) + qemuDomainObjTaint(driver, obj, VIR_DOMAIN_TAINT_DISK_PROBING); +} + + +void qemuDomainObjCheckNetTaint(struct qemud_driver *driver, + virDomainObjPtr obj, + virDomainNetDefPtr net) +{ + if ((net->type == VIR_DOMAIN_NET_TYPE_ETHERNET && + net->data.ethernet.script != NULL) || + (net->type == VIR_DOMAIN_NET_TYPE_BRIDGE && + net->data.bridge.script != NULL)) + qemuDomainObjTaint(driver, obj, VIR_DOMAIN_TAINT_SHELL_SCRIPTS); +} diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index b0ecc5a..9f454f8 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -70,7 +70,6 @@ struct _qemuDomainObjPrivate { qemuMonitorPtr mon; virDomainChrSourceDefPtr monConfig; int monJSON; - int monitor_warned; bool gotShutdown; int nvcpupids; @@ -116,4 +115,17 @@ char *qemuDomainFormatXML(struct qemud_driver *driver, virDomainObjPtr vm, int flags); +void qemuDomainObjTaint(struct qemud_driver *driver, + virDomainObjPtr obj, + enum virDomainTaintFlags taint); + +void qemuDomainObjCheckTaint(struct qemud_driver *driver, + virDomainObjPtr obj); +void qemuDomainObjCheckDiskTaint(struct qemud_driver *driver, + virDomainObjPtr obj, + virDomainDiskDefPtr disk); +void qemuDomainObjCheckNetTaint(struct qemud_driver *driver, + virDomainObjPtr obj, + virDomainNetDefPtr net); + #endif /* __QEMU_DOMAIN_H__ */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index dd89786..535a762 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3877,6 +3877,7 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm, switch (dev->type) { case VIR_DOMAIN_DEVICE_DISK: + qemuDomainObjCheckDiskTaint(driver, vm, dev->data.disk); ret = qemuDomainAttachDeviceDiskLive(driver, vm, dev); if (!ret) dev->data.disk = NULL; @@ -3889,6 +3890,7 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm, break; case VIR_DOMAIN_DEVICE_NET: + qemuDomainObjCheckNetTaint(driver, vm, dev->data.net); ret = qemuDomainAttachNetDevice(dom->conn, driver, vm, dev->data.net); if (!ret) @@ -6982,11 +6984,7 @@ static int qemuDomainMonitorCommand(virDomainPtr domain, const char *cmd, priv = vm->privateData; - if (!priv->monitor_warned) { - VIR_INFO("Qemu monitor command '%s' executed; libvirt results may be unpredictable!", - cmd); - priv->monitor_warned = 1; - } + qemuDomainObjTaint(driver, vm, VIR_DOMAIN_TAINT_CUSTOM_MONITOR); hmp = !!(flags & VIR_DOMAIN_QEMU_MONITOR_COMMAND_HMP); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 3460000..573336d 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2229,7 +2229,6 @@ int qemuProcessStart(virConnectPtr conn, #endif priv->monJSON = 0; - priv->monitor_warned = 0; priv->gotShutdown = false; if ((ret = virFileDeletePid(driver->stateDir, vm->def->name)) != 0) { @@ -2316,6 +2315,8 @@ int qemuProcessStart(virConnectPtr conn, virCommandWriteArgLog(cmd, logfile); + qemuDomainObjCheckTaint(driver, vm); + if ((pos = lseek(logfile, 0, SEEK_END)) < 0) VIR_WARN("Unable to seek to end of logfile: %s", virStrerror(errno, ebuf, sizeof ebuf)); @@ -2595,6 +2596,7 @@ retry: qemuProcessReturnPort(driver, vm->def->graphics[0]->data.spice.tlsPort); } + vm->taint = 0; vm->pid = -1; vm->def->id = -1; vm->state = VIR_DOMAIN_SHUTOFF; -- 1.7.4.4
On 05/05/2011 05:51 AM, Daniel P. Berrange wrote:
Wire up logging of VM taintint to the QEMU driver
s/taintint/tainting/
- If running QEMU as root user/group or without capabilities being cleared - If passing custom QEMU command line args - If issuing custom QEMU monitor commands - If using a network interface config with an associated shell script - If using a disk config relying on format probing
The warnings, per-VM appear in the main libvirtd logs
11:56:17.571: 10832: warning : qemuDomainObjTaint:712 : Domain id=1 name='l2' uuid=c7a3edbd-edaf-9455-926a-d65c16db1802 is tainted: high-privileges 11:56:17.571: 10832: warning : qemuDomainObjTaint:712 : Domain id=1 name='l2' uuid=c7a3edbd-edaf-9455-926a-d65c16db1802 is tainted: disk-probing
The taint flags are reset when the VM is stopped.
* src/qemu/qemu_domain.c, src/qemu/qemu_domain.h: Helper APIs for logging taint warnings * src/qemu/qemu_driver.c: Log tainting with custom QEMU monitor commands and disk/net hotplug with unsupported configs * src/qemu/qemu_process.c: Log tainting at startup based on unsupported configs
+++ b/src/qemu/qemu_driver.c @@ -3877,6 +3877,7 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm,
switch (dev->type) { case VIR_DOMAIN_DEVICE_DISK: + qemuDomainObjCheckDiskTaint(driver, vm, dev->data.disk); ret = qemuDomainAttachDeviceDiskLive(driver, vm, dev);
Do we really want to issue the taint message before attempting the attach? Or are we still safe if you perform the attach first, and issue the taint message only on success?
if (!ret) dev->data.disk = NULL; @@ -3889,6 +3890,7 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm, break;
case VIR_DOMAIN_DEVICE_NET: + qemuDomainObjCheckNetTaint(driver, vm, dev->data.net); ret = qemuDomainAttachNetDevice(dom->conn, driver, vm,
Likewise. On second thought - we can't guarantee if failure happened before a disk was probed, or after, so I guess doing unconditional taint is the conservatively safe approach, and the whole point of this is conservative safety even if it lets through some false positives. So, ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Thu, May 05, 2011 at 02:55:14PM -0600, Eric Blake wrote:
On 05/05/2011 05:51 AM, Daniel P. Berrange wrote:
Wire up logging of VM taintint to the QEMU driver
s/taintint/tainting/
- If running QEMU as root user/group or without capabilities being cleared - If passing custom QEMU command line args - If issuing custom QEMU monitor commands - If using a network interface config with an associated shell script - If using a disk config relying on format probing
The warnings, per-VM appear in the main libvirtd logs
11:56:17.571: 10832: warning : qemuDomainObjTaint:712 : Domain id=1 name='l2' uuid=c7a3edbd-edaf-9455-926a-d65c16db1802 is tainted: high-privileges 11:56:17.571: 10832: warning : qemuDomainObjTaint:712 : Domain id=1 name='l2' uuid=c7a3edbd-edaf-9455-926a-d65c16db1802 is tainted: disk-probing
The taint flags are reset when the VM is stopped.
* src/qemu/qemu_domain.c, src/qemu/qemu_domain.h: Helper APIs for logging taint warnings * src/qemu/qemu_driver.c: Log tainting with custom QEMU monitor commands and disk/net hotplug with unsupported configs * src/qemu/qemu_process.c: Log tainting at startup based on unsupported configs
+++ b/src/qemu/qemu_driver.c @@ -3877,6 +3877,7 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm,
switch (dev->type) { case VIR_DOMAIN_DEVICE_DISK: + qemuDomainObjCheckDiskTaint(driver, vm, dev->data.disk); ret = qemuDomainAttachDeviceDiskLive(driver, vm, dev);
Do we really want to issue the taint message before attempting the attach? Or are we still safe if you perform the attach first, and issue the taint message only on success?
Even if the attach operation gets half-way through and then fails I think we still want to have the taint message, because even the partial attempt may have had negative consequences. Taking that to its logical conclusion, we should always warn on any attempt to use a undesirable configuration, and thus might as well do it upfront, in case we crash later.
if (!ret) dev->data.disk = NULL; @@ -3889,6 +3890,7 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm, break;
case VIR_DOMAIN_DEVICE_NET: + qemuDomainObjCheckNetTaint(driver, vm, dev->data.net); ret = qemuDomainAttachNetDevice(dom->conn, driver, vm,
Likewise.
On second thought - we can't guarantee if failure happened before a disk was probed, or after, so I guess doing unconditional taint is the conservatively safe approach, and the whole point of this is conservative safety even if it lets through some false positives.
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 :|
Move the qemuProcessLogReadFD and qemuProcessLogFD methods into qemu_domain.c, renaming them to qemuDomainCreateLog and qemuDomainOpenLog. * src/qemu/qemu_domain.c, src/qemu/qemu_domain.h: Add qemuDomainCreateLog and qemuDomainOpenLog. * src/qemu/qemu_process.c: Remove qemuProcessLogFD and qemuProcessLogReadFD --- src/qemu/qemu_domain.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 4 ++ src/qemu/qemu_process.c | 82 ++-------------------------------------------- 3 files changed, 90 insertions(+), 79 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 938c063..204a628 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -34,8 +34,10 @@ #include "cpu/cpu.h" #include "ignore-value.h" #include "uuid.h" +#include "files.h" #include <sys/time.h> +#include <fcntl.h> #include <libxml/xpathInternals.h> @@ -805,3 +807,84 @@ void qemuDomainObjCheckNetTaint(struct qemud_driver *driver, net->data.bridge.script != NULL)) qemuDomainObjTaint(driver, obj, VIR_DOMAIN_TAINT_SHELL_SCRIPTS); } + + +static int +qemuDomainOpenLogHelper(struct qemud_driver *driver, + virDomainObjPtr vm, + int flags, + mode_t mode) +{ + char *logfile; + int fd = -1; + + if (virAsprintf(&logfile, "%s/%s.log", driver->logDir, vm->def->name) < 0) { + virReportOOMError(); + return -1; + } + + if ((fd = open(logfile, flags, mode)) < 0) { + virReportSystemError(errno, _("failed to create logfile %s"), + logfile); + goto cleanup; + } + if (virSetCloseExec(fd) < 0) { + virReportSystemError(errno, _("failed to set close-on-exec flag on %s"), + logfile); + VIR_FORCE_CLOSE(fd); + goto cleanup; + } + +cleanup: + VIR_FREE(logfile); + return fd; +} + + +int +qemuDomainCreateLog(struct qemud_driver *driver, virDomainObjPtr vm, bool append) +{ + int flags; + + flags = O_CREAT | O_WRONLY; + /* Only logrotate files in /var/log, so only append if running privileged */ + if (driver->privileged || append) + flags |= O_APPEND; + else + flags |= O_TRUNC; + + return qemuDomainOpenLogHelper(driver, vm, flags, S_IRUSR | S_IWUSR); +} + + +int +qemuDomainOpenLog(struct qemud_driver *driver, virDomainObjPtr vm, off_t pos) +{ + int fd; + off_t off; + int whence; + + if ((fd = qemuDomainOpenLogHelper(driver, vm, O_RDONLY, 0)) < 0) + return -1; + + if (pos < 0) { + off = 0; + whence = SEEK_END; + } else { + off = pos; + whence = SEEK_SET; + } + + if (lseek(fd, off, whence) < 0) { + virReportSystemError(pos < 0 ? 0 : errno, + _("Unable to seek to %lld from %s in log for %s"), + (long long)off, + whence == SEEK_END ? "end" : "start", + vm->def->name); + VIR_FORCE_CLOSE(fd); + } + + return fd; +} + + diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 9f454f8..dfb25fb 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -128,4 +128,8 @@ void qemuDomainObjCheckNetTaint(struct qemud_driver *driver, virDomainObjPtr obj, virDomainNetDefPtr net); + +int qemuDomainCreateLog(struct qemud_driver *driver, virDomainObjPtr vm, bool append); +int qemuDomainOpenLog(struct qemud_driver *driver, virDomainObjPtr vm, off_t pos); + #endif /* __QEMU_DOMAIN_H__ */ diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 573336d..eca85ae 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -679,82 +679,6 @@ error: return ret; } -static int -qemuProcessLogFD(struct qemud_driver *driver, const char* name, bool append) -{ - char *logfile; - mode_t logmode; - int fd = -1; - - if (virAsprintf(&logfile, "%s/%s.log", driver->logDir, name) < 0) { - virReportOOMError(); - return -1; - } - - logmode = O_CREAT | O_WRONLY; - /* Only logrotate files in /var/log, so only append if running privileged */ - if (driver->privileged || append) - logmode |= O_APPEND; - else - logmode |= O_TRUNC; - - if ((fd = open(logfile, logmode, S_IRUSR | S_IWUSR)) < 0) { - virReportSystemError(errno, - _("failed to create logfile %s"), - logfile); - VIR_FREE(logfile); - return -1; - } - VIR_FREE(logfile); - if (virSetCloseExec(fd) < 0) { - virReportSystemError(errno, "%s", - _("Unable to set VM logfile close-on-exec flag")); - VIR_FORCE_CLOSE(fd); - return -1; - } - return fd; -} - - -static int -qemuProcessLogReadFD(const char* logDir, const char* name, off_t pos) -{ - char *logfile; - mode_t logmode = O_RDONLY; - int fd = -1; - - if (virAsprintf(&logfile, "%s/%s.log", logDir, name) < 0) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, - _("failed to build logfile name %s/%s.log"), - logDir, name); - return -1; - } - - if ((fd = open(logfile, logmode)) < 0) { - virReportSystemError(errno, - _("failed to create logfile %s"), - logfile); - VIR_FREE(logfile); - return -1; - } - if (virSetCloseExec(fd) < 0) { - virReportSystemError(errno, "%s", - _("Unable to set VM logfile close-on-exec flag")); - VIR_FORCE_CLOSE(fd); - VIR_FREE(logfile); - return -1; - } - if (pos < 0 || lseek(fd, pos, SEEK_SET) < 0) { - virReportSystemError(pos < 0 ? 0 : errno, - _("Unable to seek to %lld in %s"), - (long long) pos, logfile); - VIR_FORCE_CLOSE(fd); - } - VIR_FREE(logfile); - return fd; -} - - typedef int qemuProcessLogHandleOutput(virDomainObjPtr vm, const char *output, int fd); @@ -1051,7 +975,7 @@ qemuProcessWaitForMonitor(struct qemud_driver* driver, virHashTablePtr paths = NULL; qemuDomainObjPrivatePtr priv; - if ((logfd = qemuProcessLogReadFD(driver->logDir, vm->def->name, pos)) < 0) + if ((logfd = qemuDomainOpenLog(driver, vm, pos)) < 0) return -1; if (VIR_ALLOC_N(buf, buf_size) < 0) { @@ -2198,7 +2122,7 @@ int qemuProcessStart(virConnectPtr conn, } VIR_DEBUG0("Creating domain log file"); - if ((logfile = qemuProcessLogFD(driver, vm->def->name, false)) < 0) + if ((logfile = qemuDomainCreateLog(driver, vm, false)) < 0) goto cleanup; VIR_DEBUG0("Determining emulator version"); @@ -2461,7 +2385,7 @@ void qemuProcessStop(struct qemud_driver *driver, return; } - if ((logfile = qemuProcessLogFD(driver, vm->def->name, true)) < 0) { + if ((logfile = qemuDomainCreateLog(driver, vm, true)) < 0) { /* To not break the normal domain shutdown process, skip the * timestamp log writing if failed on opening log file. */ VIR_WARN("Unable to open logfile: %s", -- 1.7.4.4
On 05/05/2011 05:51 AM, Daniel P. Berrange wrote:
Move the qemuProcessLogReadFD and qemuProcessLogFD methods into qemu_domain.c, renaming them to qemuDomainCreateLog and qemuDomainOpenLog.
* src/qemu/qemu_domain.c, src/qemu/qemu_domain.h: Add qemuDomainCreateLog and qemuDomainOpenLog. * src/qemu/qemu_process.c: Remove qemuProcessLogFD and qemuProcessLogReadFD
+static int +qemuDomainOpenLogHelper(struct qemud_driver *driver, + virDomainObjPtr vm, + int flags, + mode_t mode) +{ + char *logfile; + int fd = -1; + + if (virAsprintf(&logfile, "%s/%s.log", driver->logDir, vm->def->name) < 0) { + virReportOOMError(); + return -1; + } + + if ((fd = open(logfile, flags, mode)) < 0) { + virReportSystemError(errno, _("failed to create logfile %s"), + logfile); + goto cleanup; + } + if (virSetCloseExec(fd) < 0) {
I still need to implement O_CLOEXEC in gnulib. Someday...
+ virReportSystemError(errno, _("failed to set close-on-exec flag on %s"), + logfile); + VIR_FORCE_CLOSE(fd); + goto cleanup; + } + +cleanup: + VIR_FREE(logfile); + return fd; +
Not quite straight code motion - you rewrote the error handling to be saner. But I like it (it's a small enough code motion to be reviewable, not like some of those thousand-liners in the past). }
+ + +int +qemuDomainCreateLog(struct qemud_driver *driver, virDomainObjPtr vm, bool append) +{ + int flags; + + flags = O_CREAT | O_WRONLY; + /* Only logrotate files in /var/log, so only append if running privileged */ + if (driver->privileged || append) + flags |= O_APPEND; + else + flags |= O_TRUNC; + + return qemuDomainOpenLogHelper(driver, vm, flags, S_IRUSR | S_IWUSR); +} + + +int +qemuDomainOpenLog(struct qemud_driver *driver, virDomainObjPtr vm, off_t pos) +{ + int fd; + off_t off; + int whence; + + if ((fd = qemuDomainOpenLogHelper(driver, vm, O_RDONLY, 0)) < 0) + return -1; + + if (pos < 0) { + off = 0; + whence = SEEK_END; + } else { + off = pos; + whence = SEEK_SET; + } + + if (lseek(fd, off, whence) < 0) { + virReportSystemError(pos < 0 ? 0 : errno, + _("Unable to seek to %lld from %s in log for %s"), + (long long)off, + whence == SEEK_END ? "end" : "start",
_("end"), _("start") otherwise the translation looks funny for injecting an English word into a foreign sentence In fact, translators would probably prefer that you had two strings: whence == SEEK_END ? _("unable to seek to end of log for %$2s") : _("unable to seek to %lld from start for %s"), (long long) off, vm->def->name -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
The qemuDomainAppendLog method allows writing a formatted string to the end of the domain logfile, optionally opening it if needed. * src/qemu/qemu_domain.c, src/qemu/qemu_domain.h: Add qemuDomainAppendLog --- src/qemu/qemu_domain.c | 37 +++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 4 ++++ 2 files changed, 41 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 204a628..694c637 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -888,3 +888,40 @@ qemuDomainOpenLog(struct qemud_driver *driver, virDomainObjPtr vm, off_t pos) } +ATTRIBUTE_FMT_PRINTF(4, 5) +int qemuDomainAppendLog(struct qemud_driver *driver, + virDomainObjPtr obj, + int logFD, + const char *fmt, ...) +{ + int fd = logFD; + va_list argptr; + char *message = NULL; + int ret = -1; + + va_start(argptr, fmt); + + if ((fd == -1) && + (fd = qemuDomainCreateLog(driver, obj, true)) < 0) + goto cleanup; + + if (virVasprintf(&message, fmt, argptr) < 0) { + virReportOOMError(); + goto cleanup; + } + if (safewrite(logFD, message, strlen(message)) < 0) { + virReportSystemError(errno, _("Unable to write to domain logfile %s"), + obj->def->name); + goto cleanup; + } + + ret = 0; + +cleanup: + va_end(argptr); + + if (fd != logFD) + VIR_FORCE_CLOSE(fd); + + return ret; +} diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index dfb25fb..3e4d1ec 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -131,5 +131,9 @@ void qemuDomainObjCheckNetTaint(struct qemud_driver *driver, int qemuDomainCreateLog(struct qemud_driver *driver, virDomainObjPtr vm, bool append); int qemuDomainOpenLog(struct qemud_driver *driver, virDomainObjPtr vm, off_t pos); +int qemuDomainAppendLog(struct qemud_driver *driver, + virDomainObjPtr vm, + int logFD, + const char *fmt, ...); #endif /* __QEMU_DOMAIN_H__ */ -- 1.7.4.4
On 05/05/2011 05:51 AM, Daniel P. Berrange wrote:
The qemuDomainAppendLog method allows writing a formatted string to the end of the domain logfile, optionally opening it if needed.
* src/qemu/qemu_domain.c, src/qemu/qemu_domain.h: Add qemuDomainAppendLog --- src/qemu/qemu_domain.c | 37 +++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 4 ++++ 2 files changed, 41 insertions(+), 0 deletions(-)
+++ b/src/qemu/qemu_domain.c @@ -888,3 +888,40 @@ qemuDomainOpenLog(struct qemud_driver *driver, virDomainObjPtr vm, off_t pos) }
+ATTRIBUTE_FMT_PRINTF(4, 5) +int qemuDomainAppendLog(struct qemud_driver *driver,
If you put the ATTRIBUTE_FMT_PRINTF in the .h, then you don't need to copy it here. Plus, it will actually cause the compiler to do checking for files other than qemu_domain.c.
+++ b/src/qemu/qemu_domain.h @@ -131,5 +131,9 @@ void qemuDomainObjCheckNetTaint(struct qemud_driver *driver,
int qemuDomainCreateLog(struct qemud_driver *driver, virDomainObjPtr vm, bool append); int qemuDomainOpenLog(struct qemud_driver *driver, virDomainObjPtr vm, off_t pos); +int qemuDomainAppendLog(struct qemud_driver *driver, + virDomainObjPtr vm, + int logFD, + const char *fmt, ...);
ACK with the ATTRIBUTE moved to the .h. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
As well as taint warnings going to the main libvirt log, add taint warnings to the per-domain logfile Domain id=3 is tainted: high-privileges Domain id=3 is tainted: disk-probing Domain id=3 is tainted: shell-scripts Domain id=3 is tainted: custom-monitor * src/qemu/qemu_domain.c, src/qemu/qemu_domain.h: Enhance qemuDomainTaint to also log to the domain logfile * src/qemu/qemu_driver.c: Pass -1 for logFD to taint methods to auto-append to logfile * src/qemu/qemu_process.c: Pass open logFD at startup for taint methods --- src/qemu/qemu_domain.c | 44 ++++++++++++++++++++++++++++++++------------ src/qemu/qemu_domain.h | 12 ++++++++---- src/qemu/qemu_driver.c | 6 +++--- src/qemu/qemu_process.c | 2 +- 4 files changed, 44 insertions(+), 20 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 694c637..92940f5 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -746,10 +746,13 @@ cleanup: return ret; } -void qemuDomainObjTaint(struct qemud_driver *driver ATTRIBUTE_UNUSED, +void qemuDomainObjTaint(struct qemud_driver *driver, virDomainObjPtr obj, - enum virDomainTaintFlags taint) + enum virDomainTaintFlags taint, + int logFD) { + virErrorPtr orig_err = NULL; + if (virDomainObjTaint(obj, taint)) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(obj->def->uuid, uuidstr); @@ -759,53 +762,70 @@ void qemuDomainObjTaint(struct qemud_driver *driver ATTRIBUTE_UNUSED, obj->def->name, uuidstr, virDomainTaintTypeToString(taint)); + + /* We don't care about errors logging taint info, so + * preserve original error, and clear any error that + * is raised */ + orig_err = virSaveLastError(); + if (qemuDomainAppendLog(driver, obj, logFD, + "Domain id=%d is tainted: %s\n", + obj->def->id, + virDomainTaintTypeToString(taint)) < 0) + virResetLastError(); + if (orig_err) { + virSetError(orig_err); + virFreeError(orig_err); + } } } void qemuDomainObjCheckTaint(struct qemud_driver *driver, - virDomainObjPtr obj) + virDomainObjPtr obj, + int logFD) { int i; if (!driver->clearEmulatorCapabilities || driver->user == 0 || driver->group == 0) - qemuDomainObjTaint(driver, obj, VIR_DOMAIN_TAINT_HIGH_PRIVILEGES); + qemuDomainObjTaint(driver, obj, VIR_DOMAIN_TAINT_HIGH_PRIVILEGES, logFD); if (obj->def->namespaceData) { qemuDomainCmdlineDefPtr qemucmd = obj->def->namespaceData; if (qemucmd->num_args || qemucmd->num_env) - qemuDomainObjTaint(driver, obj, VIR_DOMAIN_TAINT_CUSTOM_ARGV); + qemuDomainObjTaint(driver, obj, VIR_DOMAIN_TAINT_CUSTOM_ARGV, logFD); } for (i = 0 ; i < obj->def->ndisks ; i++) - qemuDomainObjCheckDiskTaint(driver, obj, obj->def->disks[i]); + qemuDomainObjCheckDiskTaint(driver, obj, obj->def->disks[i], logFD); for (i = 0 ; i < obj->def->nnets ; i++) - qemuDomainObjCheckNetTaint(driver, obj, obj->def->nets[i]); + qemuDomainObjCheckNetTaint(driver, obj, obj->def->nets[i], logFD); } void qemuDomainObjCheckDiskTaint(struct qemud_driver *driver, virDomainObjPtr obj, - virDomainDiskDefPtr disk) + virDomainDiskDefPtr disk, + int logFD) { if (!disk->driverType && driver->allowDiskFormatProbing) - qemuDomainObjTaint(driver, obj, VIR_DOMAIN_TAINT_DISK_PROBING); + qemuDomainObjTaint(driver, obj, VIR_DOMAIN_TAINT_DISK_PROBING, logFD); } void qemuDomainObjCheckNetTaint(struct qemud_driver *driver, virDomainObjPtr obj, - virDomainNetDefPtr net) + virDomainNetDefPtr net, + int logFD) { if ((net->type == VIR_DOMAIN_NET_TYPE_ETHERNET && net->data.ethernet.script != NULL) || (net->type == VIR_DOMAIN_NET_TYPE_BRIDGE && net->data.bridge.script != NULL)) - qemuDomainObjTaint(driver, obj, VIR_DOMAIN_TAINT_SHELL_SCRIPTS); + qemuDomainObjTaint(driver, obj, VIR_DOMAIN_TAINT_SHELL_SCRIPTS, logFD); } @@ -909,7 +929,7 @@ int qemuDomainAppendLog(struct qemud_driver *driver, virReportOOMError(); goto cleanup; } - if (safewrite(logFD, message, strlen(message)) < 0) { + if (safewrite(fd, message, strlen(message)) < 0) { virReportSystemError(errno, _("Unable to write to domain logfile %s"), obj->def->name); goto cleanup; diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 3e4d1ec..fb1743f 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -117,16 +117,20 @@ char *qemuDomainFormatXML(struct qemud_driver *driver, void qemuDomainObjTaint(struct qemud_driver *driver, virDomainObjPtr obj, - enum virDomainTaintFlags taint); + enum virDomainTaintFlags taint, + int logFD); void qemuDomainObjCheckTaint(struct qemud_driver *driver, - virDomainObjPtr obj); + virDomainObjPtr obj, + int logFD); void qemuDomainObjCheckDiskTaint(struct qemud_driver *driver, virDomainObjPtr obj, - virDomainDiskDefPtr disk); + virDomainDiskDefPtr disk, + int logFD); void qemuDomainObjCheckNetTaint(struct qemud_driver *driver, virDomainObjPtr obj, - virDomainNetDefPtr net); + virDomainNetDefPtr net, + int logFD); int qemuDomainCreateLog(struct qemud_driver *driver, virDomainObjPtr vm, bool append); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 535a762..0fd0f10 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3877,7 +3877,7 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm, switch (dev->type) { case VIR_DOMAIN_DEVICE_DISK: - qemuDomainObjCheckDiskTaint(driver, vm, dev->data.disk); + qemuDomainObjCheckDiskTaint(driver, vm, dev->data.disk, -1); ret = qemuDomainAttachDeviceDiskLive(driver, vm, dev); if (!ret) dev->data.disk = NULL; @@ -3890,7 +3890,7 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm, break; case VIR_DOMAIN_DEVICE_NET: - qemuDomainObjCheckNetTaint(driver, vm, dev->data.net); + qemuDomainObjCheckNetTaint(driver, vm, dev->data.net, -1); ret = qemuDomainAttachNetDevice(dom->conn, driver, vm, dev->data.net); if (!ret) @@ -6984,7 +6984,7 @@ static int qemuDomainMonitorCommand(virDomainPtr domain, const char *cmd, priv = vm->privateData; - qemuDomainObjTaint(driver, vm, VIR_DOMAIN_TAINT_CUSTOM_MONITOR); + qemuDomainObjTaint(driver, vm, VIR_DOMAIN_TAINT_CUSTOM_MONITOR, -1); hmp = !!(flags & VIR_DOMAIN_QEMU_MONITOR_COMMAND_HMP); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index eca85ae..bd7c932 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2239,7 +2239,7 @@ int qemuProcessStart(virConnectPtr conn, virCommandWriteArgLog(cmd, logfile); - qemuDomainObjCheckTaint(driver, vm); + qemuDomainObjCheckTaint(driver, vm, logfile); if ((pos = lseek(logfile, 0, SEEK_END)) < 0) VIR_WARN("Unable to seek to end of logfile: %s", -- 1.7.4.4
On 05/05/2011 05:51 AM, Daniel P. Berrange wrote:
As well as taint warnings going to the main libvirt log, add taint warnings to the per-domain logfile
Domain id=3 is tainted: high-privileges Domain id=3 is tainted: disk-probing Domain id=3 is tainted: shell-scripts Domain id=3 is tainted: custom-monitor
* src/qemu/qemu_domain.c, src/qemu/qemu_domain.h: Enhance qemuDomainTaint to also log to the domain logfile * src/qemu/qemu_driver.c: Pass -1 for logFD to taint methods to auto-append to logfile * src/qemu/qemu_process.c: Pass open logFD at startup for taint methods --- src/qemu/qemu_domain.c | 44 ++++++++++++++++++++++++++++++++------------ src/qemu/qemu_domain.h | 12 ++++++++---- src/qemu/qemu_driver.c | 6 +++--- src/qemu/qemu_process.c | 2 +- 4 files changed, 44 insertions(+), 20 deletions(-)
@@ -909,7 +929,7 @@ int qemuDomainAppendLog(struct qemud_driver *driver, virReportOOMError(); goto cleanup; } - if (safewrite(logFD, message, strlen(message)) < 0) { + if (safewrite(fd, message, strlen(message)) < 0) {
Oops. Let's squash this hunk into 4, where it belongs. ACK, modulo the reshuffling of one hunk. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Daniel P. Berrange -
Eric Blake