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@xxxxxxxxx>
> ---
>
> 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
>From e10f66a9bd6b6853221fc71a7e13e26e2a06473f Mon Sep 17 00:00:00 2001
From: Sukrit Bhatnagar <skrtbhtngr@xxxxxxxxx>
Date: Tue, 10 Apr 2018 15:59:15 -0400
Subject: [PATCH 2/6] qemu: Escape commas for qemuBuildDriveDevStr
Add comma escaping for disk->vendor and disk->product when being
built for the command line (and not from hotplug).
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@xxxxxxxxx>
Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
---
src/qemu/qemu_command.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 4618d2dd1b..0d219c20a7 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -2174,11 +2174,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)) {
--
2.13.6
>From 1442cca2c26ef9dc7eaea5fabe067b42dc308199 Mon Sep 17 00:00:00 2001
From: Sukrit Bhatnagar <skrtbhtngr@xxxxxxxxx>
Date: Tue, 10 Apr 2018 15:51:05 -0400
Subject: [PATCH 1/6] qemu: Escape commas for qemuBuildRomStr
Add comma escaping for info->romfile.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@xxxxxxxxx>
Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
---
src/qemu/qemu_command.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 514c3ab2ef..4618d2dd1b 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;
}
--
2.13.6
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
