The code for creating external snapshots for an offline domain
called out to qemu-img without escaping commas in the manner
that qemu-img expects. This also fixes a typo in the comment.

Signed-off-by: Eric Blake <eblake@redhat.com>
---

Noticed by code inspection; I did not try very hard to see how
easy or hard it would be to convince libvirt to actually try
and create an external snapshot to /path/file,with,comma to
see how things break.

 src/qemu/qemu_driver.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index dc51de0310..971f915619 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -100,6 +100,7 @@
 #include "virnuma.h"
 #include "dirname.h"
 #include "netdev_bandwidth_conf.h"
+#include "virqemu.h"

 #define VIR_FROM_THIS VIR_FROM_QEMU

@@ -14561,6 +14562,7 @@ qemuDomainSnapshotCreateInactiveExternal(virQEMUDriverPtr driver,
     virBitmapPtr created = NULL;
     virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
     int ret = -1;
+    virBuffer buf = VIR_BUFFER_INITIALIZER;

     if (!(qemuImgPath = qemuFindQemuImgBinary(driver)))
         goto cleanup;
@@ -14589,13 +14591,15 @@ qemuDomainSnapshotCreateInactiveExternal(virQEMUDriverPtr driver,
                                          NULL)))
             goto cleanup;

-        /* adds cmd line arg: backing_file=/path/to/backing/file,backing_fmd=format */
-        virCommandAddArgFormat(cmd, "backing_file=%s,backing_fmt=%s",
-                               defdisk->src->path,
-                               virStorageFileFormatTypeToString(defdisk->src->format));
+        /* adds cmd line arg: backing_fmt=format,backing_file=/path/to/backing/file */
+        virBufferAsprintf(&buf, "backing_fmt=%s,backing_file=",
+                          virStorageFileFormatTypeToString(defdisk->src->format));
+        virQEMUBuildBufferEscapeComma(&buf, defdisk->src->path);
+        virCommandAddArgBuffer(cmd, &buf);

         /* adds cmd line args: /path/to/target/file */
-        virCommandAddArg(cmd, snapdisk->src->path);
+        virQEMUBuildBufferEscapeComma(&buf, snapdisk->src->path);
+        virCommandAddArgBuffer(cmd, &buf);

         /* If the target does not exist, we're going to create it possibly */
         if (!virFileExists(snapdisk->src->path))
@@ -14629,6 +14633,7 @@ qemuDomainSnapshotCreateInactiveExternal(virQEMUDriverPtr driver,
     ret = 0;

  cleanup:
+    virBufferFreeAndReset(&buf);
     virCommandFree(cmd);

     /* unlink images if creation has failed */
-- 
2.20.1

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

On 2/12/19 11:33 PM, Eric Blake wrote:
> The code for creating external snapshots for an offline domain
> called out to qemu-img without escaping commas in the manner
> that qemu-img expects. This also fixes a typo in the comment.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> 
> Noticed by code inspection; I did not try very hard to see how
> easy or hard it would be to convince libvirt to actually try
> and create an external snapshot to /path/file,with,comma to
> see how things break.
> 
>  src/qemu/qemu_driver.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 

Oh joy another place that uses qemu-img...

Looks like qemuDomainSnapshotForEachQcow2Raw uses virDomainDiskGetSource
or disk def->src->path as well for qemuimgarg[4].

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

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

On 2/13/19 3:58 PM, John Ferlan wrote:
> 
> 
> On 2/12/19 11:33 PM, Eric Blake wrote:
>> The code for creating external snapshots for an offline domain
>> called out to qemu-img without escaping commas in the manner
>> that qemu-img expects. This also fixes a typo in the comment.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>>
>> Noticed by code inspection; I did not try very hard to see how
>> easy or hard it would be to convince libvirt to actually try
>> and create an external snapshot to /path/file,with,comma to
>> see how things break.
>>
>>  src/qemu/qemu_driver.c | 15 ++++++++++-----
>>  1 file changed, 10 insertions(+), 5 deletions(-)
>>
> 
> Oh joy another place that uses qemu-img...
> 
> Looks like qemuDomainSnapshotForEachQcow2Raw uses virDomainDiskGetSource
> or disk def->src->path as well for qemuimgarg[4].

Urgh, so it does. And it uses virRun() instead of the nicer
virCommand... API :( I'll save that for a separate patch,

> 
> Reviewed-by: John Ferlan <jferlan@redhat.com>

and have pushed this one.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

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