Re: [libvirt] [PATCHv2 1/5] qemu: avoid adding "" in smbios arguments

2010-12-07 Thread Eric Blake 
On 12/05/2010 12:57 AM, Daniel Veillard wrote:
> On Fri, Dec 03, 2010 at 02:56:14PM -0700, Eric Blake wrote:
>> The log lists things like -smbios type=1,vendor="Red Hat", which
>> is great for shell parsing, but not so great when you realize that
>> execve() then passes those literal "" on as part of the command
>> line argument, such that qemu sets SMBIOS with extra literal quotes.
> 
>   Hum, I was afraid that QEmu parsing would fail in case of spaces if
> there is no quote, but if you checked this, sure !

What's happening here is that we are building up execve arguments, and
supplying roughly:

"qemu" "-smbios" "type=0,vendor=\"Red Hat\",version=\"Fedora 14\""

instead of the intended:

"qemu" "-smbios" "type=0,vendor=Red Hat,version=Fedora 14"

Although qemu uses a hand-rolled loop instead of getsubopt(), it looks
like qemu is using the same algorithm as getsubopt, where it parses
everything between '=' and ',', including spaces, as the subopt
argument.  At any rate, yes, I did test this; the only thing you can't
pass through qemu's -smbios is a literal comma, but that's already
excluded from our domain.rng schema. :)

>   ACK,

Thanks; I've pushed 1 through 4; I'm waiting to push 5 until after my
virCommand buffer patches have been ACK'd, so as to avoid any question
of any potential NULL dereferences due to the virCommandSetOutputBuffer
calls.

-- 
Eric Blake   ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCHv2 1/5] qemu: avoid adding "" in smbios arguments

2010-12-05 Thread Daniel Veillard 
On Fri, Dec 03, 2010 at 02:56:14PM -0700, Eric Blake wrote:
> The log lists things like -smbios type=1,vendor="Red Hat", which
> is great for shell parsing, but not so great when you realize that
> execve() then passes those literal "" on as part of the command
> line argument, such that qemu sets SMBIOS with extra literal quotes.

  Hum, I was afraid that QEmu parsing would fail in case of spaces if
there is no quote, but if you checked this, sure !

