If the first console is just a copy of the first serial device we don't need to iterate over the same device twice in order to perform actions like security labeling, cgroup configuring, etc. Currently only security SELinux manager was aware of this fact. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/conf/domain_conf.c | 26 +++++++++++++++++++++----- src/security/security_selinux.c | 10 ---------- 2 files changed, 21 insertions(+), 15 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0ff216e3a3..aa441fae3c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3517,6 +3517,24 @@ void virDomainDeviceInfoClear(virDomainDeviceInfoPtr info) } +static bool +virDomainSkipBackcompatConsole(virDomainDefPtr def, + size_t index, + bool all) +{ + virDomainChrDefPtr console = def->consoles[index]; + + if (!all && index == 0 && + (console->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL || + console->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_NONE) && + def->os.type == VIR_DOMAIN_OSTYPE_HVM) { + return true; + } + + return false; +} + + static int virDomainDeviceInfoIterateInternal(virDomainDefPtr def, virDomainDeviceInfoCallback cb, @@ -3585,11 +3603,7 @@ virDomainDeviceInfoIterateInternal(virDomainDefPtr def, return -1; } for (i = 0; i < def->nconsoles; i++) { - if (!all && - i == 0 && - (def->consoles[i]->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL || - def->consoles[i]->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_NONE) && - def->os.type == VIR_DOMAIN_OSTYPE_HVM) + if (virDomainSkipBackcompatConsole(def, i, all)) continue; device.data.chr = def->consoles[i]; if (cb(def, &device, &def->consoles[i]->info, opaque) < 0) @@ -25313,6 +25327,8 @@ virDomainChrDefForeach(virDomainDefPtr def, goto done; } for (i = 0; i < def->nconsoles; i++) { + if (virDomainSkipBackcompatConsole(def, i, false)) + continue; if ((iter)(def, def->consoles[i], opaque) < 0) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index df7c96833e..612dbc2a83 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -2339,11 +2339,6 @@ virSecuritySELinuxRestoreSecurityChardevCallback(virDomainDefPtr def, { virSecurityManagerPtr mgr = opaque; - /* This is taken care of by processing of def->serials */ - if (dev->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE && - dev->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL) - return 0; - return virSecuritySELinuxRestoreChardevLabel(mgr, def, dev, dev->source); } @@ -2733,11 +2728,6 @@ virSecuritySELinuxSetSecurityChardevCallback(virDomainDefPtr def, { virSecurityManagerPtr mgr = opaque; - /* This is taken care of by processing of def->serials */ - if (dev->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE && - dev->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL) - return 0; - return virSecuritySELinuxSetChardevLabel(mgr, def, dev, dev->source); } -- 2.13.0

