11:01 a.m.
On 02/14/2013 05:00 AM, Stefan Berger wrote:
Add support for file descriptor sets by converting some of the
command line parameters to use /dev/fdset/%d if -add-fd is found
to be supported by QEMU. For those devices libvirt now open()s
the device to obtain the file descriptor and 'transfers' the
fd using virCommand.
For the following fragments of domain XML
<disk type='file' device='disk'>
<driver name='qemu' type='raw'/>
<source file='/var/lib/libvirt/images/fc14-x86_64.img'/>
<target dev='hda' bus='ide'/>
<address type='drive' controller='0' bus='0'
target='0' unit='0'/>
</disk>
<serial type='dev'>
<source path='/dev/ttyS0'/>
<target port='0'/>
</serial>
<serial type='pipe'>
<source path='/tmp/testpipe'/>
<target port='1'/>
</serial>
libvirt now creates the following parts for the QEMU command line:
old: -drive
file=/var/lib/libvirt/images/fc14-x86_64.img,if=none,id=drive-ide0-0-0,format=raw
new: -add-fd set=1,fd=23,opaque=RDONLY:/var/lib/libvirt/images/fc14-x86_64.img
-add-fd set=1,fd=24,opaque=RDWR:/var/lib/libvirt/images/fc14-x86_64.img
-drive file=/dev/fdset/1,if=none,id=drive-ide0-0-0,format=raw
old: -chardev tty,id=charserial0,path=/dev/ttyS0
new: -add-fd set=1,fd=30,opaque=/dev/ttyS0
-chardev tty,id=charserial0,path=/dev/fdset/1
old: -chardev pipe,id=charserial1,path=/tmp/testpipe
new: -add-fd set=2,fd=32,opaque=/tmp/testpipe
-chardev pipe,id=charserial1,path=/dev/fdset/2
The example might be a little clearer if you use sets 1, 2, 3 instead of
1, 1, 2.
Test cases are part of this patch now.
Signed-off-by: Stefan Berger <stefanb(a)linux.vnet.ibm.com>
Hmm, I started reviewing this before I saw that you sent a v7. I'll go
ahead and send anyways, in case it helps.
/**
+ * qemuCommandPrintFDSet:
+ * @fdset: the number of the file descriptor set to which @fd belongs
+ * @fd: fd that will be assigned to QEMU
+ * @open_flags: the flags used for opening the fd; of interest are only
+ * O_RDONLY, O_WRONLY, O_RDWR
+ * @name: optional name of the file
+ *
+ * Write the parameters for the QEMU -add-fd command line option
+ * for the given file descriptor and return the string.
+ * This function for example returns "set=10,fd=20,opaque=RDWR:/foo/bar".
+ */
+static char *
+qemuCommandPrintFDSet(int fdset, int fd, int open_flags, const char *name)
+{
+ const char *mode = "";
+ virBuffer buf = VIR_BUFFER_INITIALIZER;
+
+ if (name) {
+ switch ((open_flags & O_ACCMODE)) {
+ case O_RDONLY:
+ mode = "RDONLY:";
+ break;
+ case O_WRONLY:
+ mode = "WRONLY:";
+ break;
+ case O_RDWR:
+ mode = "RDWR:";
+ break;
Is it worth a default case when the mode is unrecognized? Then again,
unless the Linux kernel ever gains O_SEARCH/O_EXEC support, there
probably won't ever be any code hitting the default case.
+ }
+ }
+
+ virBufferAsprintf(&buf, "set=%d,fd=%d%s%s", fdset, fd,
+ (name) ? ",opaque=" : "",
+ mode);
+ if (name)
+ virBufferEscape(&buf, ',', ",", "%s", name);
Slightly easier to read as:
virBufferAsprintf(&buf, "set=%d,fd=%d", fdset, fd);
if (name)
virBufferEscape(&buf, ',', ",", ",opaque=%s", name);
+static int
+qemuCreatePathForFilePath(virQEMUDriverPtr driver, virBufferPtr buf,
+ const char *prefix, const char *path,
+ const int *open_flags,
+ virCommandPtr cmd, virFdSetPtr fdset,
+ const virDomainDeviceInfoPtr info,
+ virQEMUCapsPtr qemuCaps)
+{
+ char *fdsetstr = NULL;
+ int i;
+
+ virBufferAdd(buf, prefix, -1);
+ /*
+ * cmd == NULL hints at hotplugging; use old way of doing things
+ * fdset == NULL hints at a call path where we should not open files
+ * In this case we fall back to the old command line
+ * (at least for now)
+ */
+ if (!cmd || !fdset || !virQEMUCapsGet(qemuCaps, QEMU_CAPS_ADD_FD)) {
+ virBufferEscape(buf, ',', ",", "%s", path);
+ } else {
+ unsigned int fdsetnum;
+
+ if (virFdSetNextSet(fdset, info->alias, &fdsetnum) < 0)
+ goto error;
+
+ qemuCommandPrintDevSet(buf, fdsetnum);
+
+ i = 0;
+ while (open_flags[i] != -1) {
Laine was mentioning in another review that we aren't very consistent
between open_flags and openFlags for local variable names, but that the
studlyCaps version seems to be more popular.
Rather than having the user supply a sentinel, would it be better to
have the user provide nopenFlags? That is, when opening a single fd,
passing '&mode, 1' is easier than passing 'int[] { mode, -1}',
especially if we don't want to use C99 initializer syntax. For that
matter, would it be any easier to pass a flags instead of a mode, where
we have the bitwise-or of:
QEMU_USE_RDONLY = 1 << 0, // O_RDONLY
QEMU_USE_RDWR = 1 << 1, // O_RDWR
QEMU_USE WRONLY = 1 << 2, // O_WRONLY
on the grounds that writing 'QEMU_USE_RDONLY | QEMU_USE_RDWR' looks a
little cleaner than writing '(int[]){O_RDONLY, O_RDWR, -1}' (no
temporary arrays required).
Index: libvirt/tests/qemuxml2argvdata/qemuxml2argv-add-fd.args
===================================================================
--- /dev/null
+++ libvirt/tests/qemuxml2argvdata/qemuxml2argv-add-fd.args
@@ -0,0 +1,23 @@
+LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test \
+/usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -nodefaults \
+-chardev socket,id=charmonitor,path=/tmp/test-monitor,server,nowait \
+-mon chardev=charmonitor,id=monitor,mode=readline -no-acpi -boot c -usb \
+\
+-add-fd set=1,fd=3,opaque=RDONLY:/dev/null \
+-add-fd set=1,fd=4,opaque=RDWR:/dev/null \
+-drive file=/dev/fdset/1,if=none,id=drive-ide0-0-0 \
+-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \
+\
Interesting way to make it more legible - I like it.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org