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

Re: [libvirt] [PATCH] Fix for parallel port passtrough for QEMU



On 05/22/2012 11:50 PM, Beat Joerg ssatr ch 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 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]