[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH V6 3/3] Add support for file descriptor sets



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 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

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]