Re: [libvirt] [PATCH V1 4/4] Add support for file descriptor sets

On 01/29/2013 09:52 AM, Stefan Berger wrote: Rearranging things just a bit: tty,id=charserial0,path=/dev/fdset/1 pipe,id=charserial1,path=/dev/fdset/2 These two conversions looks sane; although it might help to list the old style next to the new style in the commit message: 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 This conversion feels insufficient for qemu 1.3. As written, your patch can only ever tie one fd to a set (that is, each qemu fdset contains exactly one passed fd). But my understanding of the qemu 1.3 code is that for -drive arguments, qemu_open() is called twice: once with O_RDONLY to probe the file format, and again with O_RDWR to actually use the file; furthermore, qemu_open() refuses to use an O_RDWR fd for O_RDONLY operations (in the name of safety). Arguably, this is a flaw in the qemu design (qemu shouldn't need to open() a file twice to use it, and I argued at the time that qemu implemented fdset that an O_RDWR fd should serve just fine for an O_RDONLY request); but since it exists, we must live with it. That means that for THIS line, the proper translation really has to be: old: -drive file=/var/lib/libvirt/images/fc14-x86_64.img,if=none,id=drive-ide0-0-0,format=raw new: -add-fd set=0,fd=23,opaque=RDONLY:/var/lib/libvirt/images/fc14-x86_64.img -add-fd set=0,fd=24,opaque=RDWR:/var/lib/libvirt/images/fc14-x86_64.img -drive file=/dev/fdset/0,if=none,id=drive-ide0-0-0,format=raw where we really are passing two fds, 23 as O_RDONLY, and 24 as O_RDWR, within the single fdset 0. We need to add a new test: a new tests/qemuxml2argvdata .args file, with the matching .xml file copied from an existing test, along with a line in qemuxml2argvtest.c to run the new test using the new capability right next to the existing test run without the capability. Doing this will make it more obvious that we are generating appropriate command lines with or without the capability detected. I guess it gets tricky, though - the test cannot actually open /dev/ttyS0, but must limit itself to either opening files under control of the testsuite, or factoring the qemu_command.c code so that we can generate the command line that would exist if an fd were open but without actually opening the fd. Missing documentation for @opaque; but at the level of libvirt, we know what it is, and might as well call it 'name' instead of 'opaque' (it is only qemu that is treating it as opaque). Careful - opaque might contain commas, in which case you need to pass it through virBufferEscape(buf, ',', ",", "%s", opaque) instead of using it directly. s/inn/in/ Do these two functions need 'vir' in their name, or could we get by with qemuCommandGetFDSet and qemuCommandGetDevSet? See my thoughts below about printing into an existing virBuffer instead of allocating a new string with virAsprintf. Won't work to call raw open(); you MUST go through qemuOpenFile() (which works around situations such as libvirtd running as root but needing to open a file on a root-squash NFS share); also, this would be the point where we apply security manager labeling to the opened fd. In fact, the caller probably ought to be able supply additional flags to open, such as deciding whether O_CREAT, O_TRUNC, etc. makes sense on a per-caller basis. When a system call fails, we should report it with virReportSystemError(errno, _("...")) I would check for < 0, not just == -ENOMEM, to catch all possible errors (although ENOMEM is the only current error). Missed a virReportOOMError(). Below here... ...it is simpler to write: cleanup: VIR_FREE(devset); VIR_FREE(fdsetstr); return res; since all error paths already ensured res was NULL; then you aren't duplicating the cleanup paths. Hmm, rather than malloc'ing pathfdset only to then copy it into the malloc'd opt buffer, why not just print the path directly into the opt buffer to begin with? In other words, instead of having qemuCreatePathForFilePath return a char*, instead, have it take a virBufferPtr argument (the existing &opt), and merely append the right contents (path with comma escaping for old qemu, and /dev/fdset/%n for new qemu). Hmm - does fd passing really work for filesystem pass through? After all, that implies that qemu will be opening multiple files beneath the directory being passed as the source name. It may be that fdset and fs passthrough are incompatible. Again, printing directly into the existing &buf, instead of allocating a new string only to copy pathfdset into the existing &buf, would be nicer. Something like: virBufferAsprintf(&buf, "%s,id=char%s,", STRPREFIX(alias, "parallel") ? "parport" : "tty", alias); if (qemuCreatePathForFile(&buf, "path=%s", dev->data.file.path, cmd, fdset, caps) < 0) goto error; I'm also working on some patches to expose the QMP commands via src/qemu/qemu_monitor.h, which will work in tandem with this series, once we figure out in what direction we are heading. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org