Re: [PATCH V3 2/2] Add support for file descriptor sets

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

 



On Mon, Feb 04, 2013 at 07:12:38PM -0500, 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

Given this significant change to the ARGV generation, I'd expect to
see more than just this change in the test suite:

> Index: libvirt/tests/qemuxml2argvtest.c
> ===================================================================
> --- libvirt.orig/tests/qemuxml2argvtest.c
> +++ libvirt/tests/qemuxml2argvtest.c
> @@ -151,7 +151,8 @@ static int testCompareXMLToArgvFiles(con
>      if (!(cmd = qemuBuildCommandLine(conn, &driver, vmdef, &monitor_chr,
>                                       (flags & FLAG_JSON), extraFlags,
>                                       migrateFrom, migrateFd, NULL,
> -                                     VIR_NETDEV_VPORT_PROFILE_OP_NO_OP))) {
> +                                     VIR_NETDEV_VPORT_PROFILE_OP_NO_OP,
> +                                     NULL))) {
>          if (flags & FLAG_EXPECT_FAILURE) {
>              ret = 0;
>              virResetLastError();
> Index: libvirt/tests/qemuxmlnstest.c
> ===================================================================
> --- libvirt.orig/tests/qemuxmlnstest.c
> +++ libvirt/tests/qemuxmlnstest.c
> @@ -110,7 +110,8 @@ static int testCompareXMLToArgvFiles(con
>      if (!(cmd = qemuBuildCommandLine(conn, &driver,
>                                       vmdef, &monitor_chr, json, extraFlags,
>                                       migrateFrom, migrateFd, NULL,
> -                                     VIR_NETDEV_VPORT_PROFILE_OP_NO_OP)))
> +                                     VIR_NETDEV_VPORT_PROFILE_OP_NO_OP,
> +                                     NULL)))
>          goto fail;
>  
>      if (!!virGetLastError() != expectError) {


Yes, this nicely shows that you didn't break any existing QEMU ARGV
generation, but at the same time it shows there's zero testing coverage
of the new code


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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list


[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]