10:41 a.m.
This series of patches does two things
* Converts the type=file chardev over to use virtlogd
(assuming use of virtlogd is enabled in qemu.conf)
* Adds a <log file="..."/> element to all chardev
sources, allowing data to be captured to a logfile
Both of these are important features wanted by openstack.
The first thing fixes a long standing security issue that
a guest OS can trivially exhuast host disk space by outputing
lots of data to its serial port.
The second thing allows OpenStack to record boot up
messages for a guest's serial console, while still
allowing interactive serial console ussage. ie the
serial port will be configured with type=tcp, and
the new <log> element used to record the data.
Both these thing required changes in QEMU, which have
been merged for the forthcoming QEMU 2.6 release.
Daniel P. Berrange (5):
logging: allow inode/offset params to be NULL
conf: allow use of a logfile with chardev backends
qemu: add support for logging chardev output to a file
qemu: use virtlogd for character device log files
qemu: support use of virtlogd with file based chardevs
docs/schemas/domaincommon.rng | 12 +
src/conf/domain_conf.c | 28 ++
src/conf/domain_conf.h | 2 +
src/logging/log_daemon_dispatch.c | 3 +-
src/logging/log_handler.c | 6 +-
src/logging/log_handler.h | 2 +-
src/logging/log_manager.c | 6 +-
src/logging/log_manager.h | 2 +
src/logging/log_protocol.x | 4 +
src/qemu/qemu_capabilities.c | 2 +
src/qemu/qemu_capabilities.h | 1 +
src/qemu/qemu_command.c | 295 ++++++++++++++-------
src/qemu/qemu_command.h | 7 +-
src/qemu/qemu_domain.c | 6 +
src/qemu/qemu_domain.h | 3 +
src/qemu/qemu_driver.c | 2 +-
src/qemu/qemu_process.c | 4 +-
.../qemuxml2argv-serial-file-log.args | 25 ++
.../qemuxml2argv-serial-file-log.xml | 39 +++
tests/qemuxml2argvtest.c | 4 +-
20 files changed, 350 insertions(+), 103 deletions(-)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-serial-file-log.args
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-serial-file-log.xml
--
2.5.0
10:41 a.m.
New subject: [libvirt] [PATCH 1/5] logging: allow inode/offset params to be NULL
Not all callers of virLogManagerDomainOpenLogFile will
care about getting the current inode/offset, so we should
allow those parameters to be NULL
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
src/logging/log_manager.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/logging/log_manager.c b/src/logging/log_manager.c
index f5b4b7d..7c37ba1 100644
--- a/src/logging/log_manager.c
+++ b/src/logging/log_manager.c
@@ -196,8 +196,10 @@ virLogManagerDomainOpenLogFile(virLogManagerPtr mgr,
goto cleanup;
}
- *inode = ret.pos.inode;
- *offset = ret.pos.offset;
+ if (inode)
+ *inode = ret.pos.inode;
+ if (offset)
+ *offset = ret.pos.offset;
rv = fdout[0];
cleanup:
--
2.5.0
9:33 a.m.
New subject: [libvirt] [PATCH 1/5] logging: allow inode/offset params to be NULL
On 02/23/2016 11:41 AM, Daniel P. Berrange wrote:
Not all callers of virLogManagerDomainOpenLogFile will
care about getting the current inode/offset, so we should
allow those parameters to be NULL
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
src/logging/log_manager.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
In retrospect, probably could have used ATTRIBUTE_NONNULL on these args
previously...
ACK
John
10:41 a.m.
New subject: [libvirt] [PATCH 2/5] conf: allow use of a logfile with chardev backends
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(a)redhat.com>
---
docs/schemas/domaincommon.rng | 12 ++++++++++++
src/conf/domain_conf.c | 28 ++++++++++++++++++++++++++++
src/conf/domain_conf.h | 2 ++
3 files changed, 42 insertions(+)
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"/>
+ </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");
+ 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);
@@ -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. */
--
2.5.0
9:33 a.m.
New subject: [libvirt] [PATCH 2/5] conf: allow use of a logfile with chardev backends
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(a)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. */
10:56 a.m.
New subject: [libvirt] [PATCH 2/5] conf: allow use of a logfile with chardev backends
On Fri, Feb 26, 2016 at 10:33:10AM -0500, John Ferlan wrote:
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(a)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?
Yes, it should be.
> 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...
> @@ -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).
The ability to define XML is considered to be equivalent to having
a root shell. We don't try to enforce rules for any of the paths
in the XML whether disk filenames, log file names or character
device backends. So this isn't causing a problem didn't already
have.
> @@ -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)
Ah yes.
Regards,
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 :|
10:41 a.m.
New subject: [libvirt] [PATCH 3/5] qemu: add support for logging chardev output to a file
Honour the <log file='...'/> element in chardevs to output
data to a file. This requires QEMU >= 2.6
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
src/qemu/qemu_capabilities.c | 2 ++
src/qemu/qemu_capabilities.h | 1 +
src/qemu/qemu_command.c | 19 +++++++++++
.../qemuxml2argv-serial-file-log.args | 25 ++++++++++++++
.../qemuxml2argv-serial-file-log.xml | 39 ++++++++++++++++++++++
tests/qemuxml2argvtest.c | 2 ++
6 files changed, 88 insertions(+)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-serial-file-log.args
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-serial-file-log.xml
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 2b953ea..c8f41a3 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -315,6 +315,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
"vserport-change-event", /* 210 */
"virtio-balloon-pci.deflate-on-oom",
"mptsas1068",
+ "chardev-logfile",
);
@@ -2627,6 +2628,7 @@ static struct virQEMUCapsCommandLineProps virQEMUCapsCommandLine[] =
{
{ "machine", "aes-key-wrap", QEMU_CAPS_AES_KEY_WRAP },
{ "machine", "dea-key-wrap", QEMU_CAPS_DEA_KEY_WRAP },
{ "chardev", "append", QEMU_CAPS_CHARDEV_FILE_APPEND },
+ { "chardev", "logfile", QEMU_CAPS_CHARDEV_LOGFILE },
};
static int
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index b73c529..ea2e723 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -344,6 +344,7 @@ typedef enum {
QEMU_CAPS_VIRTIO_BALLOON_AUTODEFLATE, /* virtio-balloon-{device,pci,ccw}.
* deflate-on-oom */
QEMU_CAPS_SCSI_MPTSAS1068, /* -device mptsas1068 */
+ QEMU_CAPS_CHARDEV_LOGFILE, /* -chardev logfile=xxxx */
QEMU_CAPS_LAST /* this must always be the last item */
} virQEMUCapsFlags;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 78423e7..dee7208 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3967,6 +3967,19 @@ qemuBuildChrChardevStr(const virDomainChrSourceDef *dev,
goto error;
}
+ if (dev->logfile) {
+ if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_CHARDEV_LOGFILE)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("logfile not supported in this QEMU binary"));
+ goto error;
+ }
+ virBufferAsprintf(&buf, ",logfile=%s", dev->logfile);
+ if (dev->logappend != VIR_TRISTATE_SWITCH_ABSENT) {
+ virBufferAsprintf(&buf, ",logappend=%s",
+ virTristateSwitchTypeToString(dev->logappend));
+ }
+ }
+
if (virBufferCheckError(&buf) < 0)
goto error;
@@ -3984,6 +3997,12 @@ qemuBuildChrArgStr(const virDomainChrSourceDef *dev,
{
virBuffer buf = VIR_BUFFER_INITIALIZER;
+ if (dev->logfile) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("logfile not supported in this QEMU binary"));
+ goto error;
+ }
+
if (prefix)
virBufferAdd(&buf, prefix, strlen(prefix));
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-serial-file-log.args
b/tests/qemuxml2argvdata/qemuxml2argv-serial-file-log.args
new file mode 100644
index 0000000..de313e5
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-serial-file-log.args
@@ -0,0 +1,25 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu \
+-name QEMUGuest1 \
+-S \
+-M pc \
+-m 214 \
+-smp 1 \
+-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
+-nographic \
+-nodefaults \
+-chardev socket,id=charmonitor,path=/tmp/test-monitor,server,nowait \
+-mon chardev=charmonitor,id=monitor,mode=readline \
+-no-acpi \
+-boot c \
+-usb \
+-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
+-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \
+-chardev file,id=charserial0,path=/tmp/serial.log,\
+logfile=/var/lib/libvirt/qemu/demo-serial.log,logappend=off \
+-device isa-serial,chardev=charserial0,id=serial0
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-serial-file-log.xml
b/tests/qemuxml2argvdata/qemuxml2argv-serial-file-log.xml
new file mode 100644
index 0000000..adb3b78
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-serial-file-log.xml
@@ -0,0 +1,39 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219136</memory>
+ <currentMemory unit='KiB'>219136</currentMemory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='i686' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu</emulator>
+ <disk type='block' device='disk'>
+ <driver name='qemu' type='raw'/>
+ <source dev='/dev/HostVG/QEMUGuest1'/>
+ <target dev='hda' bus='ide'/>
+ <address type='drive' controller='0' bus='0'
target='0' unit='0'/>
+ </disk>
+ <controller type='usb' index='0'/>
+ <controller type='ide' index='0'/>
+ <controller type='pci' index='0' model='pci-root'/>
+ <serial type='file'>
+ <source path='/tmp/serial.log'/>
+ <log file='/var/lib/libvirt/qemu/demo-serial.log'
append='off'/>
+ <target port='0'/>
+ </serial>
+ <console type='file'>
+ <source path='/tmp/serial.log'/>
+ <target type='serial' port='0'/>
+ </console>
+ <input type='mouse' bus='ps2'/>
+ <input type='keyboard' bus='ps2'/>
+ <memballoon model='none'/>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 854623d..624639e 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1026,6 +1026,8 @@ mymain(void)
DO_TEST("serial-pty", NONE);
DO_TEST("serial-dev", NONE);
DO_TEST("serial-file", NONE);
+ DO_TEST("serial-file-log", QEMU_CAPS_CHARDEV,
QEMU_CAPS_CHARDEV_FILE_APPEND,
+ QEMU_CAPS_CHARDEV_LOGFILE);
DO_TEST("serial-unix", NONE);
DO_TEST("serial-tcp", NONE);
DO_TEST("serial-udp", NONE);
--
2.5.0
9:33 a.m.
New subject: [libvirt] [PATCH 3/5] qemu: add support for logging chardev output to a file
On 02/23/2016 11:41 AM, Daniel P. Berrange wrote:
Honour the <log file='...'/> element in chardevs to
output
data to a file. This requires QEMU >= 2.6
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
src/qemu/qemu_capabilities.c | 2 ++
src/qemu/qemu_capabilities.h | 1 +
src/qemu/qemu_command.c | 19 +++++++++++
.../qemuxml2argv-serial-file-log.args | 25 ++++++++++++++
.../qemuxml2argv-serial-file-log.xml | 39 ++++++++++++++++++++++
tests/qemuxml2argvtest.c | 2 ++
6 files changed, 88 insertions(+)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-serial-file-log.args
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-serial-file-log.xml
Seems reasonable - although as a heads up you could have conflicts with
Pavel's vram64 series...
John
diff --git a/src/qemu/qemu_capabilities.c
b/src/qemu/qemu_capabilities.c
index 2b953ea..c8f41a3 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -315,6 +315,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
"vserport-change-event", /* 210 */
"virtio-balloon-pci.deflate-on-oom",
"mptsas1068",
+ "chardev-logfile",
);
@@ -2627,6 +2628,7 @@ static struct virQEMUCapsCommandLineProps virQEMUCapsCommandLine[]
= {
{ "machine", "aes-key-wrap", QEMU_CAPS_AES_KEY_WRAP },
{ "machine", "dea-key-wrap", QEMU_CAPS_DEA_KEY_WRAP },
{ "chardev", "append", QEMU_CAPS_CHARDEV_FILE_APPEND },
+ { "chardev", "logfile", QEMU_CAPS_CHARDEV_LOGFILE },
};
static int
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index b73c529..ea2e723 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -344,6 +344,7 @@ typedef enum {
QEMU_CAPS_VIRTIO_BALLOON_AUTODEFLATE, /* virtio-balloon-{device,pci,ccw}.
* deflate-on-oom */
QEMU_CAPS_SCSI_MPTSAS1068, /* -device mptsas1068 */
+ QEMU_CAPS_CHARDEV_LOGFILE, /* -chardev logfile=xxxx */
QEMU_CAPS_LAST /* this must always be the last item */
} virQEMUCapsFlags;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 78423e7..dee7208 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3967,6 +3967,19 @@ qemuBuildChrChardevStr(const virDomainChrSourceDef *dev,
goto error;
}
+ if (dev->logfile) {
+ if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_CHARDEV_LOGFILE)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("logfile not supported in this QEMU binary"));
+ goto error;
+ }
+ virBufferAsprintf(&buf, ",logfile=%s", dev->logfile);
+ if (dev->logappend != VIR_TRISTATE_SWITCH_ABSENT) {
+ virBufferAsprintf(&buf, ",logappend=%s",
+ virTristateSwitchTypeToString(dev->logappend));
+ }
+ }
+
if (virBufferCheckError(&buf) < 0)
goto error;
@@ -3984,6 +3997,12 @@ qemuBuildChrArgStr(const virDomainChrSourceDef *dev,
{
virBuffer buf = VIR_BUFFER_INITIALIZER;
+ if (dev->logfile) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("logfile not supported in this QEMU binary"));
+ goto error;
+ }
+
if (prefix)
virBufferAdd(&buf, prefix, strlen(prefix));
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-serial-file-log.args
b/tests/qemuxml2argvdata/qemuxml2argv-serial-file-log.args
new file mode 100644
index 0000000..de313e5
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-serial-file-log.args
@@ -0,0 +1,25 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu \
+-name QEMUGuest1 \
+-S \
+-M pc \
+-m 214 \
+-smp 1 \
+-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
+-nographic \
+-nodefaults \
+-chardev socket,id=charmonitor,path=/tmp/test-monitor,server,nowait \
+-mon chardev=charmonitor,id=monitor,mode=readline \
+-no-acpi \
+-boot c \
+-usb \
+-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
+-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \
+-chardev file,id=charserial0,path=/tmp/serial.log,\
+logfile=/var/lib/libvirt/qemu/demo-serial.log,logappend=off \
+-device isa-serial,chardev=charserial0,id=serial0
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-serial-file-log.xml
b/tests/qemuxml2argvdata/qemuxml2argv-serial-file-log.xml
new file mode 100644
index 0000000..adb3b78
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-serial-file-log.xml
@@ -0,0 +1,39 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219136</memory>
+ <currentMemory unit='KiB'>219136</currentMemory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='i686' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu</emulator>
+ <disk type='block' device='disk'>
+ <driver name='qemu' type='raw'/>
+ <source dev='/dev/HostVG/QEMUGuest1'/>
+ <target dev='hda' bus='ide'/>
+ <address type='drive' controller='0' bus='0'
target='0' unit='0'/>
+ </disk>
+ <controller type='usb' index='0'/>
+ <controller type='ide' index='0'/>
+ <controller type='pci' index='0' model='pci-root'/>
+ <serial type='file'>
+ <source path='/tmp/serial.log'/>
+ <log file='/var/lib/libvirt/qemu/demo-serial.log'
append='off'/>
+ <target port='0'/>
+ </serial>
+ <console type='file'>
+ <source path='/tmp/serial.log'/>
+ <target type='serial' port='0'/>
+ </console>
+ <input type='mouse' bus='ps2'/>
+ <input type='keyboard' bus='ps2'/>
+ <memballoon model='none'/>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 854623d..624639e 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1026,6 +1026,8 @@ mymain(void)
DO_TEST("serial-pty", NONE);
DO_TEST("serial-dev", NONE);
DO_TEST("serial-file", NONE);
+ DO_TEST("serial-file-log", QEMU_CAPS_CHARDEV,
QEMU_CAPS_CHARDEV_FILE_APPEND,
+ QEMU_CAPS_CHARDEV_LOGFILE);
DO_TEST("serial-unix", NONE);
DO_TEST("serial-tcp", NONE);
DO_TEST("serial-udp", NONE);
10:41 a.m.
New subject: [libvirt] [PATCH 4/5] qemu: use virtlogd for character device log files
If use of virtlogd is enabled, then use it for backing the
character device log files too.
---
src/logging/log_daemon_dispatch.c | 3 +-
src/logging/log_handler.c | 6 +-
src/logging/log_handler.h | 2 +-
src/logging/log_manager.h | 2 +
src/logging/log_protocol.x | 4 +
src/qemu/qemu_command.c | 234 ++++++++++++++++++++++++--------------
src/qemu/qemu_command.h | 7 +-
src/qemu/qemu_domain.c | 6 +
src/qemu/qemu_domain.h | 3 +
src/qemu/qemu_driver.c | 2 +-
src/qemu/qemu_process.c | 4 +-
tests/qemuxml2argvtest.c | 2 +-
12 files changed, 179 insertions(+), 96 deletions(-)
diff --git a/src/logging/log_daemon_dispatch.c b/src/logging/log_daemon_dispatch.c
index a5fa7f0..b00cee2 100644
--- a/src/logging/log_daemon_dispatch.c
+++ b/src/logging/log_daemon_dispatch.c
@@ -50,13 +50,14 @@ virLogManagerProtocolDispatchDomainOpenLogFile(virNetServerPtr server
ATTRIBUTE_
int rv = -1;
off_t offset;
ino_t inode;
+ bool trunc = args->flags &
VIR_LOG_MANAGER_PROTOCOL_DOMAIN_OPEN_LOG_FILE_TRUNCATE;
if ((fd = virLogHandlerDomainOpenLogFile(virLogDaemonGetHandler(logDaemon),
args->driver,
(unsigned char *)args->dom.uuid,
args->dom.name,
args->path,
- args->flags,
+ trunc,
&inode, &offset)) < 0)
goto cleanup;
diff --git a/src/logging/log_handler.c b/src/logging/log_handler.c
index 92cff50..4c08223 100644
--- a/src/logging/log_handler.c
+++ b/src/logging/log_handler.c
@@ -357,7 +357,7 @@ virLogHandlerDomainOpenLogFile(virLogHandlerPtr handler,
const unsigned char *domuuid,
const char *domname,
const char *path,
- unsigned int flags,
+ bool trunc,
ino_t *inode,
off_t *offset)
{
@@ -365,8 +365,6 @@ virLogHandlerDomainOpenLogFile(virLogHandlerPtr handler,
virLogHandlerLogFilePtr file = NULL;
int pipefd[2] = { -1, -1 };
- virCheckFlags(0, -1);
-
virObjectLock(handler);
handler->inhibitor(true, handler->opaque);
@@ -400,7 +398,7 @@ virLogHandlerDomainOpenLogFile(virLogHandlerPtr handler,
if ((file->file = virRotatingFileWriterNew(path,
DEFAULT_FILE_SIZE,
DEFAULT_MAX_BACKUP,
- false,
+ trunc,
DEFAULT_MODE)) == NULL)
goto error;
diff --git a/src/logging/log_handler.h b/src/logging/log_handler.h
index e61f32d..54a9cd9 100644
--- a/src/logging/log_handler.h
+++ b/src/logging/log_handler.h
@@ -48,7 +48,7 @@ int virLogHandlerDomainOpenLogFile(virLogHandlerPtr handler,
const unsigned char *domuuid,
const char *domname,
const char *path,
- unsigned int flags,
+ bool trunc,
ino_t *inode,
off_t *offset);
diff --git a/src/logging/log_manager.h b/src/logging/log_manager.h
index d3b9d29..7deaba7 100644
--- a/src/logging/log_manager.h
+++ b/src/logging/log_manager.h
@@ -26,6 +26,8 @@
# include "internal.h"
+# include "logging/log_protocol.h"
+
typedef struct _virLogManager virLogManager;
typedef virLogManager *virLogManagerPtr;
diff --git a/src/logging/log_protocol.x b/src/logging/log_protocol.x
index b0ac31b..0363c75 100644
--- a/src/logging/log_protocol.x
+++ b/src/logging/log_protocol.x
@@ -30,6 +30,10 @@ struct virLogManagerProtocolLogFilePosition {
};
typedef struct virLogManagerProtocolLogFilePosition
virLogManagerProtocolLogFilePosition;
+enum virLogManagerProtocolDomainOpenLogFileFlags {
+ VIR_LOG_MANAGER_PROTOCOL_DOMAIN_OPEN_LOG_FILE_TRUNCATE = 1
+};
+
/* Obtain a file handle suitable for writing to a
* log file for a domain
*/
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index dee7208..8378470 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -60,6 +60,7 @@
#if defined(__linux__)
# include <linux/capability.h>
#endif
+#include "logging/log_manager.h"
#include <sys/stat.h>
#include <fcntl.h>
@@ -149,6 +150,59 @@ VIR_ENUM_IMPL(qemuNumaPolicy, VIR_DOMAIN_NUMATUNE_MEM_LAST,
"preferred",
"interleave");
+/**
+ * qemuVirCommandGetFDSet:
+ * @cmd: the command to modify
+ * @fd: fd to reassign to the child
+ *
+ * Get the parameters for the QEMU -add-fd command line option
+ * for the given file descriptor. The file descriptor must previously
+ * have been 'transferred' in a virCommandPassFD() call.
+ * This function for example returns "set=10,fd=20".
+ */
+static char *
+qemuVirCommandGetFDSet(virCommandPtr cmd, int fd)
+{
+ char *result = NULL;
+ int idx = virCommandPassFDGetFDIndex(cmd, fd);
+
+ if (idx >= 0) {
+ ignore_value(virAsprintf(&result, "set=%d,fd=%d", idx, fd));
+ } else {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("file descriptor %d has not been transferred"), fd);
+ }
+
+ return result;
+}
+
+
+/**
+ * qemuVirCommandGetDevSet:
+ * @cmd: the command to modify
+ * @fd: fd to reassign to the child
+ *
+ * Get the parameters for the QEMU path= parameter where a file
+ * descriptor is accessed via a file descriptor set, for example
+ * /dev/fdset/10. The file descriptor must previously have been
+ * 'transferred' in a virCommandPassFD() call.
+ */
+static char *
+qemuVirCommandGetDevSet(virCommandPtr cmd, int fd)
+{
+ char *result = NULL;
+ int idx = virCommandPassFDGetFDIndex(cmd, fd);
+
+ if (idx >= 0) {
+ ignore_value(virAsprintf(&result, "/dev/fdset/%d", idx));
+ } else {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("file descriptor %d has not been transferred"), fd);
+ }
+ return result;
+}
+
+
static int
qemuBuildObjectCommandLinePropsInternal(const char *key,
const virJSONValue *value,
@@ -3850,7 +3904,10 @@ qemuBuildSCSIHostdevDevStr(virDomainDefPtr def,
/* This function outputs a -chardev command line option which describes only the
* host side of the character device */
static char *
-qemuBuildChrChardevStr(const virDomainChrSourceDef *dev,
+qemuBuildChrChardevStr(virLogManagerPtr logManager,
+ virCommandPtr cmd,
+ virDomainDefPtr def,
+ const virDomainChrSourceDef *dev,
const char *alias,
virQEMUCapsPtr qemuCaps)
{
@@ -3973,10 +4030,42 @@ qemuBuildChrChardevStr(const virDomainChrSourceDef *dev,
_("logfile not supported in this QEMU binary"));
goto error;
}
- virBufferAsprintf(&buf, ",logfile=%s", dev->logfile);
- if (dev->logappend != VIR_TRISTATE_SWITCH_ABSENT) {
- virBufferAsprintf(&buf, ",logappend=%s",
- virTristateSwitchTypeToString(dev->logappend));
+ if (logManager) {
+ char *fdset, *fdpath;
+ int flags = 0;
+ int logfd;
+
+ if (dev->logappend == VIR_TRISTATE_SWITCH_OFF)
+ flags |= VIR_LOG_MANAGER_PROTOCOL_DOMAIN_OPEN_LOG_FILE_TRUNCATE;
+
+ if ((logfd = virLogManagerDomainOpenLogFile(logManager,
+ "qemu",
+ def->uuid,
+ def->name,
+ dev->logfile,
+ flags,
+ NULL, NULL)) < 0)
+ goto error;
+
+ virCommandPassFD(cmd, logfd, VIR_COMMAND_PASS_FD_CLOSE_PARENT);
+ if (!(fdset = qemuVirCommandGetFDSet(cmd, logfd)))
+ goto error;
+
+ virCommandAddArg(cmd, "-add-fd");
+ virCommandAddArg(cmd, fdset);
+ VIR_FREE(fdset);
+
+ if (!(fdpath = qemuVirCommandGetDevSet(cmd, logfd)))
+ goto error;
+
+ virBufferAsprintf(&buf, ",logfile=%s,logappend=on", fdpath);
+ VIR_FREE(fdpath);
+ } else {
+ virBufferAsprintf(&buf, ",logfile=%s", dev->logfile);
+ if (dev->logappend != VIR_TRISTATE_SWITCH_ABSENT) {
+ virBufferAsprintf(&buf, ",logappend=%s",
+ virTristateSwitchTypeToString(dev->logappend));
+ }
}
}
@@ -4093,7 +4182,9 @@ qemuBuildChrArgStr(const virDomainChrSourceDef *dev,
static int
-qemuBuildMonitorCommandLine(virCommandPtr cmd,
+qemuBuildMonitorCommandLine(virLogManagerPtr logManager,
+ virCommandPtr cmd,
+ virDomainDefPtr def,
virQEMUCapsPtr qemuCaps,
const virDomainChrSourceDef *monitor_chr,
bool monitor_json)
@@ -4106,10 +4197,11 @@ qemuBuildMonitorCommandLine(virCommandPtr cmd,
/* Use -chardev if it's available */
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_CHARDEV)) {
- virCommandAddArg(cmd, "-chardev");
- if (!(chrdev = qemuBuildChrChardevStr(monitor_chr, "monitor",
+ if (!(chrdev = qemuBuildChrChardevStr(logManager, cmd, def,
+ monitor_chr, "monitor",
qemuCaps)))
return -1;
+ virCommandAddArg(cmd, "-chardev");
virCommandAddArg(cmd, chrdev);
VIR_FREE(chrdev);
@@ -4248,7 +4340,10 @@ qemuBuildSclpDevStr(virDomainChrDefPtr dev)
static int
-qemuBuildRNGBackendChrdevStr(virDomainRNGDefPtr rng,
+qemuBuildRNGBackendChrdevStr(virLogManagerPtr logManager,
+ virCommandPtr cmd,
+ virDomainDefPtr def,
+ virDomainRNGDefPtr rng,
virQEMUCapsPtr qemuCaps,
char **chr)
{
@@ -4261,7 +4356,8 @@ qemuBuildRNGBackendChrdevStr(virDomainRNGDefPtr rng,
return 0;
case VIR_DOMAIN_RNG_BACKEND_EGD:
- if (!(*chr = qemuBuildChrChardevStr(rng->source.chardev,
+ if (!(*chr = qemuBuildChrChardevStr(logManager, cmd, def,
+ rng->source.chardev,
rng->info.alias, qemuCaps)))
return -1;
}
@@ -6567,7 +6663,10 @@ qemuBuildShmemDevStr(virDomainDefPtr def,
}
char *
-qemuBuildShmemBackendStr(virDomainShmemDefPtr shmem,
+qemuBuildShmemBackendStr(virLogManagerPtr logManager,
+ virCommandPtr cmd,
+ virDomainDefPtr def,
+ virDomainShmemDefPtr shmem,
virQEMUCapsPtr qemuCaps)
{
char *devstr = NULL;
@@ -6578,13 +6677,16 @@ qemuBuildShmemBackendStr(virDomainShmemDefPtr shmem,
shmem->name) < 0)
return NULL;
- devstr = qemuBuildChrChardevStr(&shmem->server.chr, shmem->info.alias,
qemuCaps);
+ devstr = qemuBuildChrChardevStr(logManager, cmd, def,
+ &shmem->server.chr,
+ shmem->info.alias, qemuCaps);
return devstr;
}
static int
-qemuBuildShmemCommandLine(virCommandPtr cmd,
+qemuBuildShmemCommandLine(virLogManagerPtr logManager,
+ virCommandPtr cmd,
virDomainDefPtr def,
virDomainShmemDefPtr shmem,
virQEMUCapsPtr qemuCaps)
@@ -6597,7 +6699,8 @@ qemuBuildShmemCommandLine(virCommandPtr cmd,
VIR_FREE(devstr);
if (shmem->server.enabled) {
- if (!(devstr = qemuBuildShmemBackendStr(shmem, qemuCaps)))
+ if (!(devstr = qemuBuildShmemBackendStr(logManager, cmd, def,
+ shmem, qemuCaps)))
return -1;
virCommandAddArgList(cmd, "-chardev", devstr, NULL);
@@ -6724,59 +6827,6 @@ qemuBuildTPMDevStr(const virDomainDef *def,
}
-/**
- * qemuVirCommandGetFDSet:
- * @cmd: the command to modify
- * @fd: fd to reassign to the child
- *
- * Get the parameters for the QEMU -add-fd command line option
- * for the given file descriptor. The file descriptor must previously
- * have been 'transferred' in a virCommandPassFD() call.
- * This function for example returns "set=10,fd=20".
- */
-static char *
-qemuVirCommandGetFDSet(virCommandPtr cmd, int fd)
-{
- char *result = NULL;
- int idx = virCommandPassFDGetFDIndex(cmd, fd);
-
- if (idx >= 0) {
- ignore_value(virAsprintf(&result, "set=%d,fd=%d", idx, fd));
- } else {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("file descriptor %d has not been transferred"), fd);
- }
-
- return result;
-}
-
-
-/**
- * qemuVirCommandGetDevSet:
- * @cmd: the command to modify
- * @fd: fd to reassign to the child
- *
- * Get the parameters for the QEMU path= parameter where a file
- * descriptor is accessed via a file descriptor set, for example
- * /dev/fdset/10. The file descriptor must previously have been
- * 'transferred' in a virCommandPassFD() call.
- */
-static char *
-qemuVirCommandGetDevSet(virCommandPtr cmd, int fd)
-{
- char *result = NULL;
- int idx = virCommandPassFDGetFDIndex(cmd, fd);
-
- if (idx >= 0) {
- ignore_value(virAsprintf(&result, "/dev/fdset/%d", idx));
- } else {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("file descriptor %d has not been transferred"), fd);
- }
- return result;
-}
-
-
static char *
qemuBuildTPMBackendStr(const virDomainDef *def,
virCommandPtr cmd,
@@ -7020,6 +7070,7 @@ qemuBuildCommandLineCallbacks buildCommandLineCallbacks = {
virCommandPtr
qemuBuildCommandLine(virConnectPtr conn,
virQEMUDriverPtr driver,
+ virLogManagerPtr logManager,
virDomainDefPtr def,
virDomainChrSourceDefPtr monitor_chr,
bool monitor_json,
@@ -7170,7 +7221,8 @@ qemuBuildCommandLine(virConnectPtr conn,
if (qemuBuildSgaCommandLine(cmd, def, qemuCaps) < 0)
goto error;
- if (qemuBuildMonitorCommandLine(cmd, qemuCaps, monitor_chr,
+ if (qemuBuildMonitorCommandLine(logManager, cmd, def,
+ qemuCaps, monitor_chr,
monitor_json) < 0)
goto error;
@@ -7932,13 +7984,14 @@ qemuBuildCommandLine(virConnectPtr conn,
goto error;
}
- virCommandAddArg(cmd, "-chardev");
- if (!(devstr = qemuBuildChrChardevStr(&smartcard->data.passthru,
+ if (!(devstr = qemuBuildChrChardevStr(logManager, cmd, def,
+ &smartcard->data.passthru,
smartcard->info.alias,
qemuCaps))) {
virBufferFreeAndReset(&opt);
goto error;
}
+ virCommandAddArg(cmd, "-chardev");
virCommandAddArg(cmd, devstr);
VIR_FREE(devstr);
@@ -7976,11 +8029,12 @@ qemuBuildCommandLine(virConnectPtr conn,
/* Use -chardev with -device if they are available */
if (virQEMUCapsSupportsChardev(def, qemuCaps, serial)) {
- virCommandAddArg(cmd, "-chardev");
- if (!(devstr = qemuBuildChrChardevStr(&serial->source,
+ if (!(devstr = qemuBuildChrChardevStr(logManager, cmd, def,
+ &serial->source,
serial->info.alias,
qemuCaps)))
goto error;
+ virCommandAddArg(cmd, "-chardev");
virCommandAddArg(cmd, devstr);
VIR_FREE(devstr);
@@ -8012,11 +8066,12 @@ qemuBuildCommandLine(virConnectPtr conn,
/* Use -chardev with -device if they are available */
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_CHARDEV) &&
virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
- virCommandAddArg(cmd, "-chardev");
- if (!(devstr = qemuBuildChrChardevStr(¶llel->source,
+ if (!(devstr = qemuBuildChrChardevStr(logManager, cmd, def,
+ ¶llel->source,
parallel->info.alias,
qemuCaps)))
goto error;
+ virCommandAddArg(cmd, "-chardev");
virCommandAddArg(cmd, devstr);
VIR_FREE(devstr);
@@ -8045,11 +8100,12 @@ qemuBuildCommandLine(virConnectPtr conn,
goto error;
}
- virCommandAddArg(cmd, "-chardev");
- if (!(devstr = qemuBuildChrChardevStr(&channel->source,
+ if (!(devstr = qemuBuildChrChardevStr(logManager, cmd, def,
+ &channel->source,
channel->info.alias,
qemuCaps)))
goto error;
+ virCommandAddArg(cmd, "-chardev");
virCommandAddArg(cmd, devstr);
VIR_FREE(devstr);
@@ -8090,11 +8146,12 @@ qemuBuildCommandLine(virConnectPtr conn,
* the newer -chardev interface. */
;
} else {
- virCommandAddArg(cmd, "-chardev");
- if (!(devstr = qemuBuildChrChardevStr(&channel->source,
+ if (!(devstr = qemuBuildChrChardevStr(logManager, cmd, def,
+ &channel->source,
channel->info.alias,
qemuCaps)))
goto error;
+ virCommandAddArg(cmd, "-chardev");
virCommandAddArg(cmd, devstr);
VIR_FREE(devstr);
}
@@ -8124,11 +8181,12 @@ qemuBuildCommandLine(virConnectPtr conn,
goto error;
}
- virCommandAddArg(cmd, "-chardev");
- if (!(devstr = qemuBuildChrChardevStr(&console->source,
+ if (!(devstr = qemuBuildChrChardevStr(logManager, cmd, def,
+ &console->source,
console->info.alias,
qemuCaps)))
goto error;
+ virCommandAddArg(cmd, "-chardev");
virCommandAddArg(cmd, devstr);
VIR_FREE(devstr);
@@ -8143,11 +8201,12 @@ qemuBuildCommandLine(virConnectPtr conn,
goto error;
}
- virCommandAddArg(cmd, "-chardev");
- if (!(devstr = qemuBuildChrChardevStr(&console->source,
+ if (!(devstr = qemuBuildChrChardevStr(logManager, cmd, def,
+ &console->source,
console->info.alias,
qemuCaps)))
goto error;
+ virCommandAddArg(cmd, "-chardev");
virCommandAddArg(cmd, devstr);
VIR_FREE(devstr);
@@ -8487,13 +8546,14 @@ qemuBuildCommandLine(virConnectPtr conn,
virDomainRedirdevDefPtr redirdev = def->redirdevs[i];
char *devstr;
- virCommandAddArg(cmd, "-chardev");
- if (!(devstr = qemuBuildChrChardevStr(&redirdev->source.chr,
+ if (!(devstr = qemuBuildChrChardevStr(logManager, cmd, def,
+ &redirdev->source.chr,
redirdev->info.alias,
qemuCaps))) {
goto error;
}
+ virCommandAddArg(cmd, "-chardev");
virCommandAddArg(cmd, devstr);
VIR_FREE(devstr);
@@ -8713,7 +8773,8 @@ qemuBuildCommandLine(virConnectPtr conn,
}
/* possibly add character device for backend */
- if (qemuBuildRNGBackendChrdevStr(rng, qemuCaps, &tmp) < 0)
+ if (qemuBuildRNGBackendChrdevStr(logManager, cmd, def,
+ rng, qemuCaps, &tmp) < 0)
goto error;
if (tmp) {
@@ -8858,7 +8919,8 @@ qemuBuildCommandLine(virConnectPtr conn,
}
for (i = 0; i < def->nshmems; i++) {
- if (qemuBuildShmemCommandLine(cmd, def, def->shmems[i], qemuCaps))
+ if (qemuBuildShmemCommandLine(logManager, cmd,
+ def, def->shmems[i], qemuCaps))
goto error;
}
diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
index fb684d0..decf463 100644
--- a/src/qemu/qemu_command.h
+++ b/src/qemu/qemu_command.h
@@ -32,6 +32,7 @@
# include "qemu_domain.h"
# include "qemu_domain_address.h"
# include "qemu_capabilities.h"
+# include "logging/log_manager.h"
/* Config type for XML import/export conversions */
# define QEMU_CONFIG_FORMAT_ARGV "qemu-argv"
@@ -59,6 +60,7 @@ char *qemuBuildObjectCommandlineFromJSON(const char *type,
virCommandPtr qemuBuildCommandLine(virConnectPtr conn,
virQEMUDriverPtr driver,
+ virLogManagerPtr logManager,
virDomainDefPtr def,
virDomainChrSourceDefPtr monitor_chr,
bool monitor_json,
@@ -180,7 +182,10 @@ int qemuBuildRNGBackendProps(virDomainRNGDefPtr rng,
char *qemuBuildShmemDevStr(virDomainDefPtr def,
virDomainShmemDefPtr shmem,
virQEMUCapsPtr qemuCaps);
-char *qemuBuildShmemBackendStr(virDomainShmemDefPtr shmem,
+char *qemuBuildShmemBackendStr(virLogManagerPtr logManager,
+ virCommandPtr cmd,
+ virDomainDefPtr def,
+ virDomainShmemDefPtr shmem,
virQEMUCapsPtr qemuCaps);
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index c56f9f1..df422e2 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2603,6 +2603,12 @@ void qemuDomainLogContextRef(qemuDomainLogContextPtr ctxt)
}
+virLogManagerPtr qemuDomainLogContextGetManager(qemuDomainLogContextPtr ctxt)
+{
+ return ctxt->manager;
+}
+
+
void qemuDomainLogContextFree(qemuDomainLogContextPtr ctxt)
{
bool lastRef;
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 8359b1a..f6ce19c 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -35,6 +35,7 @@
# include "qemu_capabilities.h"
# include "virchrdev.h"
# include "virobject.h"
+# include "logging/log_manager.h"
# define QEMU_DOMAIN_FORMAT_LIVE_FLAGS \
(VIR_DOMAIN_XML_SECURE | \
@@ -377,6 +378,8 @@ void qemuDomainLogContextMarkPosition(qemuDomainLogContextPtr ctxt);
void qemuDomainLogContextRef(qemuDomainLogContextPtr ctxt);
void qemuDomainLogContextFree(qemuDomainLogContextPtr ctxt);
+virLogManagerPtr qemuDomainLogContextGetManager(qemuDomainLogContextPtr ctxt);
+
const char *qemuFindQemuImgBinary(virQEMUDriverPtr driver);
int qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm,
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 45ff3c0..1afafd6 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7159,7 +7159,7 @@ static char *qemuConnectDomainXMLToNative(virConnectPtr conn,
}
}
- if (!(cmd = qemuBuildCommandLine(conn, driver, def,
+ if (!(cmd = qemuBuildCommandLine(conn, driver, NULL, def,
&monConfig, monitor_json, qemuCaps,
NULL, NULL,
VIR_NETDEV_VPORT_PROFILE_OP_NO_OP,
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index c5b9f4e..3780d0c 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -4987,7 +4987,9 @@ qemuProcessLaunch(virConnectPtr conn,
}
VIR_DEBUG("Building emulator command line");
- if (!(cmd = qemuBuildCommandLine(conn, driver, vm->def, priv->monConfig,
+ if (!(cmd = qemuBuildCommandLine(conn, driver,
+ qemuDomainLogContextGetManager(logCtxt),
+ vm->def, priv->monConfig,
priv->monJSON, priv->qemuCaps,
incoming ? incoming->launchURI : NULL,
snapshot, vmop,
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 624639e..b5323b4 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -345,7 +345,7 @@ static int testCompareXMLToArgvFiles(const char *xml,
testFailed = true;
if (!testFailed &&
- !(cmd = qemuBuildCommandLine(conn, &driver, vmdef, &monitor_chr,
+ !(cmd = qemuBuildCommandLine(conn, &driver, NULL, vmdef, &monitor_chr,
(flags & FLAG_JSON), extraFlags,
migrateURI, NULL,
VIR_NETDEV_VPORT_PROFILE_OP_NO_OP,
--
2.5.0
9:33 a.m.
New subject: [libvirt] [PATCH 4/5] qemu: use virtlogd for character device log files
On 02/23/2016 11:41 AM, Daniel P. Berrange wrote:
If use of virtlogd is enabled, then use it for backing the
character device log files too.
---
src/logging/log_daemon_dispatch.c | 3 +-
src/logging/log_handler.c | 6 +-
src/logging/log_handler.h | 2 +-
src/logging/log_manager.h | 2 +
src/logging/log_protocol.x | 4 +
src/qemu/qemu_command.c | 234 ++++++++++++++++++++++++--------------
<sigh> One of us will have conflicts depending upon whether my final
pile of qemu_command.c cleanup changes gets reviewed...
src/qemu/qemu_command.h | 7 +-
src/qemu/qemu_domain.c | 6 +
src/qemu/qemu_domain.h | 3 +
src/qemu/qemu_driver.c | 2 +-
src/qemu/qemu_process.c | 4 +-
tests/qemuxml2argvtest.c | 2 +-
12 files changed, 179 insertions(+), 96 deletions(-)
diff --git a/src/logging/log_daemon_dispatch.c b/src/logging/log_daemon_dispatch.c
index a5fa7f0..b00cee2 100644
--- a/src/logging/log_daemon_dispatch.c
+++ b/src/logging/log_daemon_dispatch.c
@@ -50,13 +50,14 @@ virLogManagerProtocolDispatchDomainOpenLogFile(virNetServerPtr server
ATTRIBUTE_
int rv = -1;
off_t offset;
ino_t inode;
+ bool trunc = args->flags &
VIR_LOG_MANAGER_PROTOCOL_DOMAIN_OPEN_LOG_FILE_TRUNCATE;
So by changing this means you don't envision other flags in the future
for virLogHandlerDomainOpenLogFile?
IOW: Why isn't this done in the next layer down? Seems we'd be changing
what we're doing (not that anything else is using this yet).
Should perhaps this part be a separate patch?
if ((fd =
virLogHandlerDomainOpenLogFile(virLogDaemonGetHandler(logDaemon),
args->driver,
(unsigned char *)args->dom.uuid,
args->dom.name,
args->path,
- args->flags,
+ trunc,
&inode, &offset)) < 0)
goto cleanup;
diff --git a/src/logging/log_handler.c b/src/logging/log_handler.c
index 92cff50..4c08223 100644
--- a/src/logging/log_handler.c
+++ b/src/logging/log_handler.c
@@ -357,7 +357,7 @@ virLogHandlerDomainOpenLogFile(virLogHandlerPtr handler,
const unsigned char *domuuid,
const char *domname,
const char *path,
- unsigned int flags,
+ bool trunc,
ino_t *inode,
off_t *offset)
{
@@ -365,8 +365,6 @@ virLogHandlerDomainOpenLogFile(virLogHandlerPtr handler,
virLogHandlerLogFilePtr file = NULL;
int pipefd[2] = { -1, -1 };
- virCheckFlags(0, -1);
-
virObjectLock(handler);
handler->inhibitor(true, handler->opaque);
@@ -400,7 +398,7 @@ virLogHandlerDomainOpenLogFile(virLogHandlerPtr handler,
if ((file->file = virRotatingFileWriterNew(path,
DEFAULT_FILE_SIZE,
DEFAULT_MAX_BACKUP,
- false,
+ trunc,
DEFAULT_MODE)) == NULL)
goto error;
diff --git a/src/logging/log_handler.h b/src/logging/log_handler.h
index e61f32d..54a9cd9 100644
--- a/src/logging/log_handler.h
+++ b/src/logging/log_handler.h
@@ -48,7 +48,7 @@ int virLogHandlerDomainOpenLogFile(virLogHandlerPtr handler,
const unsigned char *domuuid,
const char *domname,
const char *path,
- unsigned int flags,
+ bool trunc,
ino_t *inode,
off_t *offset);
diff --git a/src/logging/log_manager.h b/src/logging/log_manager.h
index d3b9d29..7deaba7 100644
--- a/src/logging/log_manager.h
+++ b/src/logging/log_manager.h
@@ -26,6 +26,8 @@
# include "internal.h"
+# include "logging/log_protocol.h"
+
typedef struct _virLogManager virLogManager;
typedef virLogManager *virLogManagerPtr;
diff --git a/src/logging/log_protocol.x b/src/logging/log_protocol.x
index b0ac31b..0363c75 100644
--- a/src/logging/log_protocol.x
+++ b/src/logging/log_protocol.x
@@ -30,6 +30,10 @@ struct virLogManagerProtocolLogFilePosition {
};
typedef struct virLogManagerProtocolLogFilePosition
virLogManagerProtocolLogFilePosition;
+enum virLogManagerProtocolDomainOpenLogFileFlags {
+ VIR_LOG_MANAGER_PROTOCOL_DOMAIN_OPEN_LOG_FILE_TRUNCATE = 1
+};
+
DomainOpenLogFile doesn't have flags now - it just has trunc - so
nothing could be added here (at least w/r/t how I understand it).
/* Obtain a file handle suitable for writing to a
* log file for a domain
*/
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index dee7208..8378470 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -60,6 +60,7 @@
#if defined(__linux__)
# include <linux/capability.h>
#endif
+#include "logging/log_manager.h"
#include <sys/stat.h>
#include <fcntl.h>
@@ -149,6 +150,59 @@ VIR_ENUM_IMPL(qemuNumaPolicy, VIR_DOMAIN_NUMATUNE_MEM_LAST,
"preferred",
"interleave");
+/**
+ * qemuVirCommandGetFDSet:
+ * @cmd: the command to modify
+ * @fd: fd to reassign to the child
+ *
+ * Get the parameters for the QEMU -add-fd command line option
+ * for the given file descriptor. The file descriptor must previously
+ * have been 'transferred' in a virCommandPassFD() call.
+ * This function for example returns "set=10,fd=20".
+ */
+static char *
+qemuVirCommandGetFDSet(virCommandPtr cmd, int fd)
+{
+ char *result = NULL;
+ int idx = virCommandPassFDGetFDIndex(cmd, fd);
+
+ if (idx >= 0) {
+ ignore_value(virAsprintf(&result, "set=%d,fd=%d", idx, fd));
+ } else {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("file descriptor %d has not been transferred"), fd);
+ }
+
+ return result;
+}
+
+
+/**
+ * qemuVirCommandGetDevSet:
+ * @cmd: the command to modify
+ * @fd: fd to reassign to the child
+ *
+ * Get the parameters for the QEMU path= parameter where a file
+ * descriptor is accessed via a file descriptor set, for example
+ * /dev/fdset/10. The file descriptor must previously have been
+ * 'transferred' in a virCommandPassFD() call.
+ */
+static char *
+qemuVirCommandGetDevSet(virCommandPtr cmd, int fd)
+{
+ char *result = NULL;
+ int idx = virCommandPassFDGetFDIndex(cmd, fd);
+
+ if (idx >= 0) {
+ ignore_value(virAsprintf(&result, "/dev/fdset/%d", idx));
+ } else {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("file descriptor %d has not been transferred"), fd);
+ }
+ return result;
+}
+
+
[1] Code motion... Should it be separate?
static int
qemuBuildObjectCommandLinePropsInternal(const char *key,
const virJSONValue *value,
@@ -3850,7 +3904,10 @@ qemuBuildSCSIHostdevDevStr(virDomainDefPtr def,
/* This function outputs a -chardev command line option which describes only the
* host side of the character device */
static char *
-qemuBuildChrChardevStr(const virDomainChrSourceDef *dev,
+qemuBuildChrChardevStr(virLogManagerPtr logManager,
+ virCommandPtr cmd,
+ virDomainDefPtr def,
+ const virDomainChrSourceDef *dev,
const char *alias,
virQEMUCapsPtr qemuCaps)
{
@@ -3973,10 +4030,42 @@ qemuBuildChrChardevStr(const virDomainChrSourceDef *dev,
_("logfile not supported in this QEMU binary"));
goto error;
}
- virBufferAsprintf(&buf, ",logfile=%s", dev->logfile);
- if (dev->logappend != VIR_TRISTATE_SWITCH_ABSENT) {
- virBufferAsprintf(&buf, ",logappend=%s",
- virTristateSwitchTypeToString(dev->logappend));
+ if (logManager) {
+ char *fdset, *fdpath;
+ int flags = 0;
+ int logfd;
+
+ if (dev->logappend == VIR_TRISTATE_SWITCH_OFF)
+ flags |= VIR_LOG_MANAGER_PROTOCOL_DOMAIN_OPEN_LOG_FILE_TRUNCATE;
+
+ if ((logfd = virLogManagerDomainOpenLogFile(logManager,
+ "qemu",
+ def->uuid,
+ def->name,
+ dev->logfile,
+ flags,
+ NULL, NULL)) < 0)
+ goto error;
+
+ virCommandPassFD(cmd, logfd, VIR_COMMAND_PASS_FD_CLOSE_PARENT);
+ if (!(fdset = qemuVirCommandGetFDSet(cmd, logfd)))
Would this error path need to close logfd or is that done because of the
above PassFD path?
+ goto error;
+
+ virCommandAddArg(cmd, "-add-fd");
+ virCommandAddArg(cmd, fdset);
+ VIR_FREE(fdset);
+
+ if (!(fdpath = qemuVirCommandGetDevSet(cmd, logfd)))
Similar question about logfd...
+ goto error;
+
+ virBufferAsprintf(&buf, ",logfile=%s,logappend=on", fdpath);
Always on? Doesn't matter what's been provided in logappend in the XML?
+ VIR_FREE(fdpath);
+ } else {
+ virBufferAsprintf(&buf, ",logfile=%s", dev->logfile);
+ if (dev->logappend != VIR_TRISTATE_SWITCH_ABSENT) {
+ virBufferAsprintf(&buf, ",logappend=%s",
+ virTristateSwitchTypeToString(dev->logappend));
+ }
Could be a separate call rather than inlining...
}
}
@@ -4093,7 +4182,9 @@ qemuBuildChrArgStr(const virDomainChrSourceDef *dev,
static int
-qemuBuildMonitorCommandLine(virCommandPtr cmd,
+qemuBuildMonitorCommandLine(virLogManagerPtr logManager,
+ virCommandPtr cmd,
+ virDomainDefPtr def,
virQEMUCapsPtr qemuCaps,
const virDomainChrSourceDef *monitor_chr,
bool monitor_json)
@@ -4106,10 +4197,11 @@ qemuBuildMonitorCommandLine(virCommandPtr cmd,
/* Use -chardev if it's available */
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_CHARDEV)) {
- virCommandAddArg(cmd, "-chardev");
- if (!(chrdev = qemuBuildChrChardevStr(monitor_chr, "monitor",
+ if (!(chrdev = qemuBuildChrChardevStr(logManager, cmd, def,
+ monitor_chr, "monitor",
qemuCaps)))
return -1;
+ virCommandAddArg(cmd, "-chardev");
doh! Although I suppose one could make the argument that all the
additions of "-chardev" could be their own patch...
virCommandAddArg(cmd, chrdev);
VIR_FREE(chrdev);
@@ -4248,7 +4340,10 @@ qemuBuildSclpDevStr(virDomainChrDefPtr dev)
static int
-qemuBuildRNGBackendChrdevStr(virDomainRNGDefPtr rng,
+qemuBuildRNGBackendChrdevStr(virLogManagerPtr logManager,
+ virCommandPtr cmd,
+ virDomainDefPtr def,
+ virDomainRNGDefPtr rng,
virQEMUCapsPtr qemuCaps,
char **chr)
{
@@ -4261,7 +4356,8 @@ qemuBuildRNGBackendChrdevStr(virDomainRNGDefPtr rng,
return 0;
case VIR_DOMAIN_RNG_BACKEND_EGD:
- if (!(*chr = qemuBuildChrChardevStr(rng->source.chardev,
+ if (!(*chr = qemuBuildChrChardevStr(logManager, cmd, def,
+ rng->source.chardev,
rng->info.alias, qemuCaps)))
return -1;
}
@@ -6567,7 +6663,10 @@ qemuBuildShmemDevStr(virDomainDefPtr def,
}
char *
-qemuBuildShmemBackendStr(virDomainShmemDefPtr shmem,
+qemuBuildShmemBackendStr(virLogManagerPtr logManager,
+ virCommandPtr cmd,
+ virDomainDefPtr def,
+ virDomainShmemDefPtr shmem,
virQEMUCapsPtr qemuCaps)
{
char *devstr = NULL;
@@ -6578,13 +6677,16 @@ qemuBuildShmemBackendStr(virDomainShmemDefPtr shmem,
shmem->name) < 0)
return NULL;
- devstr = qemuBuildChrChardevStr(&shmem->server.chr, shmem->info.alias,
qemuCaps);
+ devstr = qemuBuildChrChardevStr(logManager, cmd, def,
+ &shmem->server.chr,
+ shmem->info.alias, qemuCaps);
return devstr;
}
static int
-qemuBuildShmemCommandLine(virCommandPtr cmd,
+qemuBuildShmemCommandLine(virLogManagerPtr logManager,
+ virCommandPtr cmd,
virDomainDefPtr def,
virDomainShmemDefPtr shmem,
virQEMUCapsPtr qemuCaps)
@@ -6597,7 +6699,8 @@ qemuBuildShmemCommandLine(virCommandPtr cmd,
VIR_FREE(devstr);
if (shmem->server.enabled) {
- if (!(devstr = qemuBuildShmemBackendStr(shmem, qemuCaps)))
+ if (!(devstr = qemuBuildShmemBackendStr(logManager, cmd, def,
+ shmem, qemuCaps)))
return -1;
virCommandAddArgList(cmd, "-chardev", devstr, NULL);
@@ -6724,59 +6827,6 @@ qemuBuildTPMDevStr(const virDomainDef *def,
}
-/**
- * qemuVirCommandGetFDSet:
- * @cmd: the command to modify
- * @fd: fd to reassign to the child
- *
- * Get the parameters for the QEMU -add-fd command line option
- * for the given file descriptor. The file descriptor must previously
- * have been 'transferred' in a virCommandPassFD() call.
- * This function for example returns "set=10,fd=20".
- */
-static char *
-qemuVirCommandGetFDSet(virCommandPtr cmd, int fd)
-{
- char *result = NULL;
- int idx = virCommandPassFDGetFDIndex(cmd, fd);
-
- if (idx >= 0) {
- ignore_value(virAsprintf(&result, "set=%d,fd=%d", idx, fd));
- } else {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("file descriptor %d has not been transferred"), fd);
- }
-
- return result;
-}
-
-
-/**
- * qemuVirCommandGetDevSet:
- * @cmd: the command to modify
- * @fd: fd to reassign to the child
- *
- * Get the parameters for the QEMU path= parameter where a file
- * descriptor is accessed via a file descriptor set, for example
- * /dev/fdset/10. The file descriptor must previously have been
- * 'transferred' in a virCommandPassFD() call.
- */
-static char *
-qemuVirCommandGetDevSet(virCommandPtr cmd, int fd)
-{
- char *result = NULL;
- int idx = virCommandPassFDGetFDIndex(cmd, fd);
-
- if (idx >= 0) {
- ignore_value(virAsprintf(&result, "/dev/fdset/%d", idx));
- } else {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("file descriptor %d has not been transferred"), fd);
- }
- return result;
-}
-
-
[1] Code motion...
static char *
qemuBuildTPMBackendStr(const virDomainDef *def,
virCommandPtr cmd,
@@ -7020,6 +7070,7 @@ qemuBuildCommandLineCallbacks buildCommandLineCallbacks = {
virCommandPtr
qemuBuildCommandLine(virConnectPtr conn,
virQEMUDriverPtr driver,
+ virLogManagerPtr logManager,
virDomainDefPtr def,
virDomainChrSourceDefPtr monitor_chr,
bool monitor_json,
@@ -7170,7 +7221,8 @@ qemuBuildCommandLine(virConnectPtr conn,
if (qemuBuildSgaCommandLine(cmd, def, qemuCaps) < 0)
goto error;
- if (qemuBuildMonitorCommandLine(cmd, qemuCaps, monitor_chr,
+ if (qemuBuildMonitorCommandLine(logManager, cmd, def,
+ qemuCaps, monitor_chr,
monitor_json) < 0)
goto error;
@@ -7932,13 +7984,14 @@ qemuBuildCommandLine(virConnectPtr conn,
goto error;
}
- virCommandAddArg(cmd, "-chardev");
- if (!(devstr = qemuBuildChrChardevStr(&smartcard->data.passthru,
+ if (!(devstr = qemuBuildChrChardevStr(logManager, cmd, def,
+ &smartcard->data.passthru,
smartcard->info.alias,
qemuCaps))) {
virBufferFreeAndReset(&opt);
goto error;
}
+ virCommandAddArg(cmd, "-chardev");
virCommandAddArg(cmd, devstr);
VIR_FREE(devstr);
@@ -7976,11 +8029,12 @@ qemuBuildCommandLine(virConnectPtr conn,
/* Use -chardev with -device if they are available */
if (virQEMUCapsSupportsChardev(def, qemuCaps, serial)) {
- virCommandAddArg(cmd, "-chardev");
- if (!(devstr = qemuBuildChrChardevStr(&serial->source,
+ if (!(devstr = qemuBuildChrChardevStr(logManager, cmd, def,
+ &serial->source,
serial->info.alias,
qemuCaps)))
goto error;
+ virCommandAddArg(cmd, "-chardev");
virCommandAddArg(cmd, devstr);
VIR_FREE(devstr);
@@ -8012,11 +8066,12 @@ qemuBuildCommandLine(virConnectPtr conn,
/* Use -chardev with -device if they are available */
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_CHARDEV) &&
virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
- virCommandAddArg(cmd, "-chardev");
- if (!(devstr = qemuBuildChrChardevStr(¶llel->source,
+ if (!(devstr = qemuBuildChrChardevStr(logManager, cmd, def,
+ ¶llel->source,
parallel->info.alias,
qemuCaps)))
goto error;
+ virCommandAddArg(cmd, "-chardev");
virCommandAddArg(cmd, devstr);
VIR_FREE(devstr);
@@ -8045,11 +8100,12 @@ qemuBuildCommandLine(virConnectPtr conn,
goto error;
}
- virCommandAddArg(cmd, "-chardev");
- if (!(devstr = qemuBuildChrChardevStr(&channel->source,
+ if (!(devstr = qemuBuildChrChardevStr(logManager, cmd, def,
+ &channel->source,
channel->info.alias,
qemuCaps)))
goto error;
+ virCommandAddArg(cmd, "-chardev");
virCommandAddArg(cmd, devstr);
VIR_FREE(devstr);
@@ -8090,11 +8146,12 @@ qemuBuildCommandLine(virConnectPtr conn,
* the newer -chardev interface. */
;
} else {
- virCommandAddArg(cmd, "-chardev");
- if (!(devstr = qemuBuildChrChardevStr(&channel->source,
+ if (!(devstr = qemuBuildChrChardevStr(logManager, cmd, def,
+ &channel->source,
channel->info.alias,
qemuCaps)))
goto error;
+ virCommandAddArg(cmd, "-chardev");
virCommandAddArg(cmd, devstr);
VIR_FREE(devstr);
}
@@ -8124,11 +8181,12 @@ qemuBuildCommandLine(virConnectPtr conn,
goto error;
}
- virCommandAddArg(cmd, "-chardev");
- if (!(devstr = qemuBuildChrChardevStr(&console->source,
+ if (!(devstr = qemuBuildChrChardevStr(logManager, cmd, def,
+ &console->source,
console->info.alias,
qemuCaps)))
goto error;
+ virCommandAddArg(cmd, "-chardev");
virCommandAddArg(cmd, devstr);
VIR_FREE(devstr);
@@ -8143,11 +8201,12 @@ qemuBuildCommandLine(virConnectPtr conn,
goto error;
}
- virCommandAddArg(cmd, "-chardev");
- if (!(devstr = qemuBuildChrChardevStr(&console->source,
+ if (!(devstr = qemuBuildChrChardevStr(logManager, cmd, def,
+ &console->source,
console->info.alias,
qemuCaps)))
goto error;
+ virCommandAddArg(cmd, "-chardev");
virCommandAddArg(cmd, devstr);
VIR_FREE(devstr);
@@ -8487,13 +8546,14 @@ qemuBuildCommandLine(virConnectPtr conn,
virDomainRedirdevDefPtr redirdev = def->redirdevs[i];
char *devstr;
- virCommandAddArg(cmd, "-chardev");
- if (!(devstr = qemuBuildChrChardevStr(&redirdev->source.chr,
+ if (!(devstr = qemuBuildChrChardevStr(logManager, cmd, def,
+ &redirdev->source.chr,
redirdev->info.alias,
qemuCaps))) {
goto error;
}
+ virCommandAddArg(cmd, "-chardev");
virCommandAddArg(cmd, devstr);
VIR_FREE(devstr);
@@ -8713,7 +8773,8 @@ qemuBuildCommandLine(virConnectPtr conn,
}
/* possibly add character device for backend */
- if (qemuBuildRNGBackendChrdevStr(rng, qemuCaps, &tmp) < 0)
+ if (qemuBuildRNGBackendChrdevStr(logManager, cmd, def,
+ rng, qemuCaps, &tmp) < 0)
goto error;
if (tmp) {
@@ -8858,7 +8919,8 @@ qemuBuildCommandLine(virConnectPtr conn,
}
for (i = 0; i < def->nshmems; i++) {
- if (qemuBuildShmemCommandLine(cmd, def, def->shmems[i], qemuCaps))
+ if (qemuBuildShmemCommandLine(logManager, cmd,
+ def, def->shmems[i], qemuCaps))
goto error;
}
diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
index fb684d0..decf463 100644
--- a/src/qemu/qemu_command.h
+++ b/src/qemu/qemu_command.h
@@ -32,6 +32,7 @@
# include "qemu_domain.h"
# include "qemu_domain_address.h"
# include "qemu_capabilities.h"
+# include "logging/log_manager.h"
/* Config type for XML import/export conversions */
# define QEMU_CONFIG_FORMAT_ARGV "qemu-argv"
@@ -59,6 +60,7 @@ char *qemuBuildObjectCommandlineFromJSON(const char *type,
virCommandPtr qemuBuildCommandLine(virConnectPtr conn,
virQEMUDriverPtr driver,
+ virLogManagerPtr logManager,
virDomainDefPtr def,
virDomainChrSourceDefPtr monitor_chr,
bool monitor_json,
There's a "ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(10);" on this API,
that 10 needs to change to 11.
Also with Martin's "qemu: Shorten per-domain directory names" you could
have to modify 16 & 17 too
John
@@ -180,7 +182,10 @@ int qemuBuildRNGBackendProps(virDomainRNGDefPtr
rng,
char *qemuBuildShmemDevStr(virDomainDefPtr def,
virDomainShmemDefPtr shmem,
virQEMUCapsPtr qemuCaps);
-char *qemuBuildShmemBackendStr(virDomainShmemDefPtr shmem,
+char *qemuBuildShmemBackendStr(virLogManagerPtr logManager,
+ virCommandPtr cmd,
+ virDomainDefPtr def,
+ virDomainShmemDefPtr shmem,
virQEMUCapsPtr qemuCaps);
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index c56f9f1..df422e2 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2603,6 +2603,12 @@ void qemuDomainLogContextRef(qemuDomainLogContextPtr ctxt)
}
+virLogManagerPtr qemuDomainLogContextGetManager(qemuDomainLogContextPtr ctxt)
+{
+ return ctxt->manager;
+}
+
+
void qemuDomainLogContextFree(qemuDomainLogContextPtr ctxt)
{
bool lastRef;
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 8359b1a..f6ce19c 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -35,6 +35,7 @@
# include "qemu_capabilities.h"
# include "virchrdev.h"
# include "virobject.h"
+# include "logging/log_manager.h"
# define QEMU_DOMAIN_FORMAT_LIVE_FLAGS \
(VIR_DOMAIN_XML_SECURE | \
@@ -377,6 +378,8 @@ void qemuDomainLogContextMarkPosition(qemuDomainLogContextPtr ctxt);
void qemuDomainLogContextRef(qemuDomainLogContextPtr ctxt);
void qemuDomainLogContextFree(qemuDomainLogContextPtr ctxt);
+virLogManagerPtr qemuDomainLogContextGetManager(qemuDomainLogContextPtr ctxt);
+
const char *qemuFindQemuImgBinary(virQEMUDriverPtr driver);
int qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm,
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 45ff3c0..1afafd6 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7159,7 +7159,7 @@ static char *qemuConnectDomainXMLToNative(virConnectPtr conn,
}
}
- if (!(cmd = qemuBuildCommandLine(conn, driver, def,
+ if (!(cmd = qemuBuildCommandLine(conn, driver, NULL, def,
&monConfig, monitor_json, qemuCaps,
NULL, NULL,
VIR_NETDEV_VPORT_PROFILE_OP_NO_OP,
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index c5b9f4e..3780d0c 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -4987,7 +4987,9 @@ qemuProcessLaunch(virConnectPtr conn,
}
VIR_DEBUG("Building emulator command line");
- if (!(cmd = qemuBuildCommandLine(conn, driver, vm->def, priv->monConfig,
+ if (!(cmd = qemuBuildCommandLine(conn, driver,
+ qemuDomainLogContextGetManager(logCtxt),
+ vm->def, priv->monConfig,
priv->monJSON, priv->qemuCaps,
incoming ? incoming->launchURI : NULL,
snapshot, vmop,
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 624639e..b5323b4 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -345,7 +345,7 @@ static int testCompareXMLToArgvFiles(const char *xml,
testFailed = true;
if (!testFailed &&
- !(cmd = qemuBuildCommandLine(conn, &driver, vmdef, &monitor_chr,
+ !(cmd = qemuBuildCommandLine(conn, &driver, NULL, vmdef, &monitor_chr,
(flags & FLAG_JSON), extraFlags,
migrateURI, NULL,
VIR_NETDEV_VPORT_PROFILE_OP_NO_OP,
11:01 a.m.
New subject: [libvirt] [PATCH 4/5] qemu: use virtlogd for character device log files
On Fri, Feb 26, 2016 at 10:33:30AM -0500, John Ferlan wrote:
On 02/23/2016 11:41 AM, Daniel P. Berrange wrote:
> If use of virtlogd is enabled, then use it for backing the
> character device log files too.
> ---
> src/logging/log_daemon_dispatch.c | 3 +-
> src/logging/log_handler.c | 6 +-
> src/logging/log_handler.h | 2 +-
> src/logging/log_manager.h | 2 +
> src/logging/log_protocol.x | 4 +
> src/qemu/qemu_command.c | 234 ++++++++++++++++++++++++--------------
<sigh> One of us will have conflicts depending upon whether my final
pile of qemu_command.c cleanup changes gets reviewed...
> src/qemu/qemu_command.h | 7 +-
> src/qemu/qemu_domain.c | 6 +
> src/qemu/qemu_domain.h | 3 +
> src/qemu/qemu_driver.c | 2 +-
> src/qemu/qemu_process.c | 4 +-
> tests/qemuxml2argvtest.c | 2 +-
> 12 files changed, 179 insertions(+), 96 deletions(-)
>
> diff --git a/src/logging/log_daemon_dispatch.c b/src/logging/log_daemon_dispatch.c
> index a5fa7f0..b00cee2 100644
> --- a/src/logging/log_daemon_dispatch.c
> +++ b/src/logging/log_daemon_dispatch.c
> @@ -50,13 +50,14 @@ virLogManagerProtocolDispatchDomainOpenLogFile(virNetServerPtr
server ATTRIBUTE_
> int rv = -1;
> off_t offset;
> ino_t inode;
> + bool trunc = args->flags &
VIR_LOG_MANAGER_PROTOCOL_DOMAIN_OPEN_LOG_FILE_TRUNCATE;
>
So by changing this means you don't envision other flags in the future
for virLogHandlerDomainOpenLogFile?
IOW: Why isn't this done in the next layer down? Seems we'd be changing
what we're doing (not that anything else is using this yet).
This flag is defined by the remote protocol. I want to keep the
virLogHandler class isolated from the remote protocol, by not
exposing the flags to it. Hence I've gone for named arguments
in the internal API and convert from the flags to the named
arguemnts.
Should perhaps this part be a separate patch?
> if ((fd = virLogHandlerDomainOpenLogFile(virLogDaemonGetHandler(logDaemon),
> args->driver,
> (unsigned char *)args->dom.uuid,
> args->dom.name,
> args->path,
> - args->flags,
> + trunc,
> &inode, &offset)) < 0)
> goto cleanup;
>
> diff --git a/src/logging/log_handler.c b/src/logging/log_handler.c
> index 92cff50..4c08223 100644
> --- a/src/logging/log_handler.c
> +++ b/src/logging/log_handler.c
> @@ -357,7 +357,7 @@ virLogHandlerDomainOpenLogFile(virLogHandlerPtr handler,
> const unsigned char *domuuid,
> const char *domname,
> const char *path,
> - unsigned int flags,
> + bool trunc,
> ino_t *inode,
> off_t *offset)
> {
> @@ -365,8 +365,6 @@ virLogHandlerDomainOpenLogFile(virLogHandlerPtr handler,
> virLogHandlerLogFilePtr file = NULL;
> int pipefd[2] = { -1, -1 };
>
> - virCheckFlags(0, -1);
> -
> virObjectLock(handler);
>
> handler->inhibitor(true, handler->opaque);
> @@ -400,7 +398,7 @@ virLogHandlerDomainOpenLogFile(virLogHandlerPtr handler,
> if ((file->file = virRotatingFileWriterNew(path,
> DEFAULT_FILE_SIZE,
> DEFAULT_MAX_BACKUP,
> - false,
> + trunc,
> DEFAULT_MODE)) == NULL)
> goto error;
>
> diff --git a/src/logging/log_handler.h b/src/logging/log_handler.h
> index e61f32d..54a9cd9 100644
> --- a/src/logging/log_handler.h
> +++ b/src/logging/log_handler.h
> @@ -48,7 +48,7 @@ int virLogHandlerDomainOpenLogFile(virLogHandlerPtr handler,
> const unsigned char *domuuid,
> const char *domname,
> const char *path,
> - unsigned int flags,
> + bool trunc,
> ino_t *inode,
> off_t *offset);
>
> diff --git a/src/logging/log_manager.h b/src/logging/log_manager.h
> index d3b9d29..7deaba7 100644
> --- a/src/logging/log_manager.h
> +++ b/src/logging/log_manager.h
> @@ -26,6 +26,8 @@
>
> # include "internal.h"
>
> +# include "logging/log_protocol.h"
> +
> typedef struct _virLogManager virLogManager;
> typedef virLogManager *virLogManagerPtr;
>
> diff --git a/src/logging/log_protocol.x b/src/logging/log_protocol.x
> index b0ac31b..0363c75 100644
> --- a/src/logging/log_protocol.x
> +++ b/src/logging/log_protocol.x
> @@ -30,6 +30,10 @@ struct virLogManagerProtocolLogFilePosition {
> };
> typedef struct virLogManagerProtocolLogFilePosition
virLogManagerProtocolLogFilePosition;
>
> +enum virLogManagerProtocolDomainOpenLogFileFlags {
> + VIR_LOG_MANAGER_PROTOCOL_DOMAIN_OPEN_LOG_FILE_TRUNCATE = 1
> +};
> +
DomainOpenLogFile doesn't have flags now - it just has trunc - so
nothing could be added here (at least w/r/t how I understand it).
The wire protocol *does* have flags defined currently:
struct virLogManagerProtocolDomainOpenLogFileArgs {
virLogManagerProtocolNonNullString driver;
virLogManagerProtocolDomain dom;
virLogManagerProtocolNonNullString path;
unsigned int flags;
};
this code is just defining the first flag we'll use on the wire.
> +/**
> + * qemuVirCommandGetFDSet:
> + * @cmd: the command to modify
> + * @fd: fd to reassign to the child
> + *
> + * Get the parameters for the QEMU -add-fd command line option
> + * for the given file descriptor. The file descriptor must previously
> + * have been 'transferred' in a virCommandPassFD() call.
> + * This function for example returns "set=10,fd=20".
> + */
> +static char *
> +qemuVirCommandGetFDSet(virCommandPtr cmd, int fd)
> +{
> + char *result = NULL;
> + int idx = virCommandPassFDGetFDIndex(cmd, fd);
> +
> + if (idx >= 0) {
> + ignore_value(virAsprintf(&result, "set=%d,fd=%d", idx, fd));
> + } else {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("file descriptor %d has not been transferred"),
fd);
> + }
> +
> + return result;
> +}
> +
> +
> +/**
> + * qemuVirCommandGetDevSet:
> + * @cmd: the command to modify
> + * @fd: fd to reassign to the child
> + *
> + * Get the parameters for the QEMU path= parameter where a file
> + * descriptor is accessed via a file descriptor set, for example
> + * /dev/fdset/10. The file descriptor must previously have been
> + * 'transferred' in a virCommandPassFD() call.
> + */
> +static char *
> +qemuVirCommandGetDevSet(virCommandPtr cmd, int fd)
> +{
> + char *result = NULL;
> + int idx = virCommandPassFDGetFDIndex(cmd, fd);
> +
> + if (idx >= 0) {
> + ignore_value(virAsprintf(&result, "/dev/fdset/%d", idx));
> + } else {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("file descriptor %d has not been transferred"),
fd);
> + }
> + return result;
> +}
> +
> +
[1] Code motion... Should it be separate?
Yeah, that would be best practice.
> @@ -3973,10 +4030,42 @@ qemuBuildChrChardevStr(const
virDomainChrSourceDef *dev,
> _("logfile not supported in this QEMU
binary"));
> goto error;
> }
> - virBufferAsprintf(&buf, ",logfile=%s", dev->logfile);
> - if (dev->logappend != VIR_TRISTATE_SWITCH_ABSENT) {
> - virBufferAsprintf(&buf, ",logappend=%s",
> - virTristateSwitchTypeToString(dev->logappend));
> + if (logManager) {
> + char *fdset, *fdpath;
> + int flags = 0;
> + int logfd;
> +
> + if (dev->logappend == VIR_TRISTATE_SWITCH_OFF)
> + flags |= VIR_LOG_MANAGER_PROTOCOL_DOMAIN_OPEN_LOG_FILE_TRUNCATE;
> +
> + if ((logfd = virLogManagerDomainOpenLogFile(logManager,
> + "qemu",
> + def->uuid,
> + def->name,
> + dev->logfile,
> + flags,
> + NULL, NULL)) < 0)
> + goto error;
> +
> + virCommandPassFD(cmd, logfd, VIR_COMMAND_PASS_FD_CLOSE_PARENT);
> + if (!(fdset = qemuVirCommandGetFDSet(cmd, logfd)))
Would this error path need to close logfd or is that done because of the
above PassFD path?
Yep, because we pass VIR_COMMAND_PASS_FD_CLOSE_PARENT, we're guaranteed
that virCommandFree will always close the FD for us, no matter what
happens.
> + goto error;
> +
> + virCommandAddArg(cmd, "-add-fd");
> + virCommandAddArg(cmd, fdset);
> + VIR_FREE(fdset);
> +
> + if (!(fdpath = qemuVirCommandGetDevSet(cmd, logfd)))
Similar question about logfd...
> + goto error;
> +
> + virBufferAsprintf(&buf, ",logfile=%s,logappend=on",
fdpath);
Always on? Doesn't matter what's been provided in logappend in the XML?
The append vs truncate decision is taken by virtlogd - we pass that info
in the virLogManagerDomainOpenLogFile call flags parameter above.
At the QEMU level, it just gets an anonymous pipe file descriptor, which
can only ever be used in append mode - you can't call ftruncate() on a
pipe.
> @@ -4106,10 +4197,11 @@
qemuBuildMonitorCommandLine(virCommandPtr cmd,
> /* Use -chardev if it's available */
> if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_CHARDEV)) {
>
> - virCommandAddArg(cmd, "-chardev");
> - if (!(chrdev = qemuBuildChrChardevStr(monitor_chr, "monitor",
> + if (!(chrdev = qemuBuildChrChardevStr(logManager, cmd, def,
> + monitor_chr, "monitor",
> qemuCaps)))
> return -1;
> + virCommandAddArg(cmd, "-chardev");
doh! Although I suppose one could make the argument that all the
additions of "-chardev" could be their own patch...
Yeah, could be cleaner to separate the chardev move
Regards,
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 :|
10:41 a.m.
New subject: [libvirt] [PATCH 5/5] qemu: support use of virtlogd with file based chardevs
Currently the file based character devices let QEMU write
directly to a file on disk. This allows a malicious QEMU
to inflict a denial of service by consuming all free space.
Switch QEMU to use a pipe to virtlogd, which will enforce
file rollover.
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
src/qemu/qemu_command.c | 50 ++++++++++++++++++++++++++++++++++++++++---------
1 file changed, 41 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 8378470..9ed1b97 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3934,17 +3934,49 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
break;
case VIR_DOMAIN_CHR_TYPE_FILE:
- virBufferAsprintf(&buf, "file,id=char%s,path=%s", alias,
- dev->data.file.path);
- if (dev->data.file.append != VIR_TRISTATE_SWITCH_ABSENT) {
- if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_CHARDEV_FILE_APPEND)) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("append not supported in this QEMU binary"));
+ if (logManager && virQEMUCapsGet(qemuCaps,
QEMU_CAPS_CHARDEV_FILE_APPEND)) {
+ char *fdset, *fdpath;
+ int flags = 0;
+ int logfd;
+
+ if (dev->data.file.append == VIR_TRISTATE_SWITCH_OFF)
+ flags |= VIR_LOG_MANAGER_PROTOCOL_DOMAIN_OPEN_LOG_FILE_TRUNCATE;
+
+ if ((logfd = virLogManagerDomainOpenLogFile(logManager,
+ "qemu",
+ def->uuid,
+ def->name,
+ dev->data.file.path,
+ flags,
+ NULL, NULL)) < 0)
goto error;
- }
- virBufferAsprintf(&buf, ",append=%s",
- virTristateSwitchTypeToString(dev->data.file.append));
+ virCommandPassFD(cmd, logfd, VIR_COMMAND_PASS_FD_CLOSE_PARENT);
+ if (!(fdset = qemuVirCommandGetFDSet(cmd, logfd)))
+ goto error;
+
+ virCommandAddArg(cmd, "-add-fd");
+ virCommandAddArg(cmd, fdset);
+ VIR_FREE(fdset);
+
+ if (!(fdpath = qemuVirCommandGetDevSet(cmd, logfd)))
+ goto error;
+
+ virBufferAsprintf(&buf, "file,id=char%s,path=%s,append=on",
alias, fdpath);
+ VIR_FREE(fdpath);
+ } else {
+ virBufferAsprintf(&buf, "file,id=char%s,path=%s", alias,
+ dev->data.file.path);
+ if (dev->data.file.append != VIR_TRISTATE_SWITCH_ABSENT) {
+ if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_CHARDEV_FILE_APPEND)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("append not supported in this QEMU
binary"));
+ goto error;
+ }
+
+ virBufferAsprintf(&buf, ",append=%s",
+
virTristateSwitchTypeToString(dev->data.file.append));
+ }
}
break;
--
2.5.0
9:37 a.m.
New subject: [libvirt] [PATCH 5/5] qemu: support use of virtlogd with file based chardevs
On 02/23/2016 11:41 AM, Daniel P. Berrange wrote:
Currently the file based character devices let QEMU write
directly to a file on disk. This allows a malicious QEMU
to inflict a denial of service by consuming all free space.
Switch QEMU to use a pipe to virtlogd, which will enforce
file rollover.
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
src/qemu/qemu_command.c | 50 ++++++++++++++++++++++++++++++++++++++++---------
1 file changed, 41 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 8378470..9ed1b97 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3934,17 +3934,49 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
break;
case VIR_DOMAIN_CHR_TYPE_FILE:
- virBufferAsprintf(&buf, "file,id=char%s,path=%s", alias,
- dev->data.file.path);
- if (dev->data.file.append != VIR_TRISTATE_SWITCH_ABSENT) {
- if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_CHARDEV_FILE_APPEND)) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("append not supported in this QEMU
binary"));
+ if (logManager && virQEMUCapsGet(qemuCaps,
QEMU_CAPS_CHARDEV_FILE_APPEND)) {
+ char *fdset, *fdpath;
+ int flags = 0;
+ int logfd;
+
+ if (dev->data.file.append == VIR_TRISTATE_SWITCH_OFF)
+ flags |= VIR_LOG_MANAGER_PROTOCOL_DOMAIN_OPEN_LOG_FILE_TRUNCATE;
+
+ if ((logfd = virLogManagerDomainOpenLogFile(logManager,
+ "qemu",
+ def->uuid,
+ def->name,
+ dev->data.file.path,
+ flags,
+ NULL, NULL)) < 0)
goto error;
- }
- virBufferAsprintf(&buf, ",append=%s",
- virTristateSwitchTypeToString(dev->data.file.append));
+ virCommandPassFD(cmd, logfd, VIR_COMMAND_PASS_FD_CLOSE_PARENT);
+ if (!(fdset = qemuVirCommandGetFDSet(cmd, logfd)))
+ goto error;
+
+ virCommandAddArg(cmd, "-add-fd");
+ virCommandAddArg(cmd, fdset);
+ VIR_FREE(fdset);
+
+ if (!(fdpath = qemuVirCommandGetDevSet(cmd, logfd)))
+ goto error;
+
+ virBufferAsprintf(&buf, "file,id=char%s,path=%s,append=on",
alias, fdpath);
+ VIR_FREE(fdpath);
hmmmm this all looks very familiar... Seems we could have common API w/
patch 4... I'd obviously would have the same questions ;-)
John
+ } else {
+ virBufferAsprintf(&buf, "file,id=char%s,path=%s", alias,
+ dev->data.file.path);
+ if (dev->data.file.append != VIR_TRISTATE_SWITCH_ABSENT) {
+ if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_CHARDEV_FILE_APPEND)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("append not supported in this QEMU
binary"));
+ goto error;
+ }
+
+ virBufferAsprintf(&buf, ",append=%s",
+
virTristateSwitchTypeToString(dev->data.file.append));
+ }
}
break;
3192
days inactive
3195
days old
12 comments
2 participants
participants (2)
-
Daniel P. Berrange -
John Ferlan