If libvirt uses virtlogd instead of passing the file path directly to QEMU we shouldn't relabel the chardev source file, otherwise virtlogd will get a permission denied while reloading. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=143098 Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/conf/domain_conf.c | 20 ++++++++++++++++++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 12 ++++++++---- src/security/security_dac.c | 6 ++++++ src/security/security_selinux.c | 6 ++++++ 5 files changed, 41 insertions(+), 4 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index aa441fae3c..92f011d3a4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2064,6 +2064,7 @@ virDomainChrSourceDefCopy(virDomainChrSourceDefPtr dest, } dest->type = src->type; + dest->skipRelabel = src->skipRelabel; return 0; } @@ -10608,6 +10609,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, char *append = NULL; char *haveTLS = NULL; char *tlsFromConfig = NULL; + char *skipRelabel = NULL; int remaining = 0; while (cur != NULL) { @@ -10628,6 +10630,8 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, case VIR_DOMAIN_CHR_TYPE_UNIX: if (!append && def->type == VIR_DOMAIN_CHR_TYPE_FILE) append = virXMLPropString(cur, "append"); + if (!skipRelabel && def->type == VIR_DOMAIN_CHR_TYPE_FILE) + skipRelabel = virXMLPropString(cur, "skipRelabel"); /* PTY path is only parsed from live xml. */ if (!path && (def->type != VIR_DOMAIN_CHR_TYPE_PTY || @@ -10726,6 +10730,17 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, _("Invalid append attribute value '%s'"), append); goto error; } + if (skipRelabel && def->type == VIR_DOMAIN_CHR_TYPE_FILE && + (flags & VIR_DOMAIN_DEF_PARSE_STATUS)) { + if (STREQ(skipRelabel, "yes")) { + def->skipRelabel = true; + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("invalid 'skipRelabel' attribute value '%s'"), + skipRelabel); + goto error; + } + } if (!path && def->type != VIR_DOMAIN_CHR_TYPE_PTY) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -10902,6 +10917,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, VIR_FREE(logfile); VIR_FREE(haveTLS); VIR_FREE(tlsFromConfig); + VIR_FREE(skipRelabel); return remaining; @@ -22324,6 +22340,10 @@ virDomainChrSourceDefFormat(virBufferPtr buf, def->data.file.append != VIR_TRISTATE_SWITCH_ABSENT) virBufferAsprintf(buf, " append='%s'", virTristateSwitchTypeToString(def->data.file.append)); + if ((flags & VIR_DOMAIN_DEF_FORMAT_STATUS) && + def->type == VIR_DOMAIN_CHR_TYPE_FILE && def->skipRelabel) { + virBufferAddLit(buf, " skipRelabel='yes'"); + } virDomainSourceDefFormatSeclabel(buf, nseclabels, seclabels, flags); } break; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 09fb7aada4..329eb90392 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1166,6 +1166,7 @@ struct _virDomainChrSourceDef { } data; char *logfile; int logappend; + bool skipRelabel; }; /* A complete character device, both host and domain views. */ diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 813a8515c0..0625075bb2 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4998,6 +4998,7 @@ static int qemuBuildChrChardevFileStr(virLogManagerPtr logManager, virCommandPtr cmd, const virDomainDef *def, + virDomainChrSourceDefPtr sourceDef, virBufferPtr buf, const char *filearg, const char *fileval, const char *appendarg, int appendval) @@ -5011,6 +5012,9 @@ qemuBuildChrChardevFileStr(virLogManagerPtr logManager, appendval == VIR_TRISTATE_SWITCH_OFF) flags |= VIR_LOG_MANAGER_PROTOCOL_DOMAIN_OPEN_LOG_FILE_TRUNCATE; + if (sourceDef) + sourceDef->skipRelabel = true; + if ((logfd = virLogManagerDomainOpenLogFile(logManager, "qemu", def->uuid, @@ -5051,7 +5055,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, virCommandPtr cmd, virQEMUDriverConfigPtr cfg, const virDomainDef *def, - const virDomainChrSourceDef *dev, + virDomainChrSourceDefPtr dev, const char *alias, virQEMUCapsPtr qemuCaps, bool nowait) @@ -5093,7 +5097,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, goto cleanup; } if (qemuBuildChrChardevFileStr(virQEMUCapsGet(qemuCaps, QEMU_CAPS_CHARDEV_FILE_APPEND) ? - logManager : NULL, cmd, def, &buf, + logManager : NULL, cmd, def, dev, &buf, "path", dev->data.file.path, "append", dev->data.file.append) < 0) goto cleanup; @@ -5209,7 +5213,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, _("logfile not supported in this QEMU binary")); goto cleanup; } - if (qemuBuildChrChardevFileStr(logManager, cmd, def, &buf, + if (qemuBuildChrChardevFileStr(logManager, cmd, def, NULL, &buf, "logfile", dev->logfile, "logappend", dev->logappend) < 0) goto cleanup; @@ -5573,7 +5577,7 @@ qemuBuildMonitorCommandLine(virLogManagerPtr logManager, virQEMUDriverConfigPtr cfg, virDomainDefPtr def, virQEMUCapsPtr qemuCaps, - const virDomainChrSourceDef *monitor_chr, + virDomainChrSourceDefPtr monitor_chr, bool monitor_json) { char *chrdev; diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 922e484942..a4e02ca8bc 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1196,6 +1196,9 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, if (chr_seclabel && !chr_seclabel->relabel) return 0; + if (!chr_seclabel && dev_source->skipRelabel) + return 0; + if (chr_seclabel && chr_seclabel->label) { if (virParseOwnershipIds(chr_seclabel->label, &user, &group) < 0) return -1; @@ -1276,6 +1279,9 @@ virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr, if (chr_seclabel && !chr_seclabel->relabel) return 0; + if (!chr_seclabel && dev_source->skipRelabel) + return 0; + switch ((virDomainChrType) dev_source->type) { case VIR_DOMAIN_CHR_TYPE_DEV: case VIR_DOMAIN_CHR_TYPE_FILE: diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 612dbc2a83..64ab2795d5 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -2216,6 +2216,9 @@ virSecuritySELinuxSetChardevLabel(virSecurityManagerPtr mgr, if (chr_seclabel && !chr_seclabel->relabel) return 0; + if (!chr_seclabel && dev_source->skipRelabel) + return 0; + if (chr_seclabel) imagelabel = chr_seclabel->label; if (!imagelabel) @@ -2289,6 +2292,9 @@ virSecuritySELinuxRestoreChardevLabel(virSecurityManagerPtr mgr, if (chr_seclabel && !chr_seclabel->relabel) return 0; + if (!chr_seclabel && dev_source->skipRelabel) + return 0; + switch (dev_source->type) { case VIR_DOMAIN_CHR_TYPE_DEV: case VIR_DOMAIN_CHR_TYPE_FILE: -- 2.13.0

What sense would it make in this case? There is only one option when we're running virtlogd and that is no label. If they supply relabel='yes', it won't work with virtlogd unless they change it back to 'no'. So, you've got an option the functionality of which depends on a service running, which is by default on by the way. Since I don't want to be someone who just criticizes other people's ideas without adding anything to the discussion, I was thinking about re-using the qemu config parameter stdioLogD. But that solution is also wrong as it turned out. I haven't come up with anything better yet, though. So, having something in the status XML seems like a plausible approach to me. Erik

On Tue, May 23, 2017 at 04:25:01PM +0200, Martin Kletzander wrote: The use of virtlogd affects many devices in guests. So we should just record at the top level of the QEMU domain status, whether virtlogd was used or not. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On Tue, May 23, 2017 at 05:53:46PM +0200, Pavel Hrdina wrote: Having some devices use virtlogd and some devices not use virtlogd is just a recipe for maximising confusion of developers, users, and support people alike. We should record whether we used virtlogd when we first start the guest, and honour that until QEMU is killed. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On Wed, May 24, 2017 at 12:25:51PM +0200, Martin Kletzander wrote: Now it makes sense what you've meant by private structure, I was confused because we were talking about XML :). The related crasher is not fix to my knowledge and probably no-one is working on it. Pavel