Incremental patches do look better. Just to make sure I am on the right track, I have some queries.
I have to apply the changes one function at a time, and these changes will be the same ones I made in the v2 and v3 patches, right? If that is the case, do I need the next patch to be v4 or can the series of patches start from v1? I can start afresh with the patches and this might save some confusion later. Thanks. On Wed, Apr 11, 2018 at 3:52 AM, John Ferlan <jfer...@redhat.com> wrote: > > > 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 <skrtbht...@gmail.com> > > --- > > > > 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 >
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list