10:50 a.m.
See patch 3 for the core change.
Peter Krempa (4):
qemu: command: Fix up coding style of smbios commandine formatters
qemu: command: Don't bother reporting errors in smbios formatters
qemu: command: escape smbios entry strings
schema: smbios: allow any strings
docs/schemas/domaincommon.rng | 4 +-
src/qemu/qemu_command.c | 129 ++++++++++++++++++++++++------------------
2 files changed, 76 insertions(+), 57 deletions(-)
--
2.10.0
10:51 a.m.
New subject: [libvirt] [PATCH 1/4] qemu: command: Fix up coding style of smbios commandine formatters
---
src/qemu/qemu_command.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 0804133..13c2cde 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -5694,7 +5694,8 @@ qemuBuildRNGCommandLine(virLogManagerPtr logManager,
}
-static char *qemuBuildSmbiosBiosStr(virSysinfoBIOSDefPtr def)
+static char *
+qemuBuildSmbiosBiosStr(virSysinfoBIOSDefPtr def)
{
virBuffer buf = VIR_BUFFER_INITIALIZER;
@@ -5726,8 +5727,10 @@ static char *qemuBuildSmbiosBiosStr(virSysinfoBIOSDefPtr def)
return NULL;
}
-static char *qemuBuildSmbiosSystemStr(virSysinfoSystemDefPtr def,
- bool skip_uuid)
+
+static char *
+qemuBuildSmbiosSystemStr(virSysinfoSystemDefPtr def,
+ bool skip_uuid)
{
virBuffer buf = VIR_BUFFER_INITIALIZER;
@@ -5772,7 +5775,9 @@ static char *qemuBuildSmbiosSystemStr(virSysinfoSystemDefPtr def,
return NULL;
}
-static char *qemuBuildSmbiosBaseBoardStr(virSysinfoBaseBoardDefPtr def)
+
+static char *
+qemuBuildSmbiosBaseBoardStr(virSysinfoBaseBoardDefPtr def)
{
virBuffer buf = VIR_BUFFER_INITIALIZER;
--
2.10.0
10:51 a.m.
New subject: [libvirt] [PATCH 2/4] qemu: command: Don't bother reporting errors in smbios formatters
qemuBuildSmbiosBiosStr and qemuBuildSmbiosSystemStr return NULL if
there's noting to format on the commandline. Reporting errors from
buffer creation doesn't make sense since it would be ignored.
---
src/qemu/qemu_command.c | 14 --------------
1 file changed, 14 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 13c2cde..11ae363 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -5717,14 +5717,7 @@ qemuBuildSmbiosBiosStr(virSysinfoBIOSDefPtr def)
if (def->release)
virBufferAsprintf(&buf, ",release=%s", def->release);
- if (virBufferCheckError(&buf) < 0)
- goto error;
-
return virBufferContentAndReset(&buf);
-
- error:
- virBufferFreeAndReset(&buf);
- return NULL;
}
@@ -5765,14 +5758,7 @@ qemuBuildSmbiosSystemStr(virSysinfoSystemDefPtr def,
if (def->family)
virBufferAsprintf(&buf, ",family=%s", def->family);
- if (virBufferCheckError(&buf) < 0)
- goto error;
-
return virBufferContentAndReset(&buf);
-
- error:
- virBufferFreeAndReset(&buf);
- return NULL;
}
--
2.10.0
4:11 p.m.
New subject: [libvirt] [PATCH 2/4] qemu: command: Don't bother reporting errors in smbios formatters
On 10/10/2016 11:51 AM, Peter Krempa wrote:
qemuBuildSmbiosBiosStr and qemuBuildSmbiosSystemStr return NULL if
there's noting to format on the commandline. Reporting errors from
nothing
buffer creation doesn't make sense since it would be ignored.
Introduced by 54c0237ccb, so it's been this way a long time...
---
src/qemu/qemu_command.c | 14 --------------
1 file changed, 14 deletions(-)
I suppose since it seems the only legitimate error you'll hit is ENOMEM
and if you ignore it here, some shortly to be run code is sure to run
into it, then no big deal...
Although, one could argue the callers should check/return on error, but
they'd probably lose that argument.
ACK for what's here
John
8:59 p.m.
New subject: [libvirt] [PATCH 2/4] qemu: command: Don't bother reporting errors in smbios formatters
On Thu, Oct 13, 2016 at 17:11:54 -0400, John Ferlan wrote:
On 10/10/2016 11:51 AM, Peter Krempa wrote:
> qemuBuildSmbiosBiosStr and qemuBuildSmbiosSystemStr return NULL if
> there's noting to format on the commandline. Reporting errors from
nothing
> buffer creation doesn't make sense since it would be ignored.
Introduced by 54c0237ccb, so it's been this way a long time...
> ---
> src/qemu/qemu_command.c | 14 --------------
> 1 file changed, 14 deletions(-)
>
I suppose since it seems the only legitimate error you'll hit is ENOMEM
and if you ignore it here, some shortly to be run code is sure to run
into it, then no big deal...
Although, one could argue the callers should check/return on error, but
they'd probably lose that argument.
Not really :). I thought the same when writing the patch as it's fully
possible although extremely unlikely that we'd start a VM with invalid
configuration. Given that we were doing it like this for quite some
while and it would require a more invasive refactoring of the code I
just decided to drop the code altogether.
ACK for what's here
Thanks, I'll go with this version currently, since it keeps the
semantics present for a rather long time.
Peter
10:51 a.m.
New subject: [libvirt] [PATCH 3/4] qemu: command: escape smbios entry strings
We pass free-form strings from the users to qemu, thus we need escape
commas since they are passed to qemu monitor.
Partially resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1373535
---
src/qemu/qemu_command.c | 102 +++++++++++++++++++++++++++++++-----------------
1 file changed, 66 insertions(+), 36 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 11ae363..fffeeef 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -5705,17 +5705,25 @@ qemuBuildSmbiosBiosStr(virSysinfoBIOSDefPtr def)
virBufferAddLit(&buf, "type=0");
/* 0:Vendor */
- if (def->vendor)
- virBufferAsprintf(&buf, ",vendor=%s", def->vendor);
+ if (def->vendor) {
+ virBufferAddLit(&buf, ",vendor=");
+ virQEMUBuildBufferEscapeComma(&buf, def->vendor);
+ }
/* 0:BIOS Version */
- if (def->version)
- virBufferAsprintf(&buf, ",version=%s", def->version);
+ if (def->version) {
+ virBufferAddLit(&buf, ",version=");
+ virQEMUBuildBufferEscapeComma(&buf, def->version);
+ }
/* 0:BIOS Release Date */
- if (def->date)
- virBufferAsprintf(&buf, ",date=%s", def->date);
+ if (def->date) {
+ virBufferAddLit(&buf, ",date=");
+ virQEMUBuildBufferEscapeComma(&buf, def->date);
+ }
/* 0:System BIOS Major Release and 0:System BIOS Minor Release */
- if (def->release)
- virBufferAsprintf(&buf, ",release=%s", def->release);
+ if (def->release) {
+ virBufferAddLit(&buf, ",release=");
+ virQEMUBuildBufferEscapeComma(&buf, def->release);
+ }
return virBufferContentAndReset(&buf);
}
@@ -5736,27 +5744,40 @@ qemuBuildSmbiosSystemStr(virSysinfoSystemDefPtr def,
virBufferAddLit(&buf, "type=1");
/* 1:Manufacturer */
- if (def->manufacturer)
- virBufferAsprintf(&buf, ",manufacturer=%s",
- def->manufacturer);
+ if (def->manufacturer) {
+ virBufferAddLit(&buf, ",manufacturer=");
+ virQEMUBuildBufferEscapeComma(&buf, def->manufacturer);
+ }
/* 1:Product Name */
- if (def->product)
- virBufferAsprintf(&buf, ",product=%s", def->product);
+ if (def->product) {
+ virBufferAddLit(&buf, ",product=");
+ virQEMUBuildBufferEscapeComma(&buf, def->product);
+ }
/* 1:Version */
- if (def->version)
- virBufferAsprintf(&buf, ",version=%s", def->version);
+ if (def->version) {
+ virBufferAddLit(&buf, ",version=");
+ virQEMUBuildBufferEscapeComma(&buf, def->version);
+ }
/* 1:Serial Number */
- if (def->serial)
- virBufferAsprintf(&buf, ",serial=%s", def->serial);
+ if (def->serial) {
+ virBufferAddLit(&buf, ",serial=");
+ virQEMUBuildBufferEscapeComma(&buf, def->serial);
+ }
/* 1:UUID */
- if (def->uuid && !skip_uuid)
- virBufferAsprintf(&buf, ",uuid=%s", def->uuid);
+ if (def->uuid && !skip_uuid) {
+ virBufferAddLit(&buf, ",uuid=");
+ virQEMUBuildBufferEscapeComma(&buf, def->uuid);
+ }
/* 1:SKU Number */
- if (def->sku)
- virBufferAsprintf(&buf, ",sku=%s", def->sku);
+ if (def->sku) {
+ virBufferAddLit(&buf, ",sku=");
+ virQEMUBuildBufferEscapeComma(&buf, def->sku);
+ }
/* 1:Family */
- if (def->family)
- virBufferAsprintf(&buf, ",family=%s", def->family);
+ if (def->family) {
+ virBufferAddLit(&buf, ",family=");
+ virQEMUBuildBufferEscapeComma(&buf, def->family);
+ }
return virBufferContentAndReset(&buf);
}
@@ -5773,24 +5794,33 @@ qemuBuildSmbiosBaseBoardStr(virSysinfoBaseBoardDefPtr def)
virBufferAddLit(&buf, "type=2");
/* 2:Manufacturer */
- if (def->manufacturer)
- virBufferAsprintf(&buf, ",manufacturer=%s",
- def->manufacturer);
+ virBufferAddLit(&buf, ",manufacturer=");
+ virQEMUBuildBufferEscapeComma(&buf, def->manufacturer);
/* 2:Product Name */
- if (def->product)
- virBufferAsprintf(&buf, ",product=%s", def->product);
+ if (def->product) {
+ virBufferAddLit(&buf, ",product=");
+ virQEMUBuildBufferEscapeComma(&buf, def->product);
+ }
/* 2:Version */
- if (def->version)
- virBufferAsprintf(&buf, ",version=%s", def->version);
+ if (def->version) {
+ virBufferAddLit(&buf, ",version=");
+ virQEMUBuildBufferEscapeComma(&buf, def->version);
+ }
/* 2:Serial Number */
- if (def->serial)
- virBufferAsprintf(&buf, ",serial=%s", def->serial);
+ if (def->serial) {
+ virBufferAddLit(&buf, ",serial=");
+ virQEMUBuildBufferEscapeComma(&buf, def->serial);
+ }
/* 2:Asset Tag */
- if (def->asset)
- virBufferAsprintf(&buf, ",asset=%s", def->asset);
+ if (def->asset) {
+ virBufferAddLit(&buf, ",asset=");
+ virQEMUBuildBufferEscapeComma(&buf, def->asset);
+ }
/* 2:Location */
- if (def->location)
- virBufferAsprintf(&buf, ",location=%s", def->location);
+ if (def->location) {
+ virBufferAddLit(&buf, ",location=");
+ virQEMUBuildBufferEscapeComma(&buf, def->location);
+ }
if (virBufferCheckError(&buf) < 0)
goto error;
--
2.10.0
4:18 p.m.
New subject: [libvirt] [PATCH 3/4] qemu: command: escape smbios entry strings
On 10/10/2016 11:51 AM, Peter Krempa wrote:
We pass free-form strings from the users to qemu, thus we need
escape
commas since they are passed to qemu monitor.
Partially resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1373535
---
src/qemu/qemu_command.c | 102 +++++++++++++++++++++++++++++++-----------------
1 file changed, 66 insertions(+), 36 deletions(-)
ACK (oy vei!)
John
10:51 a.m.
New subject: [libvirt] [PATCH 4/4] schema: smbios: allow any strings
The smbios docs allow any string to be passed and libvirt does not
really do any validation on them. Allow passing any string.
Partially resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1373535
---
docs/schemas/domaincommon.rng | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 6eeb4e9..f1609f9 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -4517,9 +4517,7 @@
</define>
<define name="sysinfo-value">
- <data type="string">
- <param name='pattern'>[a-zA-Z0-9/\-_\. \(\)]+</param>
- </data>
+ <data type="string"/>
</define>
<define name="acpiTable">
--
2.10.0
4:23 p.m.
New subject: [libvirt] [PATCH 4/4] schema: smbios: allow any strings
On 10/10/2016 11:51 AM, Peter Krempa wrote:
The smbios docs allow any string to be passed and libvirt does not
really do any validation on them. Allow passing any string.
Partially resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1373535
---
docs/schemas/domaincommon.rng | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
ACK, although couldn't the <ref name="sysinfo-value"/>'s each be
replaced by <text/>, too?
John
diff --git a/docs/schemas/domaincommon.rng
b/docs/schemas/domaincommon.rng
index 6eeb4e9..f1609f9 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -4517,9 +4517,7 @@
</define>
<define name="sysinfo-value">
- <data type="string">
- <param name='pattern'>[a-zA-Z0-9/\-_\. \(\)]+</param>
- </data>
+ <data type="string"/>
</define>
<define name="acpiTable">
9:03 p.m.
New subject: [libvirt] [PATCH 4/4] schema: smbios: allow any strings
On Thu, Oct 13, 2016 at 17:23:33 -0400, John Ferlan wrote:
On 10/10/2016 11:51 AM, Peter Krempa wrote:
> The smbios docs allow any string to be passed and libvirt does not
> really do any validation on them. Allow passing any string.
>
> Partially resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1373535
> ---
> docs/schemas/domaincommon.rng | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
ACK, although couldn't the <ref name="sysinfo-value"/>'s each be
replaced by <text/>, too?
It could probably but I did not want to remove it if we ever revisit it.
On the other hand in such case it will most likely be replaced with
specific entries.
2995
days inactive
2999
days old
10 comments
2 participants
participants (2)
-
John Ferlan -
Peter Krempa