This patch adds virQEMUBuildBufferEscapeComma wherever applicable in src/qemu/qemu_command.c
Based on: https://wiki.libvirt.org/page/BiteSizedTasks#qemu:_Use_comma_escaping_for_more_command_line_values

Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com>
---


This patch is submitted towards my proposal for GSoC'18.

Changes made:
- info->romfile in qemuBuildRomStr
- disk->vendor, disk->product in qemuBuildDriveDevStr
- fs->src->path in qemuBuildFSStr
- fs->dst in qemuBuildFSDevStr
- connect= in qemuBuildHostNetStr
- fileval handling in qemuBuildChrChardevStr
- TYPE_DEV, TYPE_PIPE handling in qemuBuildChrChardevStr
- cfg->vncTLSx509certdir in qemuBuildGraphicsVNCCommandLine
- cfg->spiceTLSx509certdir in qemuBuildGraphicsSPICECommandLine
- loader->path, loader->nvram usage qemuBuildDomainLoaderCommandLine

Places where no changes were made:
- src->hosts->socket in qemuBuildNetworkDriveURI uses virAsprintf not virBufferAsprintf; src->path, src->configFile are not used
- qemuBuildChrArgStr function does not exist
- not applicable on data.nix.path in qemuBuildVhostuserCommandLine
- converting places that use strchr in qemuBuildSmartcardCommandLine to use virBufferEscape

I have run `make check VIR_TEST_EXPENSIVE=1`, `make syntax-check` and `make -C tests valgrind`.
Some tests fail on my system, even for an unmodified clone of the repo.
But, all the tests which were passed by the unmodified clone were passed after I made these changes.

As always, your feedback is welcome!


src/qemu/qemu_command.c | 73 +++++++++++++++++++++++++++++--------------------
 1 file changed, 44 insertions(+), 29 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index fa0aa5d5c..06f4f72fc 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -474,8 +474,10 @@ qemuBuildRomStr(virBufferPtr buf,
         default:
             break;
         }
-        if (info->romfile)
-           virBufferAsprintf(buf, ",romfile=%s", info->romfile);
+        if (info->romfile) {
+            virBufferAddLit(buf, ",romfile=");
+            virQEMUBuildBufferEscapeComma(buf, info->romfile);
+        }
     }
     return 0;
 }
@@ -2177,11 +2179,15 @@ qemuBuildDriveDevStr(const virDomainDef *def,
             virBufferAsprintf(&opt, ",wwn=0x%s", disk->wwn);
     }
 
-    if (disk->vendor)
-        virBufferAsprintf(&opt, ",vendor=%s", disk->vendor);
+    if (disk->vendor) {
+        virBufferAddLit(&opt, ",vendor=");
+        virQEMUBuildBufferEscapeComma(&opt, disk->vendor);
+    }
 