>  src/qemu/qemu_conf.c|   20 ++--
>  tests/qemuxml2argvdata/qemuxml2argv-smbios.args |2 +-
>  2 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index 925585a..8985241 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -3622,16 +3622,16 @@ static char *qemuBuildSmbiosBiosStr(virSysinfoDefPtr 
> def)
> 
>  /* 0:Vendor */
>  if (def->bios_vendor)
> -virBufferVSprintf(&buf, ",vendor=\"%s\"", def->bios_vendor);
> +virBufferVSprintf(&buf, ",vendor=%s", def->bios_vendor);
>  /* 0:BIOS Version */
>  if (def->bios_version)
> -virBufferVSprintf(&buf, ",version=\"%s\"", def->bios_version);
> +virBufferVSprintf(&buf, ",version=%s", def->bios_version);
>  /* 0:BIOS Release Date */
>  if (def->bios_date)
> -virBufferVSprintf(&buf, ",date=\"%s\"", def->bios_date);
> +virBufferVSprintf(&buf, ",date=%s", def->bios_date);
>  /* 0:System BIOS Major Release and 0:System BIOS Minor Release */
>  if (def->bios_release)
> -virBufferVSprintf(&buf, ",release=\"%s\"", def->bios_release);
> +virBufferVSprintf(&buf, ",release=%s", def->bios_release);
> 
>  if (virBufferError(&buf)) {
>  virReportOOMError();
> @@ -3658,23 +3658,23 @@ static char 
> *qemuBuildSmbiosSystemStr(virSysinfoDefPtr def)
> 
>  /* 1:Manufacturer */
>  if (def->system_manufacturer)
> -virBufferVSprintf(&buf, ",manufacturer=\"%s\"",
> +virBufferVSprintf(&buf, ",manufacturer=%s",
>def->system_manufacturer);
>   /* 1:Product Name */
>  if (def->system_product)
> -virBufferVSprintf(&buf, ",product=\"%s\"", def->system_product);
> +virBufferVSprintf(&buf, ",product=%s", def->system_product);
>  /* 1:Version */
>  if (def->system_version)
> -virBufferVSprintf(&buf, ",version=\"%s\"", def->system_version);
> +virBufferVSprintf(&buf, ",version=%s", def->system_version);
>  /* 1:Serial Number */
>  if (def->system_serial)
> -virBufferVSprintf(&buf, ",serial=\"%s\"", def->system_serial);
> +virBufferVSprintf(&buf, ",serial=%s", def->system_serial);
>  /* 1:UUID */
>  if (def->system_uuid)
> -virBufferVSprintf(&buf, ",uuid=\"%s\"", def->system_uuid);
> +virBufferVSprintf(&buf, ",uuid=%s", def->system_uuid);
>  /* 1:SKU Number */
>  if (def->system_sku)
> -virBufferVSprintf(&buf, ",sku=\"%s\"", def->system_sku);
> +virBufferVSprintf(&buf, ",sku=%s", def->system_sku);
> 
>  if (virBufferError(&buf)) {
>  virReportOOMError();
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-smbios.args 
> b/tests/qemuxml2argvdata/qemuxml2argv-smbios.args
> index d5bd289..bd3ede4 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-smbios.args
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-smbios.args
> @@ -1 +1 @@
> -LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S 
> -M pc -m 214 -smp 1 -smbios type=0,vendor="QEmu/KVM",version="0.13" -smbios 
> type=1,manufacturer="Fedora",product="Virt-Manager",version="0.8.2-3.fc14",serial="32dfcb37-5af1-552b-357c-be8c3aa38310",uuid="c7a5fdbd-edaf-9455-926a-d65c16db1809"
>  -nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c 
> -hda /dev/HostVG/QEMUGuest1 -net none -serial none -parallel none -usb
> +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S 
> -M pc -m 214 -smp 1 -smbios type=0,vendor=QEmu/KVM,version=0.13 -smbios 
> type=1,manufacturer=Fedora,product=Virt-Manager,version=0.8.2-3.fc14,serial=32dfcb37-5af1-552b-357c-be8c3aa38310,uuid=c7a5fdbd-edaf-9455-926a-d65c16db1809
>  -nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c 
> -hda /dev/HostVG/QEMUGuest1 -net none -serial none -parallel none -usb

  ACK,

Daniel

-- 
Daniel Veillard  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
dan...@veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/

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

[libvirt] [PATCHv2 1/5] qemu: avoid adding "" in smbios arguments

2010-12-03 Thread Eric Blake 
The log lists things like -smbios type=1,vendor="Red Hat", which
is great for shell parsing, but not so great when you realize that
execve() then passes those literal "" on as part of the command
line argument, such that qemu sets SMBIOS with extra literal quotes.

The eventual addition of virCommand is needed before we have the API
to shell-quote a string representation of a command line, so that the
log can still be pasted into a shell, but without inserting extra
bytes into the execve() arguments.

* src/qemu/qemu_conf.c (qemuBuildSmbiosBiosStr)
(qemuBuildSmbiosSystemStr): Qemu doesn't like quotes around uuid
arguments, and the remaining quotes are passed literally to
smbios, making  inaccurate.  Removing the
quotes makes the log harder to parse, but that can be fixed later
with virCommand improvements.
* tests/qemuxml2argvdata/qemuxml2argv-smbios.args: 'Fix' test; it
will need fixing again once virCommand learns how to shell-quote a
potential command line.
---
 src/qemu/qemu_conf.c|   20 ++--
 tests/qemuxml2argvdata/qemuxml2argv-smbios.args |2 +-
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 925585a..8985241 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -3622,16 +3622,16 @@ static char *qemuBuildSmbiosBiosStr(virSysinfoDefPtr 
def)

 /* 0:Vendor */
 if (def->bios_vendor)
-virBufferVSprintf(&buf, ",vendor=\"%s\"", def->bios_vendor);
+virBufferVSprintf(&buf, ",vendor=%s", def->bios_vendor);
 /* 0:BIOS Version */
 if (def->bios_version)
-virBufferVSprintf(&buf, ",version=\"%s\"", def->bios_version);
+virBufferVSprintf(&buf, ",version=%s", def->bios_version);
 /* 0:BIOS Release Date */
 if (def->bios_date)
-virBufferVSprintf(&buf, ",date=\"%s\"", def->bios_date);
+virBufferVSprintf(&buf, ",date=%s", def->bios_date);
 /* 0:System BIOS Major Release and 0:System BIOS Minor Release */
 if (def->bios_release)
-virBufferVSprintf(&buf, ",release=\"%s\"", def->bios_release);
+virBufferVSprintf(&buf, ",release=%s", def->bios_release);

 if (virBufferError(&buf)) {
 virReportOOMError();
@@ -3658,23 +3658,23 @@ static char *qemuBuildSmbiosSystemStr(virSysinfoDefPtr 
def)

 /* 1:Manufacturer */
 if (def->system_manufacturer)
-virBufferVSprintf(&buf, ",manufacturer=\"%s\"",
+virBufferVSprintf(&buf, ",manufacturer=%s",
   def->system_manufacturer);
  /* 1:Product Name */
 if (def->system_product)
-virBufferVSprintf(&buf, ",product=\"%s\"", def->system_product);
+virBufferVSprintf(&buf, ",product=%s", def->system_product);
 /* 1:Version */
 if (def->system_version)
-virBufferVSprintf(&buf, ",version=\"%s\"", def->system_version);
+virBufferVSprintf(&buf, ",version=%s", def->system_version);
 /* 1:Serial Number */
 if (def->system_serial)
-virBufferVSprintf(&buf, ",serial=\"%s\"", def->system_serial);
+virBufferVSprintf(&buf, ",serial=%s", def->system_serial);
 /* 1:UUID */
 if (def->system_uuid)
-virBufferVSprintf(&buf, ",uuid=\"%s\"", def->system_uuid);
+virBufferVSprintf(&buf, ",uuid=%s", def->system_uuid);
 /* 1:SKU Number */
 if (def->system_sku)
-virBufferVSprintf(&buf, ",sku=\"%s\"", def->system_sku);
+virBufferVSprintf(&buf, ",sku=%s", def->system_sku);

 if (virBufferError(&buf)) {
 virReportOOMError();
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-smbios.args 
b/tests/qemuxml2argvdata/qemuxml2argv-smbios.args
index d5bd289..bd3ede4 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-smbios.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-smbios.args
@@ -1 +1 @@
-LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M 
pc -m 214 -smp 1 -smbios type=0,vendor="QEmu/KVM",version="0.13" -smbios 
type=1,manufacturer="Fedora",product="Virt-Manager",version="0.8.2-3.fc14",serial="32dfcb37-5af1-552b-357c-be8c3aa38310",uuid="c7a5fdbd-edaf-9455-926a-d65c16db1809"
 -nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -hda 
/dev/HostVG/QEMUGuest1 -net none -serial none -parallel none -usb
+LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M 
pc -m 214 -smp 1 -smbios type=0,vendor=QEmu/KVM,version=0.13 -smbios 
type=1,manufacturer=Fedora,product=Virt-Manager,version=0.8.2-3.fc14,serial=32dfcb37-5af1-552b-357c-be8c3aa38310,uuid=c7a5fdbd-edaf-9455-926a-d65c16db1809
 -nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -hda 
/dev/HostVG/QEMUGuest1 -net none -serial none -parallel none -usb
-- 
1.7.3.2

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