[libvirt] [PATCH v3] qemu: add virQEMUBuildBufferEscapeComma in qemu_command.c
Sukrit Bhatnagar
skrtbhtngr at gmail.com
Thu Apr 12 21:40:04 UTC 2018
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 <jferlan at 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 <skrtbhtngr at 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180413/2585e4a9/attachment-0001.htm>
More information about the libvir-list
mailing list