-    if (disk->product)
-        virBufferAsprintf(&opt, ",product=%s", disk->product);
+    if (disk->product) {
+        virBufferAddLit(&opt, ",product=");
+        virQEMUBuildBufferEscapeComma(&opt, disk->product);
+    }
 
     if (disk->bus == VIR_DOMAIN_DISK_BUS_USB) {
         if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_USB_STORAGE_REMOVABLE)) {
@@ -2418,7 +2424,8 @@ qemuBuildFSStr(virDomainFSDefPtr fs,
     }
 
     virBufferAsprintf(&opt, ",id=%s%s", QEMU_FSDEV_HOST_PREFIX, fs->info.alias);
-    virBufferAsprintf(&opt, ",path=%s", fs->src->path);
+    virBufferAddLit(&opt, ",path=");
+    virQEMUBuildBufferEscapeComma(&opt, fs->src->path);
 
     if (fs->readonly) {
         if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_FSDEV_READONLY)) {
@@ -2463,7 +2470,8 @@ qemuBuildFSDevStr(const virDomainDef *def,
     virBufferAsprintf(&opt, ",id=%s", fs->info.alias);
     virBufferAsprintf(&opt, ",fsdev=%s%s",
                       QEMU_FSDEV_HOST_PREFIX, fs->info.alias);
-    virBufferAsprintf(&opt, ",mount_tag=%s", fs->dst);
+    virBufferAddLit(&opt, ",mount_tag=");
+    virQEMUBuildBufferEscapeComma(&opt, fs->dst);
 
     if (qemuBuildVirtioOptionsStr(&opt, fs->virtio, qemuCaps) < 0)
         goto error;
@@ -3603,10 +3611,9 @@ qemuBuildHostNetStr(virDomainNetDefPtr net,
         break;
 
     case VIR_DOMAIN_NET_TYPE_CLIENT:
-        virBufferAsprintf(&buf, "socket%cconnect=%s:%d,",
-                          type_sep,
-                          net->data.socket.address,
-                          net->data.socket.port);
+        virBufferAsprintf(&buf, "socket%cconnect=", type_sep);
+        virQEMUBuildBufferEscapeComma(&buf, net->data.socket.address);
+        virBufferAsprintf(&buf, ":%d,", net->data.socket.port);
         break;
 
     case VIR_DOMAIN_NET_TYPE_SERVER:
@@ -4858,7 +4865,8 @@ qemuBuildChrChardevFileStr(virLogManagerPtr logManager,
         virBufferAsprintf(buf, ",%s=%s,%s=on", filearg, fdpath, appendarg);
         VIR_FREE(fdpath);
     } else {
-        virBufferAsprintf(buf, ",%s=%s", filearg, fileval);
+        virBufferAsprintf(buf, ",%s=", filearg);
+        virQEMUBuildBufferEscapeComma(buf, fileval);
         if (appendval != VIR_TRISTATE_SWITCH_ABSENT) {
             virBufferAsprintf(buf, ",%s=%s", appendarg,
                               virTristateSwitchTypeToString(appendval));
@@ -4916,9 +4924,10 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
         break;
 
     case VIR_DOMAIN_CHR_TYPE_DEV:
-        virBufferAsprintf(&buf, "%s,id=%s,path=%s",
+        virBufferAsprintf(&buf, "%s,id=%s,path=",
                           STRPREFIX(alias, "parallel") ? "parport" : "tty",
-                          charAlias, dev->data.file.path);
+                          charAlias);
+        virQEMUBuildBufferEscapeComma(&buf, dev->data.file.path);
         break;
 
     case VIR_DOMAIN_CHR_TYPE_FILE:
@@ -4938,8 +4947,8 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
         break;
 
     case VIR_DOMAIN_CHR_TYPE_PIPE:
-        virBufferAsprintf(&buf, "pipe,id=%s,path=%s", charAlias,
-                          dev->data.file.path);
+        virBufferAsprintf(&buf, "pipe,id=%s,path=", charAlias);
+        virQEMUBuildBufferEscapeComma(&buf, dev->data.file.path);
         break;
 
     case VIR_DOMAIN_CHR_TYPE_STDIO:
@@ -7829,10 +7838,13 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr cfg,
 
     if (cfg->vncTLS) {
         virBufferAddLit(&opt, ",tls");
-        if (cfg->vncTLSx509verify)
-            virBufferAsprintf(&opt, ",x509verify=%s", cfg->vncTLSx509certdir);
-        else
-            virBufferAsprintf(&opt, ",x509=%s", cfg->vncTLSx509certdir);
+        if (cfg->vncTLSx509verify) {
+            virBufferAddLit(&opt, ",x509verify=");
+            virQEMUBuildBufferEscapeComma(&opt, cfg->vncTLSx509certdir);
+        } else {
+            virBufferAddLit(&opt, ",x509=");
+            virQEMUBuildBufferEscapeComma(&opt, cfg->vncTLSx509certdir);
+        }
     }
 
     if (cfg->vncSASL) {
@@ -7977,8 +7989,11 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
         !cfg->spicePassword)
         virBufferAddLit(&opt, "disable-ticketing,");
 
-    if (hasSecure)
-        virBufferAsprintf(&opt, "x509-dir=%s,", cfg->spiceTLSx509certdir);
+    if (hasSecure) {
+        virBufferAddLit(&opt, "x509-dir=");
+        virQEMUBuildBufferEscapeComma(&opt, cfg->spiceTLSx509certdir);
+        virBufferAddLit(&opt, ",");
+    }
 
     switch (graphics->data.spice.defaultMode) {
     case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_SECURE:
@@ -9463,9 +9478,9 @@ qemuBuildDomainLoaderCommandLine(virCommandPtr cmd,
                                  NULL);
         }
 
-        virBufferAsprintf(&buf,
-                          "file=%s,if=pflash,format=raw,unit=%d",
-                          loader->path, unit);
+        virBufferAddLit(&buf, "file=");
+        virQEMUBuildBufferEscapeComma(&buf, loader->path);
+        virBufferAsprintf(&buf, ",if=pflash,format=raw,unit=%d", unit);
         unit++;
 
         if (loader->readonly) {
@@ -9478,9 +9493,9 @@ qemuBuildDomainLoaderCommandLine(virCommandPtr cmd,
 
         if (loader->nvram) {
             virBufferFreeAndReset(&buf);
-            virBufferAsprintf(&buf,
-                              "file=%s,if=pflash,format=raw,unit=%d",
-                              loader->nvram, unit);
+            virBufferAddLit(&buf, "file=");
+            virQEMUBuildBufferEscapeComma(&buf, loader->nvram);
+            virBufferAsprintf(&buf, ",if=pflash,format=raw,unit=%d", unit);
 
             virCommandAddArg(cmd, "-drive");
             virCommandAddArgBuffer(cmd, &buf);
-- 
2.16.2

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

On 03/16/2018 01:02 PM, Sukrit Bhatnagar wrote:
> This patch adds virQEMUBuildBufferEscapeComma wherever applicable in src/qemu/qemu_command.c
> Based on: https://wiki.libvirt.org/page/BiteSizedTasks#qemu:_Use_comma_escaping_for_more_command_line_values
> 

Try to keep shorter lines in your commit message (generally <=70 ish
characters) and the link to the bite size task should have gone under
the --- below since it's likely that once this task is complete (and
code pushed) that someone will remove the entry from the bite size task
page and we don't really want a knowingly dead link to live in git
history forever.

In any case, consider the following (or something even shorter):

qemu: Add comma escape for more command line options

This patch adds virQEMUBuildBufferEscapeComma to qemu command
line processing for data fields which are user provided in order
to ensure that if a comma (',') is provided that it'll be properly
handled by the qemu monitor which expects a double comma as a form of
escaping.

> Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com>
> ---
> 

BTW: Since this was a followup/adjustment of what you submitted:

https://www.redhat.com/archives/libvir-list/2018-March/msg00940.html

then you should prefix with a "v2" via something like
'--subject-prefix="PATCH v2"' on your 'git send-email' where you can
place a link to the v1 in this section (below the ---) and a description
of what changed or was fixed since v1.

BTW: Your next patch will be v3, so you'll have a shot to try it!
Providing a link to the previous version is generally "nice" so that the
reviewer doesn't have to search for it and it gives a bit more history.

> 
> This patch is submitted towards my proposal for GSoC'18.
> 
> Changes made:
> - info->romfile in qemuBuildRomStr
> - disk->vendor, disk->product in qemuBuildDriveDevStr
> - fs->src->path in qemuBuildFSStr
> - fs->dst in qemuBuildFSDevStr
> - connect= in qemuBuildHostNetStr
> - fileval handling in qemuBuildChrChardevStr
> - TYPE_DEV, TYPE_PIPE handling in qemuBuildChrChardevStr
> - cfg->vncTLSx509certdir in qemuBuildGraphicsVNCCommandLine
> - cfg->spiceTLSx509certdir in qemuBuildGraphicsSPICECommandLine
> - loader->path, loader->nvram usage qemuBuildDomainLoaderCommandLine
> 
> Places where no changes were made:
> - src->hosts->socket in qemuBuildNetworkDriveURI uses virAsprintf not virBufferAsprintf; src->path, src->configFile are not used

Sure; however, you could use virBuffer* API's and then use
virBufferContentAndReset in order to get the escaped string you'd want
for the final result.

I'll admit the src->configFile processing in qemuBuildNetworkDriveStr
appears awful due to the need to escape the ':' for ":conf=", but I
think you can get away with the EscapeComma on just the src->configFile.
There is a test for adding the ":conf=%s", so if you do get it wrong,
you'll know...  If you wanted to be sure the right thing was done with
the comma, then alter the XML file and see that the ARGS output gets the
double comma. Whether that becomes part of the commit isn't necessary,
but at least it proves to yourself that things were done right.

> - qemuBuildChrArgStr function does not exist

hint...

git log -p src/qemu/qemu_command.c
<search for qemuBuildChrArgStr>

and you'd see that processing was moved by commit id '426dc5eb' into
qemuBuildChrChardevStr and qemuBuildChrChardevFileStr which were handled
in this patch.

> - not applicable on data.nix.path in qemuBuildVhostuserCommandLine

True, since it's not built into the qemu command line but rather as part
of some other command...

> - converting places that use strchr in qemuBuildSmartcardCommandLine to use virBufferEscape

Perhaps the request here was to remove the check and failure for "," in
the name/path and use EscapeComma instead...

Another one that wasn't listed is "rendernode" - although nothing tells
us "how" that field is populated in the XML, I suppose someone could add
a "," into the provided entry and that'd be problematic.

Also "throttling.group" and "group_name" - although that's already
escaped, someone conceivably could put in a "," into the name too as
it's not checked.

FWIW: After applying your patch I did a:

grep "=%s" src/qemu/qemu_command.c | \
    grep -v "bus=%s" | grep -v "fd=%s" | grep -v "id=%s"

and that reduced the number things to specifically look at for needing
the comma escaping.  You may want to make the same check - I didn't
complete looking at the output...

> 
> I have run `make check VIR_TEST_EXPENSIVE=1`, `make syntax-check` and `make -C tests valgrind`.
> Some tests fail on my system, even for an unmodified clone of the repo.
> But, all the tests which were passed by the unmodified clone were passed after I made these changes.
> 
> As always, your feedback is welcome!
> 
> 
> src/qemu/qemu_command.c | 73 +++++++++++++++++++++++++++++--------------------
>  1 file changed, 44 insertions(+), 29 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index fa0aa5d5c..06f4f72fc 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c

[...]

> @@ -3603,10 +3611,9 @@ qemuBuildHostNetStr(virDomainNetDefPtr net,
>          break;
>  
>      case VIR_DOMAIN_NET_TYPE_CLIENT:
> -        virBufferAsprintf(&buf, "socket%cconnect=%s:%d,",
> -                          type_sep,
> -                          net->data.socket.address,
> -                          net->data.socket.port);
> +        virBufferAsprintf(&buf, "socket%cconnect=", type_sep);
> +        virQEMUBuildBufferEscapeComma(&buf, net->data.socket.address);
> +        virBufferAsprintf(&buf, ":%d,", net->data.socket.port);
>          break;
>  
>      case VIR_DOMAIN_NET_TYPE_SERVER:

Below here there's a few more "%s" for net->data.socket.address that
weren't addressed in different case's (SERVER, MCAST, UDP)

> @@ -4858,7 +4865,8 @@ qemuBuildChrChardevFileStr(virLogManagerPtr logManager,

[...]

The rest seemed fine to my eyes.

John

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

On Thu, Mar 29, 2018 at 12:10:46PM -0400, John Ferlan wrote:
>
>
>On 03/16/2018 01:02 PM, Sukrit Bhatnagar wrote:
>> This patch adds virQEMUBuildBufferEscapeComma wherever applicable in src/qemu/qemu_command.c
>> Based on: https://wiki.libvirt.org/page/BiteSizedTasks#qemu:_Use_comma_escaping_for_more_command_line_values
>>
>
>Try to keep shorter lines in your commit message (generally <=70 ish
>characters) and the link to the bite size task should have gone under
>the --- below since it's likely that once this task is complete (and
>code pushed) that someone will remove the entry from the bite size task
>page and we don't really want a knowingly dead link to live in git
>history forever.
>
>In any case, consider the following (or something even shorter):
>
>qemu: Add comma escape for more command line options
>
>This patch adds virQEMUBuildBufferEscapeComma to qemu command
>line processing for data fields which are user provided in order
>to ensure that if a comma (',') is provided that it'll be properly
>handled by the qemu monitor which expects a double comma as a form of
>escaping.
>

Yeah, this sounds nice, although I must admit I'm usually very lazy to explain
what's all the fuzz behind it when the difference between the long explanation
and just "Use function X more" is basically just the description of function X.
And since that function already exists and is not being added by the patch...
You know where I'm going with it =) Anyway, that's not why I'm replying here.  I
just want to give some hints further down.

>> Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com>
>> ---
>>
>
>BTW: Since this was a followup/adjustment of what you submitted:
>
>https://www.redhat.com/archives/libvir-list/2018-March/msg00940.html
>
>then you should prefix with a "v2" via something like
>'--subject-prefix="PATCH v2"' on your 'git send-email' where you can
>place a link to the v1 in this section (below the ---) and a description
>of what changed or was fixed since v1.
>

Even better, format-patch and send-email both accept `-v` parameter, so you can
just do `git send-email -v2` and it will do it properly for you, even if you
have some subject-prefix already set (e.g. for a subproject)

>BTW: Your next patch will be v3, so you'll have a shot to try it!
>Providing a link to the previous version is generally "nice" so that the
>reviewer doesn't have to search for it and it gives a bit more history.
>
>>
>> This patch is submitted towards my proposal for GSoC'18.
>>
>> Changes made:
>> - info->romfile in qemuBuildRomStr
>> - disk->vendor, disk->product in qemuBuildDriveDevStr
>> - fs->src->path in qemuBuildFSStr
>> - fs->dst in qemuBuildFSDevStr
>> - connect= in qemuBuildHostNetStr
>> - fileval handling in qemuBuildChrChardevStr
>> - TYPE_DEV, TYPE_PIPE handling in qemuBuildChrChardevStr
>> - cfg->vncTLSx509certdir in qemuBuildGraphicsVNCCommandLine
>> - cfg->spiceTLSx509certdir in qemuBuildGraphicsSPICECommandLine
>> - loader->path, loader->nvram usage qemuBuildDomainLoaderCommandLine
>>
>> Places where no changes were made:
>> - src->hosts->socket in qemuBuildNetworkDriveURI uses virAsprintf not virBufferAsprintf; src->path, src->configFile are not used
>
>Sure; however, you could use virBuffer* API's and then use
>virBufferContentAndReset in order to get the escaped string you'd want
>for the final result.
>
>I'll admit the src->configFile processing in qemuBuildNetworkDriveStr
>appears awful due to the need to escape the ':' for ":conf=", but I
>think you can get away with the EscapeComma on just the src->configFile.
>There is a test for adding the ":conf=%s", so if you do get it wrong,
>you'll know...  If you wanted to be sure the right thing was done with
>the comma, then alter the XML file and see that the ARGS output gets the
>double comma. Whether that becomes part of the commit isn't necessary,
>but at least it proves to yourself that things were done right.
>
>> - qemuBuildChrArgStr function does not exist
>
>hint...
>
>git log -p src/qemu/qemu_command.c
><search for qemuBuildChrArgStr>
>

Again, just a hint, you could do `git log -p -G qemuBuildChrArgStr`.  That will
only show you commits that had the string after -G in one of the added/removed
lines.

>and you'd see that processing was moved by commit id '426dc5eb' into
>qemuBuildChrChardevStr and qemuBuildChrChardevFileStr which were handled
>in this patch.
>
>> - not applicable on data.nix.path in qemuBuildVhostuserCommandLine
>
>True, since it's not built into the qemu command line but rather as part
>of some other command...
>
>> - converting places that use strchr in qemuBuildSmartcardCommandLine to use virBufferEscape
>
>Perhaps the request here was to remove the check and failure for "," in
>the name/path and use EscapeComma instead...
>
>Another one that wasn't listed is "rendernode" - although nothing tells
>us "how" that field is populated in the XML, I suppose someone could add
>a "," into the provided entry and that'd be problematic.
>
>Also "throttling.group" and "group_name" - although that's already
>escaped, someone conceivably could put in a "," into the name too as
>it's not checked.
>
>FWIW: After applying your patch I did a:
>
>grep "=%s" src/qemu/qemu_command.c | \
>    grep -v "bus=%s" | grep -v "fd=%s" | grep -v "id=%s"
>
>and that reduced the number things to specifically look at for needing
>the comma escaping.  You may want to make the same check - I didn't
>complete looking at the output...
>
>>
>> I have run `make check VIR_TEST_EXPENSIVE=1`, `make syntax-check` and `make -C tests valgrind`.
>> Some tests fail on my system, even for an unmodified clone of the repo.
>> But, all the tests which were passed by the unmodified clone were passed after I made these changes.
>>

VIR_TEST_EXPENSIVE is fine, but you probably won't be changing stuff related to
that.  The valgrind tests are kind of broken sometimes, don't worry about it.

Hope that helped, looking forward to v3 ;)

>> As always, your feedback is welcome!
>>
>>
>> src/qemu/qemu_command.c | 73 +++++++++++++++++++++++++++++--------------------
>>  1 file changed, 44 insertions(+), 29 deletions(-)
>>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index fa0aa5d5c..06f4f72fc 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>
>[...]
>
>> @@ -3603,10 +3611,9 @@ qemuBuildHostNetStr(virDomainNetDefPtr net,
>>          break;
>>
>>      case VIR_DOMAIN_NET_TYPE_CLIENT:
>> -        virBufferAsprintf(&buf, "socket%cconnect=%s:%d,",
>> -                          type_sep,
>> -                          net->data.socket.address,
>> -                          net->data.socket.port);
>> +        virBufferAsprintf(&buf, "socket%cconnect=", type_sep);
>> +        virQEMUBuildBufferEscapeComma(&buf, net->data.socket.address);
>> +        virBufferAsprintf(&buf, ":%d,", net->data.socket.port);
>>          break;
>>
>>      case VIR_DOMAIN_NET_TYPE_SERVER:
>
>Below here there's a few more "%s" for net->data.socket.address that
>weren't addressed in different case's (SERVER, MCAST, UDP)
>
>> @@ -4858,7 +4865,8 @@ qemuBuildChrChardevFileStr(virLogManagerPtr logManager,
>
>[...]
>
>The rest seemed fine to my eyes.
>
>John
>
>--
>libvir-list mailing list
>libvir-list@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list