On 05/22/2012 11:50 PM, Beat.Joerg@xxxxxxxx wrote: > Hi > > As requested by Dave, I send this to the list. Thank you. > > I came across a bug that the command line generated for passtrough of the host parallel port /dev/parport0 by libvirt for QEMU is incorrect. s/passtrough/passthrough/ > > It currently produces: > -chardev tty,id=charparallel0,path=/dev/parport0 > -device isa-parallel,chardev=charparallel0,id=parallel0 > > > The first parameter is "tty". It sould be "parport". > > > > If I launch qemu with -chardev parport,... it works as expected. > > > > I have already filled a bug report ( https://bugzilla.redhat.com/show_bug.cgi?id=823879 ), the topic was already on the list some months ago: > > https://www.redhat.com/archives/libvirt-users/2011-September/msg00095.html > > > > Not sure if this is fix is very clean, but for my case it works. > > Perhaps it should also be checked if similar problems / patches are required for other chardev backends. > > > > Kind regards > > Beat > > > > diff -Naur libvirt-0.9.4.orig/src/qemu/qemu_command.c libvirt-0.9.4/src/qemu/qemu_command.c > > --- libvirt-0.9.4.orig/src/qemu/qemu_command.c 2012-05-22 13:40:16.788633656 +0200 > > +++ libvirt-0.9.4/src/qemu/qemu_command.c 2012-05-22 13:52:18.453608557 +0200 > > @@ -2357,8 +2357,14 @@ Unfortunately, your patch came through with severe whitespace mangling. Thankfully, it was small enough, so I reconstructed it by hand. > > break; > > > > case VIR_DOMAIN_CHR_TYPE_DEV: > > - virBufferAsprintf(&buf, "tty,id=char%s,path=%s", alias, > > - dev->data.file.path); > > + if (STRPREFIX(dev->data.file.path, "/dev/parport")) { Path-based parsing is wrong. But we _do_ know whether the user is creating a <parallel> or <serial> device based on the alias name, so we should key off of that instead. > > + virBufferAsprintf(&buf, "parport,id=char%s,path=%s", alias, > > + dev->data.file.path); > > + } > > + else { Style - libvirt prefers '} else {' on one line. > > + virBufferAsprintf(&buf, "tty,id=char%s,path=%s", alias, > > + dev->data.file.path); > Rather than duplicating the entire virBufferAsprintf, you can use an extra %s and the ?: operator to condense to one line. It would also be nice to enhance the testsuite to cover this. With that, I think this patch should have the same effect, but a bit cleaner. I'll go ahead and push it in your name; I updated AUTHORS in the process, so let me know if I need to adjust the spelling at all. src/qemu/qemu_command.c | 5 ++-- .../qemuxml2argv-parallel-parport-chardev.args | 7 +++++ .../qemuxml2argv-parallel-parport-chardev.xml | 29 ++++++++++++++++++++ tests/qemuxml2argvtest.c | 2 ++ 5 files changed, 42 insertions(+), 2 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-parallel-parport-chardev.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-parallel-parport-chardev.xml diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 9ca7641..fb8d9a3 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3365,8 +3365,9 @@ qemuBuildChrChardevStr(virDomainChrSourceDefPtr dev, const char *alias, break; case VIR_DOMAIN_CHR_TYPE_DEV: - virBufferAsprintf(&buf, "tty,id=char%s,path=%s", alias, - dev->data.file.path); + virBufferAsprintf(&buf, "%s,id=char%s,path=%s", + STRPREFIX(alias, "parallel") ? "parport" : "tty", + alias, dev->data.file.path); break; case VIR_DOMAIN_CHR_TYPE_FILE: diff --git a/tests/qemuxml2argvdata/qemuxml2argv-parallel-parport-chardev.args b/tests/qemuxml2argvdata/qemuxml2argv-parallel-parport-chardev.args new file mode 100644 index 0000000..48f968a --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-parallel-parport-chardev.args @@ -0,0 +1,7 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M \ +pc -m 214 -smp 1 -nographic -nodefconfig -nodefaults -chardev socket,\ +id=charmonitor,path=/tmp/test-monitor,server,nowait -mon chardev=charmonitor,\ +id=monitor,mode=readline -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 \ +-chardev parport,id=charparallel0,path=/dev/parport0 -device \ +isa-parallel,chardev=charparallel0,id=parallel0 -usb -device \ +virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-parallel-parport-chardev.xml b/tests/qemuxml2argvdata/qemuxml2argv-parallel-parport-chardev.xml new file mode 100644 index 0000000..b495cdc --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-parallel-parport-chardev.xml @@ -0,0 +1,29 @@ +<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'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='ide' index='0'/> + <parallel type='dev'> + <source path='/dev/parport0'/> + <target port='0'/> + </parallel> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 617b178..7b00ea2 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -604,6 +604,8 @@ mymain(void) QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG); DO_TEST("parallel-tcp-chardev", false, QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG); + DO_TEST("parallel-parport-chardev", false, + QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG); DO_TEST("console-compat-chardev", false, QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG); -- 1.7.10.2 -- Eric Blake eblake@xxxxxxxxxx +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list