On 04/02/2018 04:17 PM, Sukrit Bhatnagar wrote: > This patch adds virQEMUBuildBufferEscapeComma to properly > escape commas in user provided data fields for qemu command > line processing. > > Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com&gt; > --- > > Thank you for the helpful feedback and apologies for the delay. Well we all get busy and delayed by other things! It's been a week since you posted and well, I know I'm behind doing reviews! Nice on the using the --- for your adjustments and the issue you discovered. What happened to the changes from your previous version? They don't seem to be included in this patch and they weren't pushed upstream. This patch is all new changes. The "next" step is to attempt to generate patches that make incremental progress towards the end goal. Not all your changes need to go in one pile especially if something is separable - there's a methodology one develops over time. Changes don't need to be "so separated" that you get very large series, but you can create smaller patches, altering single API's/helpers and adjusting anything called by them to handle the changes. Some times it's a changed result and other times it's doing nothing because the patch fixes something. Again, it's one of those over time things. In this case, almost every function could have had it's own patch. That way a reviewer can ACK/Reviewed-by and push part of the series while perhaps asking for respins on other parts. It also allows for a NACK of a specific area. Much easier to change and review smaller things too! Since this is a GSOC type activity I won't "do" the work for you, but I will help "guide" you to the answer. First things first - hopefully you haven't lost your original patch. It's easily recreateable at least. We will start easy/slow... Using that patch as a starting point, create a series of 5 patches patch 1: Changes for qemuBuildRomStr patch 2: Changes for qemuBuildDriveDevStr patch 3: Changes for qemuBuildFSStr and qemuBuildFSDevStr patch 4: Changes for qemuBuildGraphicsVNCCommandLine patch 5: Changes for qemuBuildDomainLoaderCommandLine Hold onto the changes for qemuBuildHostNetStr, qemuBuildChrChardevFileStr, qemuBuildChrChardevStr, and qemuBuildGraphicsSPICECommandLine as they'll be combined with separated patches from this patch. Post the above 5 - I've included patch 1 & 2 for you as an attachment to this reply so you can see the format... It should be fairly easy to copy from there and post as a v4. Once you've done that - work through the rest of this using similar context - although a few of these will be a bit larger and more complicated (especially the first one for build network drive. > > Changes in v3: > virQEMUBuildBufferEscapeComma was applied to: > - src->hosts->socket in qemuBuildNetworkDriveURI > - src->path, src->configFile in qemuBuildNetworkDriveStr > - disk->blkdeviotune.group_name in qemuBuildDiskThrottling > - net->data.socket.address, net->data.socket.localaddr in > qemuBuildHostNetStr > - dev->data.file.path in qemuBuildChrChardevStr > - graphics->data.spice.rendernode in > qemuBuildGraphicsSPICECommandLine > - smartcard->data.cert.file[i], smartcard->data.cert.database in > qemuBuildSmartcardCommandLine > > Changes in v2: > virQEMUBuildBufferEscapeComma was applied to: > - 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 in > qemuBuildDomainLoaderCommandLine > > Link to v2: https://www.redhat.com/archives/libvir-list/2018- March/msg00965.html > > > When I tried to change src->path in qemuBuildNetworkDriveStr > for this portion > > 961 } else if (src->nhosts == 1) { > 962 if (virAsprintf(&ret, "sheepdog:%s:%u:%s", > 963 src->hosts->name, src->hosts->port, > 964 src->path) < 0) > 965 goto cleanup; > 966 } else { > > make check reported the following error. > > 141) QEMU XML-2-ARGV disk-drive-network-sheepdog ... > In '/home/skrtbhtngr/libvirt/tests/qemuxml2argvdata/disk- drive-network-sheepdog.args': > Offset 0 > Expect [LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none /usr/bin/qemu-system-i686 -name QEMUGuest1 -S -M pc -m 214 -smp 1,sockets=1,cores=1,threads=1 -uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 -nographic -nodefaults -chardev socket,id=charmonitor,path=/ tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait -mon chardev=charmonitor,id=monitor,mode=readline -no-acpi -boot c -usb -drive file=/dev/HostVG/QEMU,,Guest,,,,1,format=raw,if=none,id=drive-ide0-0-0 -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 -drive file=sheepdog:example.org:6000:image,,with,,commas,format= raw,if=none,id=drive-virtio-disk0 -device virtio-blk-pci,bus=pci.0,addr= 0x3,drive=drive-virtio-disk0,id=virtio-disk0 > ] > Actual [LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none /usr/bin/qemu-system-i686 -name QEMUGuest1 -S -M pc -m 214 -smp 1,sockets=1,cores=1,threads=1 -uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 -nographic -nodefaults -chardev socket,id=charmonitor,path=/ tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait -mon chardev=charmonitor,id=monitor,mode=readline -no-acpi -boot c -usb -drive file=/dev/HostVG/QEMU,,Guest,,,,1,format=raw,if=none,id=drive-ide0-0-0 -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 -drive file=sheepdog:example.org:6000:image,,,,with,,,,commas, format=raw,if=none,id=drive-virtio-disk0 -device virtio-blk-pci,bus=pci.0,addr=0x3,drive=drive-virtio-disk0,id=virtio-disk0 > ] > ... FAILED > > > In disk-drive-network-sheepdog.args: > ... > -drive file=sheepdog:example.org:6000:image,,with,,commas,format= raw,if=none,\ > ... > > I was not quite sure how to handle this part. Adding > virQEMUBuildBufferEscapeComma there is escaping the twin commas > in the file name again. I have left that part unchanged. > This is because qemuBuildDriveSourceStr calls qemuGetDriveSourceString which calls qemuBuildNetworkDriveStr returning @source. Then back in qemuBuildDriveSourceStr the @source goes through another transformation: if (source) { virBufferAddLit(buf, "file="); ... virQEMUBuildBufferEscapeComma(buf, source); ... Because the return from qemuBuildNetworkDriveStr is used by callers that don't expect qemuGetDriveSourceString to return a comma escaped string that means adding a bit of logic so that qemuBuildNetworkDriveURI and qemuBuildNetworkDriveStr can escape only when necessary. Returning an escaped string for qemuDomainSnapshotCreateSingleDiskActive would not be a good thing. Still it's a *good thing* you've gone through this exercise *and* made note of what you saw. It makes it clearer what the "right" path is for me at least. Of course if you'd separated out your patches it would make resolving it a bit easier! Also, this exercise has shown there's a bug in how the command line is built for hostdev's. The source is not escaped, although I doubt we'd get an iSCSI tgt/lun with a "rogue" comma - it's just not expected. Still who knows, we still need to handle it. > > src/qemu/qemu_command.c | 111 +++++++++++++++++++++++++----- ------------------ > 1 file changed, 59 insertions(+), 52 deletions(-) > > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index 6f76f18ab..26b36551c 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -844,14 +844,18 @@ qemuBuildNetworkDriveURI(virStorageSourcePtr src, > qemuDomainSecretInfoPtr secinfo) I think a third parameter "bool escape" will be necessary... > { > virURIPtr uri = NULL; > - char *ret = NULL; > + char *ret = NULL, *socket = NULL; There is a general preference for one argument per line. > + virBuffer buf = VIR_BUFFER_INITIALIZER; > > if (!(uri = qemuBlockStorageSourceGetURI(src))) > goto cleanup; > > - if (src->hosts->socket && > - virAsprintf(&uri->query, "socket=%s", src->hosts->socket) < 0) > - goto cleanup; > + if (src->hosts->socket) { > + virQEMUBuildBufferEscapeComma(&buf, src->hosts->socket); > + socket = virBufferContentAndReset(&buf); > + if (virAsprintf(&uri->query, "socket=%s", socket) < 0) > + goto cleanup; > + } This logic needs to be separated into "if (escape)" escape the socket, e.g.: if (src->hosts->socket) { if (escape) { virQEMUBuildBufferEscapeComma(&buf, src->hosts->socket); socket = virBufferContentAndReset(&buf); } if (virAsprintf(&uri->query, "socket=%s", socket ? socket : src->hosts->socket) < 0) goto cleanup; } > > if (qemuBuildGeneralSecinfoURI(uri, secinfo) < 0) > goto cleanup; > @@ -860,6 +864,8 @@ qemuBuildNetworkDriveURI(virStorageSourcePtr src, > > cleanup: > virURIFree(uri); > + virBufferFreeAndReset(&buf); > + The virBufferContentAndReset will reinitialize the buffer, so no need for this call, but we would leak @socket possibly, so that does need to be freed.... Also, need for extra blank line here. This then is just: VIR_FREE(socket); > return ret; > } > > @@ -868,8 +874,9 @@ static char * > qemuBuildNetworkDriveStr(virStorageSourcePtr src, > qemuDomainSecretInfoPtr secinfo) I think a third parameter "bool escape" will be necessary... > { > - char *ret = NULL; > + char *ret = NULL, *path = NULL, *file = NULL; again, one line and we should only need one, e.g. 'tmp' - since we know it's initialized to NULL we can use that when deciding whether we escape or not. > virBuffer buf = VIR_BUFFER_INITIALIZER; > + virBuffer bufTemp = VIR_BUFFER_INITIALIZER; > size_t i; > > switch ((virStorageNetProtocol) src->protocol) { > @@ -914,8 +921,10 @@ qemuBuildNetworkDriveStr(virStorageSourcePtr src, > goto cleanup; > } > > - if (src->path) > - virBufferAsprintf(&buf, ":exportname=%s", src->path); > + if (src->path) { > + virBufferAddLit(&buf, ":exportname="); > + virQEMUBuildBufferEscapeComma(&buf, src->path); > + } if (src->path) { if (escape) { virBufferAddLit(&buf, ":exportname="); virQEMUBuildBufferEscapeComma(&buf, src->path); } else { virBufferAsprintf(&buf, ":exportname=%s", src->path); } } > > if (virBufferCheckError(&buf) < 0) > goto cleanup; > @@ -945,7 +954,9 @@ qemuBuildNetworkDriveStr(virStorageSourcePtr src, > } > > if (src->nhosts == 0) { > - if (virAsprintf(&ret, "sheepdog:%s", src->path) < 0) > + virQEMUBuildBufferEscapeComma(&bufTemp, src->path); > + path = virBufferContentAndReset(&bufTemp); > + if (virAsprintf(&ret, "sheepdog:%s", path) < 0) > goto cleanup; > } else if (src->nhosts == 1) { > if (virAsprintf(&ret, "sheepdog:%s:%u:%s", if (escape) { virQEMUBuildBufferEscapeComma(&bufTemp, src->path); tmp = virBufferContentAndReset(&bufTemp); } if (src->nhosts == 0) { if (virAsprintf(&ret, "sheepdog:%s", tmp ? tmp : src->path) < 0) goto cleanup; } else if (src->nhosts == 1) { if (virAsprintf(&ret, "sheepdog:%s:%u:%s", src->hosts->name, src->hosts->port, tmp ? tmp : src->path) < 0) goto cleanup; } else { NB: In your patch - if the .xml/.args file hadn't used a host w/ a path having a comma, then that would have failed. IOW: if tests/qemuxml2argdata/disk-drive-network-sheepdog.xml: <disk type='network' device='disk'> <driver name='qemu' type='raw'/> <source protocol='sheepdog' name='image,with,commas'> <host name='example.org' port='6000'/> </source> <target dev='vda' bus='virtio'/> </disk> was: <disk type='network' device='disk'> <driver name='qemu' type='raw'/> <source protocol='sheepdog' name='image,with,commas'/> <target dev='vda' bus='virtio'/> </disk> then tests/qemuxml2argvdata/disk-drive-network-sheepdog.args would change from: -drive file=sheepdog:example.org:6000:image,,with,,commas,format=raw,if=none,\ id=drive-virtio-disk0 \ to (something like): -drive file=sheepdog:image,,with,,commas,format=raw,if=none,\ id=drive-virtio-disk0 \ But with your change it would have had the 4 commas (hopefully this makes sense). > @@ -967,8 +978,9 @@ qemuBuildNetworkDriveStr(virStorageSourcePtr src, > src->path); > goto cleanup; > } > - > - virBufferStrcat(&buf, "rbd:", src->volume, "/", src->path, NULL); > + virQEMUBuildBufferEscapeComma(&bufTemp, src->path); > + path = virBufferContentAndReset(&bufTemp); > + virBufferStrcat(&buf, "rbd:", src->volume, "/", path, NULL); if (escape) { virQEMUBuildBufferEscapeComma(&bufTemp, src->path); tmp = virBufferContentAndReset(&bufTemp); } virBufferStrcat(&buf, "rbd:", src->volume, "/", tmp ? tmp : src->path, NULL); VIR_FREE(tmp); > > if (src->snapshot) > virBufferEscape(&buf, '\\', ":", "@%s", src->snapshot); > @@ -994,8 +1006,11 @@ qemuBuildNetworkDriveStr(virStorageSourcePtr src, > } > } > > - if (src->configFile) > - virBufferEscape(&buf, '\\', ":", ":conf=%s", src->configFile); > + if (src->configFile) { > + virQEMUBuildBufferEscapeComma(&bufTemp, src->configFile); > + file = virBufferContentAndReset(&bufTemp); > + virBufferEscape(&buf, '\\', ":", ":conf=%s", file); > + } if (src->configFile) { if (escape) { virQEMUBuildBufferEscapeComma(&bufTemp, src->configFile); tmp = virBufferContentAndReset(&bufTemp); } virBufferEscape(&buf, '\\', ":", ":conf=%s", tmp ? tmp : src->configFile); } > > if (virBufferCheckError(&buf) < 0) > goto cleanup; > @@ -1022,6 +1037,7 @@ qemuBuildNetworkDriveStr(virStorageSourcePtr src, > } > > cleanup: > + virBufferFreeAndReset(&bufTemp); Again, each of the callers used virBufferContentAndReset, so the @path and @file arguments would have been needed to be VIR_FREE'd instead. However, if you take my suggestion w/ a tmp variable, then you just VIR_FREE(tmp) instead. > virBufferFreeAndReset(&buf); > > return ret; When you post your next patch - use this opportunity to separate out these two functions into their own patch (along with adjustments to callers to pass the escape bool. This will be the most complex patch out of them all. > @@ -1630,6 +1646,8 @@ qemuBuildDiskThrottling(virDomainDiskDefPtr disk, > virBufferAsprintf(buf, ",throttling." _label "=%llu", \ > disk->blkdeviotune._field); \ > } > + virBuffer bufTemp = VIR_BUFFER_INITIALIZER; > + char *name = NULL; These can move to ... > > IOTUNE_ADD(total_bytes_sec, "bps-total"); > IOTUNE_ADD(read_bytes_sec, "bps-read"); > @@ -1647,8 +1665,9 @@ qemuBuildDiskThrottling(virDomainDiskDefPtr disk, > > IOTUNE_ADD(size_iops_sec, "iops-size"); > if (disk->blkdeviotune.group_name) { ... here (IOW: localize them more). > - virBufferEscapeString(buf, ",throttling.group=%s", > - disk->blkdeviotune.group_name); > + virQEMUBuildBufferEscapeComma(&bufTemp, disk->blkdeviotune.group_name); > + name = virBufferContentAndReset(&bufTemp); > + virBufferEscapeString(buf, ",throttling.group=%s", name); VIR_FREE(name); > } > > IOTUNE_ADD(total_bytes_sec_max_length, "bps-total-max-length"); > @@ -1657,6 +1676,8 @@ qemuBuildDiskThrottling(virDomainDiskDefPtr disk, > IOTUNE_ADD(total_iops_sec_max_length, "iops-total-max-length"); > IOTUNE_ADD(read_iops_sec_max_length, "iops-read-max-length"); > IOTUNE_ADD(write_iops_sec_max_length, "iops-write-max-length"); > + > + virBufferFreeAndReset(&bufTemp); NB: virBufferContentAndReset will be all you need for bufTemp, but @name is leaked, but that's adjusted above > #undef IOTUNE_ADD > } > The changes for qemuBuildDiskThrottling should be in one patch. > @@ -3651,27 +3672,25 @@ qemuBuildHostNetStr(virDomainNetDefPtr net, > break; > > case VIR_DOMAIN_NET_TYPE_SERVER: > - virBufferAsprintf(&buf, "socket%clisten=%s:%d,", > - type_sep, > + virBufferAsprintf(&buf, "socket%clisten=", type_sep); > + virQEMUBuildBufferEscapeComma(&buf, > net->data.socket.address ? net->data.socket.address > - : "", > - net->data.socket.port); > + : ""); This has alignment issues w/ the previous line. It also points out something I'm not sure whether it's a bug or not. If net->data.socket.address == NULL, then the command line would be (from net-vhostuser.args): -netdev socket,listen=:2015, But that looks strange to me, I guess I'd expect: -netdev socket,listen="":2015, Still the former syntax works so I suppose it's OK. Still changes could be rewritten as: virBufferAsprintf(&buf, "socket%clisten=", type_sep); virQEMUBuildBufferEscapeComma(&buf, net->data.socket.address); virBufferAsprintf(&buf, ":%d,", net->data.socket.port); BTW: Your previous patch added a couple of changes to this API - they should be moved into here so that we have all the adjustments to qemuBuildHostNetStr in one patch. > + virBufferAsprintf(&buf, ":%d,", net->data.socket.port); > break; > > case VIR_DOMAIN_NET_TYPE_MCAST: > - virBufferAsprintf(&buf, "socket%cmcast=%s:%d,", > - type_sep, > - net->data.socket.address, > - net->data.socket.port); > + virBufferAsprintf(&buf, "socket%cmcast=", type_sep); > + virQEMUBuildBufferEscapeComma(&buf, net->data.socket.address); > + virBufferAsprintf(&buf, ":%d,", net->data.socket.port); > break; > > case VIR_DOMAIN_NET_TYPE_UDP: > - virBufferAsprintf(&buf, "socket%cudp=%s:%d,localaddr=%s:%d,", > - type_sep, > - net->data.socket.address, > - net->data.socket.port, > - net->data.socket.localaddr, > - net->data.socket.localport); > + virBufferAsprintf(&buf, "socket%cudp=", type_sep); > + virQEMUBuildBufferEscapeComma(&buf, net->data.socket.address); > + virBufferAsprintf(&buf, ":%d,localaddr=", net->data.socket.port); > + virQEMUBuildBufferEscapeComma(&buf, net->data.socket.localaddr); > + virBufferAsprintf(&buf, ":%d,", net->data.socket.localport); > break; > > case VIR_DOMAIN_NET_TYPE_USER: And like noted before - the above hunk can be it's own patch! > @@ -4954,9 +4973,10 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, > bool chardevStdioLogd) > { > virBuffer buf = VIR_BUFFER_INITIALIZER; > + virBuffer bufTemp = VIR_BUFFER_INITIALIZER; > bool telnet; > char *charAlias = NULL; > - char *ret = NULL; > + char *ret = NULL, *path = NULL; One line per argument... > > if (!(charAlias = qemuAliasChardevFromDevAlias(alias))) > goto cleanup; > @@ -4990,9 +5010,11 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, > _("append not supported in this QEMU binary")); > goto cleanup; > } > + virQEMUBuildBufferEscapeComma(&bufTemp, dev->data.file.path); > + path = virBufferContentAndReset(&bufTemp); > if (qemuBuildChrChardevFileStr(chardevStdioLogd ? logManager : NULL, > cmd, def, &buf, > - "path", dev->data.file.path, > + "path", path, > "append", dev->data.file.append) < 0) path is leaked. > goto cleanup; > break; And the above is in it's own patch. Here again, I'd combine the changes from your original patch to qemuBuildChrChardevStr into this one. I'd also include the changes for qemuBuildChrChardevFileStr within the same patch since they are "related". > @@ -8150,8 +8172,8 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, > _("This QEMU doesn't support spice OpenGL rendernode")); > goto error; > } > - > - virBufferAsprintf(&opt, "rendernode=%s,", graphics->data.spice.rendernode); > + virBufferAddLit(&opt, "rendernode="); > + virQEMUBuildBufferEscapeComma(&opt, graphics->data.spice. rendernode); > } > } > The above can be it's own patch and of course include the SPICE change from your original patch too. > @@ -8771,7 +8793,6 @@ qemuBuildSmartcardCommandLine(virLogManagerPtr logManager, > virDomainSmartcardDefPtr smartcard; > char *devstr; > virBuffer opt = VIR_BUFFER_INITIALIZER; > - const char *database; > const char *contAlias = NULL; > > if (!def->nsmartcards) > @@ -8814,29 +8835,15 @@ qemuBuildSmartcardCommandLine(virLogManagerPtr logManager, > > virBufferAddLit(&opt, "ccid-card-emulated,backend= certificates"); > for (i = 0; i < VIR_DOMAIN_SMARTCARD_NUM_CERTIFICATES; i++) { > - if (strchr(smartcard->data.cert.file[i], ',')) { > - virBufferFreeAndReset(&opt); > - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > - _("invalid certificate name: %s"), > - smartcard->data.cert.file[i]); > - return -1; > - } > - virBufferAsprintf(&opt, ",cert%zu=%s", i + 1, > - smartcard->data.cert.file[i]); > + virBufferAsprintf(&opt, ",cert%zu=", i + 1); > + virQEMUBuildBufferEscapeComma(&opt, smartcard->data.cert.file[i]); > } > if (smartcard->data.cert.database) { > - if (strchr(smartcard->data.cert.database, ',')) { > - virBufferFreeAndReset(&opt); > - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > - _("invalid database name: %s"), > - smartcard->data.cert.database); > - return -1; > - } > - database = smartcard->data.cert.database; > + virBufferAddLit(&opt, ",db="); > + virQEMUBuildBufferEscapeComma(&opt, smartcard->data.cert.database); > } else { > - database = VIR_DOMAIN_SMARTCARD_DEFAULT_DATABASE; > + virBufferAsprintf(&opt, ",db=%s", VIR_DOMAIN_SMARTCARD_DEFAULT_DATABASE); > } > - virBufferAsprintf(&opt, ",db=%s", database); > break; > > case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH: > And this one gets it's own patch too. In the end, probably 11 patches total. Do the easy ones first. It's always good to make some progress and have some success rather than having to redo everything all over again and have this large pile of a singlular change waiting for some part of it to be fixed. Once everything is done we can remove the entry from bite sized tasks. John