On 02/23/2016 11:41 AM, Daniel P. Berrange wrote:
Extend the chardev source XML so that there is a new optional <log/> element, which is applicable to all character device backend types. For example, to log output of a TCP backed serial port
<serial type='tcp'> <source mode='connect' host='127.0.0.1' service='9999'/> <protocol type='raw'/> <log file='/var/log/libvirt/qemu/demo-serial0.log' append='on'/> <target port='0'/> </serial>
Not all hypervisors will support use of logfiles.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- docs/schemas/domaincommon.rng | 12 ++++++++++++ src/conf/domain_conf.c | 28 ++++++++++++++++++++++++++++ src/conf/domain_conf.h | 2 ++ 3 files changed, 42 insertions(+)
Probably should be described in formatdomain.html.in too, right?
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 67af93a..b49d9eb 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3276,6 +3276,18 @@ </optional> </element> </optional> + <optional> + <element name="log"> + <attribute name="file"> + <ref name="absFilePath"/>
Although rng requires absFilePath, it's not checked during parse. I think what separates this from others is that it's going to be used for write purposes. I see that virDomainSnapshotDefParse which seemingly can accept an file path as input does check if (def->file && def->file[0] == '/') to ensure absolute file path. I have a secondary concern about where the writing can occur...
+ </attribute> + <optional> + <attribute name="append"> + <ref name="virOnOff"/> + </attribute> + </optional> + </element> + </optional> </define> <!-- The description for a console diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 56bd1aa..92c3ce9 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1768,6 +1768,8 @@ virDomainChrSourceDefClear(virDomainChrSourceDefPtr def) VIR_FREE(def->data.spiceport.channel); break; } + + VIR_FREE(def->logfile); }
/* Deep copies the contents of src into dest. Return -1 and report @@ -9396,6 +9398,8 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, char *connectHost = NULL; char *connectService = NULL; char *path = NULL; + char *logfile = NULL; + char *logappend = NULL; char *mode = NULL; char *protocol = NULL; char *channel = NULL; @@ -9483,6 +9487,11 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, } ctxt->node = saved_node; } + } else if (xmlStrEqual(cur->name, BAD_CAST "log")) { + if (!logfile) + logfile = virXMLPropString(cur, "file");
Shouldn't we validate the logfile path at least? Even virLogParseOutputs validates what's in logfile is at least an absFilePath. I looked forward to the next patch and see it's just supplied to qemu. Unlike a number of the absDirPath elements I looked at in domain_conf, this is a "write" path and I would think we want to be ultra conservative... Secondary concern - not that it couldn't be done today w/ log_outputs file name in the libvirtd.conf file... What if someone supplied a name they shouldn't be supplying? That is somewhere they shouldn't be writing... Is there a way to "demand" that the value is to a specific path prefix? or maybe we should just dictate providing a "name" and it's up to the hypervisor to prepend the hypervisor specific path (IOW: the /var/log/libvirt/qemu). That way someone could mount whatever they want at /var/log/libvirt/qemu in order to store inordinate amount of logs (not clogging /var unnecessarily).
+ if (!logappend) + logappend = virXMLPropString(cur, "append"); } else if (xmlStrEqual(cur->name, BAD_CAST "protocol")) { if (!protocol) protocol = virXMLPropString(cur, "type"); @@ -9641,6 +9650,16 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, break; }
+ def->logfile = logfile; + logfile = NULL; + + if (logappend != NULL && + (def->logappend = virTristateSwitchTypeFromString(logappend)) <= 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid append attribute value '%s'"), logappend); + goto error; + } + cleanup: VIR_FREE(mode); VIR_FREE(protocol);
Need VIR_FREE(logfile) and VIR_FREE(logappend) John
@@ -20093,6 +20112,15 @@ virDomainChrSourceDefFormat(virBufferPtr buf,
}
+ if (def->logfile) { + virBufferEscapeString(buf, "<log file='%s'", def->logfile); + if (def->logappend != VIR_TRISTATE_SWITCH_ABSENT) { + virBufferAsprintf(buf, " append='%s'", + virTristateSwitchTypeToString(def->logappend)); + } + virBufferAddLit(buf, "/>\n"); + } + return 0; }
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 1de3be3..2b2f75b 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1204,6 +1204,8 @@ struct _virDomainChrSourceDef { char *channel; } spiceport; } data; + char *logfile; + int logappend; };
/* A complete character device, both host and domain views. */