src/qemu/qemu_command.c | 111 +++++++++++++++++++++++++----------------------- 1 file changed, 59 insertions(+), 52 deletions(-)
This patch adds virQEMUBuildBufferEscapeComma to properly
escape commas in user provided data fields for qemu command
line processing.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com>
---
Thank you for the helpful feedback and apologies for the delay.
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.
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)
{
virURIPtr uri = NULL;
- char *ret = NULL;
+ char *ret = NULL, *socket = NULL;
+ 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;
+ }
if (qemuBuildGeneralSecinfoURI(uri, secinfo) < 0)
goto cleanup;
@@ -860,6 +864,8 @@ qemuBuildNetworkDriveURI(virStorageSourcePtr src,
cleanup:
virURIFree(uri);
+ virBufferFreeAndReset(&buf);
+
return ret;
}
@@ -868,8 +874,9 @@ static char *
qemuBuildNetworkDriveStr(virStorageSourcePtr src,
qemuDomainSecretInfoPtr secinfo)
{
- char *ret = NULL;
+ char *ret = NULL, *path = NULL, *file = NULL;
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 (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",
@@ -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 (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 (virBufferCheckError(&buf) < 0)
goto cleanup;
@@ -1022,6 +1037,7 @@ qemuBuildNetworkDriveStr(virStorageSourcePtr src,
}
cleanup:
+ virBufferFreeAndReset(&bufTemp);
virBufferFreeAndReset(&buf);
return ret;
@@ -1630,6 +1646,8 @@ qemuBuildDiskThrottling(virDomainDiskDefPtr disk,
virBufferAsprintf(buf, ",throttling." _label "=%llu", \
disk->blkdeviotune._field); \
}
+ virBuffer bufTemp = VIR_BUFFER_INITIALIZER;
+ char *name = NULL;
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) {
- virBufferEscapeString(buf, ",throttling.group=%s",
- disk->blkdeviotune.group_name);
+ virQEMUBuildBufferEscapeComma(&bufTemp, disk->blkdeviotune.group_name);
+ name = virBufferContentAndReset(&bufTemp);
+ virBufferEscapeString(buf, ",throttling.group=%s", 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);
#undef IOTUNE_ADD
}
@@ -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);
+ : "");
+ 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:
@@ -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;
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)
goto cleanup;
break;
@@ -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);
}
}
@@ -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:
--
2.16.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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
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@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@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
On 04/12/2018 05:40 PM, Sukrit Bhatnagar wrote: > 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? yes, combined > If that is the case, do I need the next patch to be v4 or can the series > of patches start from v1? v4 and in this case you'd generate a cover letter along with your git send-email to describe the changes and can then point at the v2 and v3 of the series for which the v4 will combine the changes. > I can start afresh with the patches and this might save some confusion > later. > > Thanks. > John FWIW: top posting is another one of those "things" not to do in a technical news group [...] -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.