[libvirt] [PATCH 00/10] start tackling *printf issues
Based on the long thread about whether %zu and %llu are safe to use, here's a series of patches to start tackling the problems. I've got more work to do, but had enough ready that it was worth posting this much for review, and can push in pieces as individual patches are ACK'd. Eric Blake (10): uml: fix logic bug in checking reply length nwfilter: use consistent OOM reporting build: delete dead comment maint: whitespace cleanups storage: avoid s[n]printf squash to dead comment xenapi: avoid sprintf vbox: add location used in rpmfusion release vbox: factor a large function vbox: avoid sprintf configure.ac | 1 + src/nwfilter/nwfilter_driver.c | 6 +- src/qemu/qemu_driver.c | 1 - src/storage/storage_backend.c | 16 ++- src/storage/storage_backend_disk.c | 124 +++++++++----- src/uml/uml_driver.c | 8 +- src/vbox/vbox_tmpl.c | 327 +++++++++++++++++++----------------- src/xen/sexpr.c | 8 - src/xenapi/xenapi_utils.c | 15 +- src/xenapi/xenapi_utils.h | 4 - 10 files changed, 277 insertions(+), 233 deletions(-) -- 1.7.2.1
* src/uml/uml_driver.c (umlMonitorCommand): Validate that enough bytes were read to dereference both res.length, and that many bytes from res.data. Reported by Soren Hansen. --- Whoops; this is a resend of an unrelated issue, but it is still sitting on my tree, and the original email has no review yet, perhaps because it was in a reply to a longish thread. src/uml/uml_driver.c | 7 ++----- 1 files changed, 2 insertions(+), 5 deletions(-) diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 04493ba..37ddc39 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -737,14 +737,11 @@ static int umlMonitorCommand(const struct uml_driver *driver, virReportSystemError(errno, _("cannot read reply %s"), cmd); goto error; } - if (nbytes < sizeof res) { + if (nbytes < offsetof(struct monitor_request, data) || + nbytes < res.length + offsetof(struct monitor_request, data)) { virReportSystemError(0, _("incomplete reply %s"), cmd); goto error; } - if (sizeof res.data < res.length) { - virReportSystemError(0, _("invalid length in reply %s"), cmd); - goto error; - } if (VIR_REALLOC_N(retdata, retlen + res.length) < 0) { virReportOOMError(); -- 1.7.2.1
2010/8/19 Eric Blake <eblake@redhat.com>:
* src/uml/uml_driver.c (umlMonitorCommand): Validate that enough bytes were read to dereference both res.length, and that many bytes from res.data. Reported by Soren Hansen. ---
Whoops; this is a resend of an unrelated issue, but it is still sitting on my tree, and the original email has no review yet, perhaps because it was in a reply to a longish thread.
src/uml/uml_driver.c | 7 ++----- 1 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 04493ba..37ddc39 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -737,14 +737,11 @@ static int umlMonitorCommand(const struct uml_driver *driver, virReportSystemError(errno, _("cannot read reply %s"), cmd); goto error; } - if (nbytes < sizeof res) { + if (nbytes < offsetof(struct monitor_request, data) || + nbytes < res.length + offsetof(struct monitor_request, data)) {
You could reverse the order to nbytes < offsetof(struct monitor_request, data) + res.length to be in line with the layout of the data, but that's just me nit-picking here. ACK. Matthias
On 08/19/2010 02:47 PM, Matthias Bolte wrote:
2010/8/19 Eric Blake <eblake@redhat.com>:
* src/uml/uml_driver.c (umlMonitorCommand): Validate that enough bytes were read to dereference both res.length, and that many bytes from res.data. Reported by Soren Hansen. --- - if (nbytes < sizeof res) { + if (nbytes < offsetof(struct monitor_request, data) || + nbytes < res.length + offsetof(struct monitor_request, data)) {
You could reverse the order to
nbytes < offsetof(struct monitor_request, data) + res.length
to be in line with the layout of the data, but that's just me nit-picking here.
Sure, why not.
ACK.
Done, and pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
* src/nwfilter/nwfilter_driver.c (nwfilterLog): Delete. (nwfilterDriverStartup): Use virReportOOMError instead. --- No point making printf uses harder to audit by hiding them in a macro, especially when this file already uses virReportOOMError elsewhere. src/nwfilter/nwfilter_driver.c | 6 +----- 1 files changed, 1 insertions(+), 5 deletions(-) diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 0e8241e..bda50f9 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -42,9 +42,6 @@ #define VIR_FROM_THIS VIR_FROM_NWFILTER -#define nwfilterLog(msg...) fprintf(stderr, msg) - - static virNWFilterDriverStatePtr driverState; static int nwfilterDriverShutdown(void); @@ -95,7 +92,6 @@ nwfilterDriverStartup(int privileged) { goto error; if (virAsprintf(&base, "%s/.libvirt", userdir) == -1) { - nwfilterLog("out of memory in virAsprintf"); VIR_FREE(userdir); goto out_of_memory; } @@ -118,7 +114,7 @@ nwfilterDriverStartup(int privileged) { return 0; out_of_memory: - nwfilterLog("virNWFilterStartup: out of memory"); + virReportOOMError(); error: VIR_FREE(base); -- 1.7.2.1
libvir-list-bounces@redhat.com wrote on 08/18/2010 07:45:05 PM:
libvir-list-bounces@redhat.com
* src/nwfilter/nwfilter_driver.c (nwfilterLog): Delete. (nwfilterDriverStartup): Use virReportOOMError instead. ---
No point making printf uses harder to audit by hiding them in a macro, especially when this file already uses virReportOOMError elsewhere.
src/nwfilter/nwfilter_driver.c | 6 +----- 1 files changed, 1 insertions(+), 5 deletions(-)
diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 0e8241e..bda50f9 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -42,9 +42,6 @@
#define VIR_FROM_THIS VIR_FROM_NWFILTER
-#define nwfilterLog(msg...) fprintf(stderr, msg) - - static virNWFilterDriverStatePtr driverState;
static int nwfilterDriverShutdown(void); @@ -95,7 +92,6 @@ nwfilterDriverStartup(int privileged) { goto error;
if (virAsprintf(&base, "%s/.libvirt", userdir) == -1) { - nwfilterLog("out of memory in virAsprintf"); VIR_FREE(userdir); goto out_of_memory; } @@ -118,7 +114,7 @@ nwfilterDriverStartup(int privileged) { return 0;
out_of_memory: - nwfilterLog("virNWFilterStartup: out of memory"); + virReportOOMError();
error: VIR_FREE(base);
ACK. Stefan
On 08/19/2010 12:41 PM, Stefan Berger wrote:
libvir-list-bounces@redhat.com wrote on 08/18/2010 07:45:05 PM:
libvir-list-bounces@redhat.com
* src/nwfilter/nwfilter_driver.c (nwfilterLog): Delete. (nwfilterDriverStartup): Use virReportOOMError instead.
ACK.
Thanks; pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
* src/qemu/qemu_driver.c (qemudGetProcessInfo): Clean up. --- src/qemu/qemu_driver.c | 1 - 1 files changed, 0 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3d61ccd..656a1e4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4316,7 +4316,6 @@ static int qemudGetProcessInfo(unsigned long long *cpuTime, int *lastCpu, int pi } if (!(pidinfo = fopen(proc, "r"))) { - /*printf("cannot read pid info");*/ /* VM probably shut down, so fake 0 */ if (cpuTime) *cpuTime = 0; -- 1.7.2.1
2010/8/19 Eric Blake <eblake@redhat.com>:
* src/qemu/qemu_driver.c (qemudGetProcessInfo): Clean up. --- src/qemu/qemu_driver.c | 1 - 1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3d61ccd..656a1e4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4316,7 +4316,6 @@ static int qemudGetProcessInfo(unsigned long long *cpuTime, int *lastCpu, int pi }
if (!(pidinfo = fopen(proc, "r"))) { - /*printf("cannot read pid info");*/ /* VM probably shut down, so fake 0 */ if (cpuTime) *cpuTime = 0;
This line even got a typo fix 2 years ago :) ACK. Matthias
* src/storage/storage_backend_disk.c (virStorageBackendDiskPartFormat): Fix spacing. --- Should be cosmetic only. src/storage/storage_backend_disk.c | 66 +++++++++++++++++++----------------- 1 files changed, 35 insertions(+), 31 deletions(-) diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index 7188386..4038093 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -385,20 +385,22 @@ virStorageBackendDiskPartFormat(virStoragePoolObjPtr pool, { int i; if (pool->def->source.format == VIR_STORAGE_POOL_DISK_DOS) { - const char *partedFormat = virStoragePartedFsTypeTypeToString(vol->target.format); + const char *partedFormat; + partedFormat = virStoragePartedFsTypeTypeToString(vol->target.format); if(partedFormat == NULL) { - virStorageReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Invalid partition type")); - return -1; + virStorageReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Invalid partition type")); + return -1; } if (vol->target.format == VIR_STORAGE_VOL_DISK_EXTENDED) { /* make sure we don't have a extended partition already */ for (i = 0; i < pool->volumes.count; i++) { - if (pool->volumes.objs[i]->target.format == VIR_STORAGE_VOL_DISK_EXTENDED) { - virStorageReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("extended partition already exists")); - return -1; - } + if (pool->volumes.objs[i]->target.format == + VIR_STORAGE_VOL_DISK_EXTENDED) { + virStorageReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("extended partition already exists")); + return -1; + } } sprintf(partFormat, "%s", partedFormat); } else { @@ -407,25 +409,26 @@ virStorageBackendDiskPartFormat(virStoragePoolObjPtr pool, to create logical partitions. */ /* XXX Only support one extended partition */ switch (virStorageBackendDiskPartTypeToCreate(pool)) { - case VIR_STORAGE_VOL_DISK_TYPE_PRIMARY: - sprintf(partFormat, "primary %s", partedFormat); - break; - case VIR_STORAGE_VOL_DISK_TYPE_LOGICAL: - /* make sure we have a extended partition */ - for (i = 0; i < pool->volumes.count; i++) { - if (pool->volumes.objs[i]->target.format == VIR_STORAGE_VOL_DISK_EXTENDED) { - sprintf(partFormat, "logical %s", partedFormat); - break; - } - } - if (i == pool->volumes.count) { - virStorageReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("no extended partition found and no primary partition available")); - return -1; - } - break; - default: - break; + case VIR_STORAGE_VOL_DISK_TYPE_PRIMARY: + sprintf(partFormat, "primary %s", partedFormat); + break; + case VIR_STORAGE_VOL_DISK_TYPE_LOGICAL: + /* make sure we have a extended partition */ + for (i = 0; i < pool->volumes.count; i++) { + if (pool->volumes.objs[i]->target.format == + VIR_STORAGE_VOL_DISK_EXTENDED) { + sprintf(partFormat, "logical %s", partedFormat); + break; + } + } + if (i == pool->volumes.count) { + virStorageReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("no extended partition found and no primary partition available")); + return -1; + } + break; + default: + break; } } } else { @@ -436,7 +439,7 @@ virStorageBackendDiskPartFormat(virStoragePoolObjPtr pool, /** * Aligns a new partition to nearest cylinder boundry - * when haveing a msdos partition table type + * when having a msdos partition table type * to avoid any problem with all ready existing * partitions */ @@ -455,7 +458,8 @@ virStorageBackendDiskPartBoundries(virStoragePoolObjPtr pool, unsigned long long cylinderSize = dev->geometry.heads * dev->geometry.sectors * SECTOR_SIZE; - DEBUG("find free area: allocation %llu, cyl size %llu", allocation, cylinderSize); + DEBUG("find free area: allocation %llu, cyl size %llu", allocation, + cylinderSize); int partType = virStorageBackendDiskPartTypeToCreate(pool); /* how many extra bytes we have since we allocate @@ -561,7 +565,7 @@ virStorageBackendDiskCreateVol(virConnectPtr conn ATTRIBUTE_UNUSED, if (virStorageBackendDiskPartBoundries(pool, &startOffset, &endOffset, vol->capacity) != 0) { - return -1; + return -1; } snprintf(start, sizeof(start)-1, "%lluB", startOffset); -- 1.7.2.1
2010/8/19 Eric Blake <eblake@redhat.com>:
* src/storage/storage_backend_disk.c (virStorageBackendDiskPartFormat): Fix spacing. ---
Should be cosmetic only.
src/storage/storage_backend_disk.c | 66 +++++++++++++++++++----------------- 1 files changed, 35 insertions(+), 31 deletions(-)
diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index 7188386..4038093 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -385,20 +385,22 @@ virStorageBackendDiskPartFormat(virStoragePoolObjPtr pool, { int i; if (pool->def->source.format == VIR_STORAGE_POOL_DISK_DOS) { - const char *partedFormat = virStoragePartedFsTypeTypeToString(vol->target.format); + const char *partedFormat; + partedFormat = virStoragePartedFsTypeTypeToString(vol->target.format); if(partedFormat == NULL) {
While touching this code add a space between if and (. ACK. Matthias
On 08/19/2010 02:59 PM, Matthias Bolte wrote:
2010/8/19 Eric Blake <eblake@redhat.com>:
* src/storage/storage_backend_disk.c (virStorageBackendDiskPartFormat): Fix spacing. ---
Should be cosmetic only.
src/storage/storage_backend_disk.c | 66 +++++++++++++++++++----------------- 1 files changed, 35 insertions(+), 31 deletions(-)
diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index 7188386..4038093 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -385,20 +385,22 @@ virStorageBackendDiskPartFormat(virStoragePoolObjPtr pool, { int i; if (pool->def->source.format == VIR_STORAGE_POOL_DISK_DOS) { - const char *partedFormat = virStoragePartedFsTypeTypeToString(vol->target.format); + const char *partedFormat; + partedFormat = virStoragePartedFsTypeTypeToString(vol->target.format); if(partedFormat == NULL) {
While touching this code add a space between if and (.
ACK.
Done for several instance in that file, and pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
* src/storage/storage_backend.c (virStorageBackendCreateQemuImg) (virStorageBackendCreateQcowCreate): Use virAsprintf instead. * src/storage/storage_backend_disk.c (virStorageBackendDiskCreateVol, virStorageBackendDiskPartFormat): Likewise. --- Things to look out for: virStorageBackendDiskPartFormat can now fail where it used to do nothing to the passed-in partFormat variable, if the switch statement hits the default. src/storage/storage_backend.c | 16 ++++++-- src/storage/storage_backend_disk.c | 66 +++++++++++++++++++++++++---------- 2 files changed, 59 insertions(+), 23 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 1fe7ba6..580d859 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -636,7 +636,7 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, unsigned int flags ATTRIBUTE_UNUSED) { int ret = -1; - char size[100]; + char *size = NULL; char *create_tool; const char *type = virStorageFileFormatTypeToString(vol->target.format); @@ -726,7 +726,10 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, } /* Size in KB */ - snprintf(size, sizeof(size), "%lluK", vol->capacity/1024); + if (virAsprintf(&size, "%lluK", vol->capacity / 1024) < 0) { + virReportOOMError(); + goto cleanup; + } /* KVM is usually ahead of qemu on features, so try that first */ create_tool = virFindFileInPath("kvm-img"); @@ -821,6 +824,7 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, } cleanup: + VIR_FREE(size); VIR_FREE(create_tool); return ret; @@ -838,7 +842,7 @@ virStorageBackendCreateQcowCreate(virConnectPtr conn ATTRIBUTE_UNUSED, unsigned int flags ATTRIBUTE_UNUSED) { int ret; - char size[100]; + char *size; const char *imgargv[4]; if (inputvol) { @@ -867,7 +871,10 @@ virStorageBackendCreateQcowCreate(virConnectPtr conn ATTRIBUTE_UNUSED, } /* Size in MB - yes different units to qemu-img :-( */ - snprintf(size, sizeof(size), "%llu", vol->capacity/1024/1024); + if (virAsprintf(&size, "%llu", vol->capacity / 1024 / 1024) < 0) { + virReportOOMError(); + return -1; + } imgargv[0] = virFindFileInPath("qcow-create"); imgargv[1] = size; @@ -876,6 +883,7 @@ virStorageBackendCreateQcowCreate(virConnectPtr conn ATTRIBUTE_UNUSED, ret = virStorageBackendCreateExecCommand(pool, vol, imgargv); VIR_FREE(imgargv[0]); + VIR_FREE(size); return ret; } diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index 4038093..587b1a6 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -381,7 +381,7 @@ virStorageBackendDiskPartTypeToCreate(virStoragePoolObjPtr pool) static int virStorageBackendDiskPartFormat(virStoragePoolObjPtr pool, virStorageVolDefPtr vol, - char* partFormat) + char** partFormat) { int i; if (pool->def->source.format == VIR_STORAGE_POOL_DISK_DOS) { @@ -402,7 +402,10 @@ virStorageBackendDiskPartFormat(virStoragePoolObjPtr pool, return -1; } } - sprintf(partFormat, "%s", partedFormat); + if ((*partFormat = strdup(partedFormat)) == NULL) { + virReportOOMError(); + return -1; + } } else { /* create primary partition as long as it is possible and after that check if an extended partition exists @@ -410,14 +413,21 @@ virStorageBackendDiskPartFormat(virStoragePoolObjPtr pool, /* XXX Only support one extended partition */ switch (virStorageBackendDiskPartTypeToCreate(pool)) { case VIR_STORAGE_VOL_DISK_TYPE_PRIMARY: - sprintf(partFormat, "primary %s", partedFormat); + if (virAsprintf(partFormat, "primary %s", partedFormat) < 0) { + virReportOOMError(); + return -1; + } break; case VIR_STORAGE_VOL_DISK_TYPE_LOGICAL: /* make sure we have a extended partition */ for (i = 0; i < pool->volumes.count; i++) { if (pool->volumes.objs[i]->target.format == VIR_STORAGE_VOL_DISK_EXTENDED) { - sprintf(partFormat, "logical %s", partedFormat); + if (virAsprintf(partFormat, "logical %s", + partedFormat) < 0) { + virReportOOMError(); + return -1; + } break; } } @@ -428,11 +438,16 @@ virStorageBackendDiskPartFormat(virStoragePoolObjPtr pool, } break; default: - break; + virStorageReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("unknown partition type")); + return -1; } } } else { - sprintf(partFormat, "primary"); + if ((*partFormat = strdup("primary")) == NULL) { + virReportOOMError(); + return -1; + } } return 0; } @@ -538,16 +553,19 @@ virStorageBackendDiskCreateVol(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolObjPtr pool, virStorageVolDefPtr vol) { - char start[100], end[100], partFormat[100]; + int res = -1; + char *start = NULL; + char *end = NULL; + char *partFormat; unsigned long long startOffset = 0, endOffset = 0; const char *cmdargv[] = { PARTED, pool->def->source.devices[0].path, "mkpart", "--script", - partFormat, - start, - end, + NULL /*partFormat*/, + NULL /*start*/, + NULL /*end*/, NULL }; @@ -558,23 +576,27 @@ virStorageBackendDiskCreateVol(virConnectPtr conn ATTRIBUTE_UNUSED, return -1; } - if (virStorageBackendDiskPartFormat(pool, vol, partFormat) != 0) { + if (virStorageBackendDiskPartFormat(pool, vol, &partFormat) != 0) { return -1; } + cmdargv[4] = partFormat; if (virStorageBackendDiskPartBoundries(pool, &startOffset, &endOffset, vol->capacity) != 0) { - return -1; + goto cleanup; } - snprintf(start, sizeof(start)-1, "%lluB", startOffset); - start[sizeof(start)-1] = '\0'; - snprintf(end, sizeof(end)-1, "%lluB", endOffset); - end[sizeof(end)-1] = '\0'; + if (virAsprintf(&start, "%lluB", startOffset) < 0 || + virAsprintf(&end, "%lluB", endOffset) < 0) { + virReportOOMError(); + goto cleanup; + } + cmdargv[5] = start; + cmdargv[6] = end; if (virRun(cmdargv, NULL) < 0) - return -1; + goto cleanup; /* wait for device node to show up */ virFileWaitForDevices(); @@ -588,9 +610,15 @@ virStorageBackendDiskCreateVol(virConnectPtr conn ATTRIBUTE_UNUSED, /* Fetch actual extent info, generate key */ if (virStorageBackendDiskReadPartitions(pool, vol) < 0) - return -1; + goto cleanup; - return 0; + res = 0; + +cleanup: + VIR_FREE(partFormat); + VIR_FREE(start); + VIR_FREE(end); + return res; } static int -- 1.7.2.1
2010/8/19 Eric Blake <eblake@redhat.com>:
* src/storage/storage_backend.c (virStorageBackendCreateQemuImg) (virStorageBackendCreateQcowCreate): Use virAsprintf instead. * src/storage/storage_backend_disk.c (virStorageBackendDiskCreateVol, virStorageBackendDiskPartFormat): Likewise. ---
Things to look out for: virStorageBackendDiskPartFormat can now fail where it used to do nothing to the passed-in partFormat variable, if the switch statement hits the default.
Nice one, before it would just have executed parted with a random string in the argumentlist in that case. ACK. Matthias
On 08/19/2010 03:10 PM, Matthias Bolte wrote:
2010/8/19 Eric Blake <eblake@redhat.com>:
* src/storage/storage_backend.c (virStorageBackendCreateQemuImg) (virStorageBackendCreateQcowCreate): Use virAsprintf instead. * src/storage/storage_backend_disk.c (virStorageBackendDiskCreateVol, virStorageBackendDiskPartFormat): Likewise. ---
Things to look out for: virStorageBackendDiskPartFormat can now fail where it used to do nothing to the passed-in partFormat variable, if the switch statement hits the default.
Nice one, before it would just have executed parted with a random string in the argumentlist in that case.
ACK.
Thanks; pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
* src/uml/uml_driver.c (umlGetProcessInfo): Likewise. * src/xen/sexpr.c (_string2sexpr): Likewise. --- Oops - I hit 'git send-email' before rebasing one last time. This one will be squashed into 3/10 before I push anything. src/uml/uml_driver.c | 1 - src/xen/sexpr.c | 8 -------- 2 files changed, 0 insertions(+), 9 deletions(-) diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 37ddc39..2940978 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -1069,7 +1069,6 @@ static int umlGetProcessInfo(unsigned long long *cpuTime, int pid) { } if (!(pidinfo = fopen(proc, "r"))) { - /*printf("cannot read pid info");*/ /* VM probably shut down, so fake 0 */ *cpuTime = 0; return 0; diff --git a/src/xen/sexpr.c b/src/xen/sexpr.c index 7e370db..2184060 100644 --- a/src/xen/sexpr.c +++ b/src/xen/sexpr.c @@ -320,14 +320,6 @@ _string2sexpr(const char *buffer, size_t * end) sexpr_free(tmp); goto error; } -#if 0 - if (0) { - char buf[4096]; - - sexpr2string(ret, buf, sizeof(buf)); - printf("%s\n", buffer); - } -#endif ptr = trim(ptr + tmp_len); } -- 1.7.2.1
2010/8/19 Eric Blake <eblake@redhat.com>:
* src/uml/uml_driver.c (umlGetProcessInfo): Likewise. * src/xen/sexpr.c (_string2sexpr): Likewise. ---
Oops - I hit 'git send-email' before rebasing one last time. This one will be squashed into 3/10 before I push anything.
src/uml/uml_driver.c | 1 - src/xen/sexpr.c | 8 -------- 2 files changed, 0 insertions(+), 9 deletions(-)
diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 37ddc39..2940978 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -1069,7 +1069,6 @@ static int umlGetProcessInfo(unsigned long long *cpuTime, int pid) { }
if (!(pidinfo = fopen(proc, "r"))) { - /*printf("cannot read pid info");*/ /* VM probably shut down, so fake 0 */ *cpuTime = 0; return 0; diff --git a/src/xen/sexpr.c b/src/xen/sexpr.c index 7e370db..2184060 100644 --- a/src/xen/sexpr.c +++ b/src/xen/sexpr.c @@ -320,14 +320,6 @@ _string2sexpr(const char *buffer, size_t * end) sexpr_free(tmp); goto error; } -#if 0 - if (0) { - char buf[4096]; - - sexpr2string(ret, buf, sizeof(buf)); - printf("%s\n", buffer); - } -#endif
Disabled two times :) ACK. Matthias
On 08/19/2010 03:11 PM, Matthias Bolte wrote:
2010/8/19 Eric Blake <eblake@redhat.com>:
* src/uml/uml_driver.c (umlGetProcessInfo): Likewise. * src/xen/sexpr.c (_string2sexpr): Likewise. ---
Oops - I hit 'git send-email' before rebasing one last time. This one will be squashed into 3/10 before I push anything.
-#if 0 - if (0) { - char buf[4096]; - - sexpr2string(ret, buf, sizeof(buf)); - printf("%s\n", buffer); - } -#endif
Disabled two times :)
Try as you might, you ain't running this code ;)
ACK.
Squashed with 3/10, and pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
* src/xenapi/xenapi_utils.h (createVifNetwork): Delete prototype. * src/xenapi/xenapi_utils.c (createVifNetwork): Change signature, and use virAsprintf. Detect allocation failure. (createVMRecordFromXml): Adjust caller. --- I couldn't find any other uses of createVifNetwork, so changing its prototype and marking it static seemed best. src/xenapi/xenapi_utils.c | 15 +++++++++------ src/xenapi/xenapi_utils.h | 4 ---- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/src/xenapi/xenapi_utils.c b/src/xenapi/xenapi_utils.c index 4eb17fa..23d3fef 100644 --- a/src/xenapi/xenapi_utils.c +++ b/src/xenapi/xenapi_utils.c @@ -387,8 +387,8 @@ xenapiSessionErrorHandle(virConnectPtr conn, virErrorNumber errNum, } /* creates network intereface for VM */ -int -createVifNetwork (virConnectPtr conn, xen_vm vm, char *device, +static int +createVifNetwork (virConnectPtr conn, xen_vm vm, int device, char *bridge, char *mac) { xen_session *session = ((struct _xenapiPrivate *)(conn->privateData))->session; @@ -432,7 +432,8 @@ createVifNetwork (virConnectPtr conn, xen_vm vm, char *device, vif_record->other_config = xen_string_string_map_alloc(0); vif_record->runtime_properties = xen_string_string_map_alloc(0); vif_record->qos_algorithm_params = xen_string_string_map_alloc(0); - vif_record->device = strdup(device); + if (virAsprintf(&vif_record->device, "%d", device) < 0) + return -1; xen_vif_create(session, &vif, vif_record); if (!vif) { xen_vif_free(vif); @@ -553,9 +554,11 @@ createVMRecordFromXml (virConnectPtr conn, virDomainDefPtr def, } } if (mac != NULL && bridge != NULL) { - char device[NETWORK_DEVID_SIZE] = "\0"; - sprintf(device, "%d", device_number); - createVifNetwork(conn, *vm, device, bridge, mac); + if (createVifNetwork(conn, *vm, device_number, bridge, + mac) < 0) { + VIR_FREE(bridge); + goto error_cleanup; + } VIR_FREE(bridge); device_number++; } diff --git a/src/xenapi/xenapi_utils.h b/src/xenapi/xenapi_utils.h index c062a1e..2140105 100644 --- a/src/xenapi/xenapi_utils.h +++ b/src/xenapi/xenapi_utils.h @@ -78,8 +78,4 @@ createVMRecordFromXml (virConnectPtr conn, virDomainDefPtr defPtr, int allocStringMap (xen_string_string_map **strings, char *key, char *val); -int -createVifNetwork(virConnectPtr conn, xen_vm vm, char *device, - char *bridge, char *mac); - #endif /* __VIR_XENAPI_UTILS__ */ -- 1.7.2.1
2010/8/19 Eric Blake <eblake@redhat.com>:
* src/xenapi/xenapi_utils.h (createVifNetwork): Delete prototype. * src/xenapi/xenapi_utils.c (createVifNetwork): Change signature, and use virAsprintf. Detect allocation failure. (createVMRecordFromXml): Adjust caller. ---
I couldn't find any other uses of createVifNetwork, so changing its prototype and marking it static seemed best.
ACK. Matthias
On 08/19/2010 03:14 PM, Matthias Bolte wrote:
2010/8/19 Eric Blake <eblake@redhat.com>:
* src/xenapi/xenapi_utils.h (createVifNetwork): Delete prototype. * src/xenapi/xenapi_utils.c (createVifNetwork): Change signature, and use virAsprintf. Detect allocation failure. (createVMRecordFromXml): Adjust caller. ---
I couldn't find any other uses of createVifNetwork, so changing its prototype and marking it static seemed best.
ACK.
Thanks; pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
* configure.ac (vbox_xpcomc_dir): Add another potential dir. --- To test that my vbox changes would still compile, I had to install vbox. But VirtualBox-OSE-3.2.6-2.fc13.x86_64 from rpmfusion-free installs to a new location. configure.ac | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/configure.ac b/configure.ac index 3968617..d42d9e6 100644 --- a/configure.ac +++ b/configure.ac @@ -336,6 +336,7 @@ if test "x$with_vbox" = "xyes" || test "x$with_vbox" = "xcheck"; then for vbox in \ /usr/lib/virtualbox/VBoxXPCOMC.so \ + /usr/lib64/virtualbox/VBoxXPCOMC.so \ /usr/lib/VirtualBox/VBoxXPCOMC.so \ /opt/virtualbox/VBoxXPCOMC.so \ /opt/VirtualBox/VBoxXPCOMC.so \ -- 1.7.2.1
2010/8/19 Eric Blake <eblake@redhat.com>:
* configure.ac (vbox_xpcomc_dir): Add another potential dir. ---
To test that my vbox changes would still compile, I had to install vbox. But VirtualBox-OSE-3.2.6-2.fc13.x86_64 from rpmfusion-free installs to a new location.
configure.ac | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/configure.ac b/configure.ac index 3968617..d42d9e6 100644 --- a/configure.ac +++ b/configure.ac @@ -336,6 +336,7 @@ if test "x$with_vbox" = "xyes" || test "x$with_vbox" = "xcheck"; then
for vbox in \ /usr/lib/virtualbox/VBoxXPCOMC.so \ + /usr/lib64/virtualbox/VBoxXPCOMC.so \ /usr/lib/VirtualBox/VBoxXPCOMC.so \ /opt/virtualbox/VBoxXPCOMC.so \ /opt/VirtualBox/VBoxXPCOMC.so \
You could have tricked configure instead of actually installing VirtualBox, but this way had an additional benefit :) ACK. Matthias
On 08/19/2010 03:17 PM, Matthias Bolte wrote:
2010/8/19 Eric Blake <eblake@redhat.com>:
* configure.ac (vbox_xpcomc_dir): Add another potential dir. ---
To test that my vbox changes would still compile, I had to install vbox. But VirtualBox-OSE-3.2.6-2.fc13.x86_64 from rpmfusion-free installs to a new location.
configure.ac | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/configure.ac b/configure.ac index 3968617..d42d9e6 100644 --- a/configure.ac +++ b/configure.ac @@ -336,6 +336,7 @@ if test "x$with_vbox" = "xyes" || test "x$with_vbox" = "xcheck"; then
for vbox in \ /usr/lib/virtualbox/VBoxXPCOMC.so \ + /usr/lib64/virtualbox/VBoxXPCOMC.so \ /usr/lib/VirtualBox/VBoxXPCOMC.so \ /opt/virtualbox/VBoxXPCOMC.so \ /opt/VirtualBox/VBoxXPCOMC.so \
You could have tricked configure instead of actually installing VirtualBox, but this way had an additional benefit :)
ACK.
Thanks; pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
* src/vbox/vbox_tmpl.c (vboxDomainCreateWithFlags): Split... (vboxStartMachine): ...into new helper. --- This function was just too unbearable with that much nested indentation. This should be a no-op refactoring. vboxDomainDefineXML is even worse, and contains the remaining sprintf instances in this file; I'll probably try to factor that one down as well. src/vbox/vbox_tmpl.c | 318 ++++++++++++++++++++++++++------------------------ 1 files changed, 165 insertions(+), 153 deletions(-) diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 31fec67..c28981c 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -3144,14 +3144,174 @@ cleanup: return ret; } + +static int +vboxStartMachine(virDomainPtr dom, int i, IMachine *machine, vboxIID *iid + ) +{ + VBOX_OBJECT_CHECK(dom->conn, int, -1); + int vrdpPresent = 0; + int sdlPresent = 0; + int guiPresent = 0; + char *guiDisplay = NULL; + char *sdlDisplay = NULL; + PRUnichar *keyTypeUtf16 = NULL; + PRUnichar *valueTypeUtf16 = NULL; + char *valueTypeUtf8 = NULL; + PRUnichar *keyDislpayUtf16 = NULL; + PRUnichar *valueDisplayUtf16 = NULL; + char *valueDisplayUtf8 = NULL; + IProgress *progress = NULL; + char displayutf8[32] = {0}; + PRUnichar *env = NULL; + PRUnichar *sessionType = NULL; + nsresult rc; + + VBOX_UTF8_TO_UTF16("FRONTEND/Type", &keyTypeUtf16); + machine->vtbl->GetExtraData(machine, keyTypeUtf16, &valueTypeUtf16); + VBOX_UTF16_FREE(keyTypeUtf16); + + if (valueTypeUtf16) { + VBOX_UTF16_TO_UTF8(valueTypeUtf16, &valueTypeUtf8); + VBOX_UTF16_FREE(valueTypeUtf16); + + if ( STREQ(valueTypeUtf8, "sdl") || STREQ(valueTypeUtf8, "gui") ) { + + VBOX_UTF8_TO_UTF16("FRONTEND/Display", &keyDislpayUtf16); + machine->vtbl->GetExtraData(machine, keyDislpayUtf16, + &valueDisplayUtf16); + VBOX_UTF16_FREE(keyDislpayUtf16); + + if (valueDisplayUtf16) { + VBOX_UTF16_TO_UTF8(valueDisplayUtf16, &valueDisplayUtf8); + VBOX_UTF16_FREE(valueDisplayUtf16); + + if (strlen(valueDisplayUtf8) <= 0) { + VBOX_UTF8_FREE(valueDisplayUtf8); + valueDisplayUtf8 = NULL; + } + } + + if (STREQ(valueTypeUtf8, "sdl")) { + sdlPresent = 1; + if (valueDisplayUtf8) { + sdlDisplay = strdup(valueDisplayUtf8); + if (sdlDisplay == NULL) { + virReportOOMError(); + /* just don't go to cleanup yet as it is ok to have + * sdlDisplay as NULL and we check it below if it + * exist and then only use it there + */ + } + } + } + + if (STREQ(valueTypeUtf8, "gui")) { + guiPresent = 1; + if (valueDisplayUtf8) { + guiDisplay = strdup(valueDisplayUtf8); + if (guiDisplay == NULL) { + virReportOOMError(); + /* just don't go to cleanup yet as it is ok to have + * guiDisplay as NULL and we check it below if it + * exist and then only use it there + */ + } + } + } + } + + if (STREQ(valueTypeUtf8, "vrdp")) { + vrdpPresent = 1; + } + + if (!vrdpPresent && !sdlPresent && !guiPresent) { + /* if nothing is selected it means either the machine xml + * file is really old or some values are missing so fallback + */ + guiPresent = 1; + } + + VBOX_UTF8_FREE(valueTypeUtf8); + + } else { + guiPresent = 1; + } + if (valueDisplayUtf8) + VBOX_UTF8_FREE(valueDisplayUtf8); + + if (guiPresent) { + if (guiDisplay) { + sprintf(displayutf8, "DISPLAY=%.24s", guiDisplay); + VBOX_UTF8_TO_UTF16(displayutf8, &env); + VIR_FREE(guiDisplay); + } + + VBOX_UTF8_TO_UTF16("gui", &sessionType); + } + + if (sdlPresent) { + if (sdlDisplay) { + sprintf(displayutf8, "DISPLAY=%.24s", sdlDisplay); + VBOX_UTF8_TO_UTF16(displayutf8, &env); + VIR_FREE(sdlDisplay); + } + + VBOX_UTF8_TO_UTF16("sdl", &sessionType); + } + + if (vrdpPresent) { + VBOX_UTF8_TO_UTF16("vrdp", &sessionType); + } + + rc = data->vboxObj->vtbl->OpenRemoteSession(data->vboxObj, + data->vboxSession, + iid, + sessionType, + env, + &progress ); + if (NS_FAILED(rc)) { + vboxError(VIR_ERR_OPERATION_FAILED, "%s", + _("openremotesession failed, domain can't be started")); + ret = -1; + } else { + PRBool completed = 0; +#if VBOX_API_VERSION == 2002 + nsresult resultCode; +#else + PRInt32 resultCode; +#endif + progress->vtbl->WaitForCompletion(progress, -1); + rc = progress->vtbl->GetCompleted(progress, &completed); + if (NS_FAILED(rc)) { + /* error */ + ret = -1; + } + progress->vtbl->GetResultCode(progress, &resultCode); + if (NS_FAILED(resultCode)) { + /* error */ + ret = -1; + } else { + /* all ok set the domid */ + dom->id = i + 1; + ret = 0; + } + } + + VBOX_RELEASE(progress); + + data->vboxSession->vtbl->Close(data->vboxSession); + + VBOX_UTF16_FREE(env); + VBOX_UTF16_FREE(sessionType); + + return ret; +} + static int vboxDomainCreateWithFlags(virDomainPtr dom, unsigned int flags) { VBOX_OBJECT_CHECK(dom->conn, int, -1); IMachine **machines = NULL; - IProgress *progress = NULL; PRUint32 machineCnt = 0; - PRUnichar *env = NULL; - PRUnichar *sessionType = NULL; - char displayutf8[32] = {0}; unsigned char iidl[VIR_UUID_BUFLEN] = {0}; nsresult rc; int i = 0; @@ -3194,152 +3354,7 @@ static int vboxDomainCreateWithFlags(virDomainPtr dom, unsigned int flags) { if ( (state == MachineState_PoweredOff) || (state == MachineState_Saved) || (state == MachineState_Aborted) ) { - int vrdpPresent = 0; - int sdlPresent = 0; - int guiPresent = 0; - char *guiDisplay = NULL; - char *sdlDisplay = NULL; - PRUnichar *keyTypeUtf16 = NULL; - PRUnichar *valueTypeUtf16 = NULL; - char *valueTypeUtf8 = NULL; - PRUnichar *keyDislpayUtf16 = NULL; - PRUnichar *valueDisplayUtf16 = NULL; - char *valueDisplayUtf8 = NULL; - - VBOX_UTF8_TO_UTF16("FRONTEND/Type", &keyTypeUtf16); - machine->vtbl->GetExtraData(machine, keyTypeUtf16, &valueTypeUtf16); - VBOX_UTF16_FREE(keyTypeUtf16); - - if (valueTypeUtf16) { - VBOX_UTF16_TO_UTF8(valueTypeUtf16, &valueTypeUtf8); - VBOX_UTF16_FREE(valueTypeUtf16); - - if ( STREQ(valueTypeUtf8, "sdl") || STREQ(valueTypeUtf8, "gui") ) { - - VBOX_UTF8_TO_UTF16("FRONTEND/Display", &keyDislpayUtf16); - machine->vtbl->GetExtraData(machine, keyDislpayUtf16, &valueDisplayUtf16); - VBOX_UTF16_FREE(keyDislpayUtf16); - - if (valueDisplayUtf16) { - VBOX_UTF16_TO_UTF8(valueDisplayUtf16, &valueDisplayUtf8); - VBOX_UTF16_FREE(valueDisplayUtf16); - - if (strlen(valueDisplayUtf8) <= 0) { - VBOX_UTF8_FREE(valueDisplayUtf8); - valueDisplayUtf8 = NULL; - } - } - - if (STREQ(valueTypeUtf8, "sdl")) { - sdlPresent = 1; - if (valueDisplayUtf8) { - sdlDisplay = strdup(valueDisplayUtf8); - if (sdlDisplay == NULL) { - virReportOOMError(); - /* just don't go to cleanup yet as it is ok to have - * sdlDisplay as NULL and we check it below if it - * exist and then only use it there - */ - } - } - } - - if (STREQ(valueTypeUtf8, "gui")) { - guiPresent = 1; - if (valueDisplayUtf8) { - guiDisplay = strdup(valueDisplayUtf8); - if (guiDisplay == NULL) { - virReportOOMError(); - /* just don't go to cleanup yet as it is ok to have - * guiDisplay as NULL and we check it below if it - * exist and then only use it there - */ - } - } - } - } - - if (STREQ(valueTypeUtf8, "vrdp")) { - vrdpPresent = 1; - } - - if (!vrdpPresent && !sdlPresent && !guiPresent) { - /* if nothing is selected it means either the machine xml - * file is really old or some values are missing so fallback - */ - guiPresent = 1; - } - - VBOX_UTF8_FREE(valueTypeUtf8); - - } else { - guiPresent = 1; - } - if (valueDisplayUtf8) - VBOX_UTF8_FREE(valueDisplayUtf8); - - if (guiPresent) { - if (guiDisplay) { - sprintf(displayutf8, "DISPLAY=%.24s", guiDisplay); - VBOX_UTF8_TO_UTF16(displayutf8, &env); - VIR_FREE(guiDisplay); - } - - VBOX_UTF8_TO_UTF16("gui", &sessionType); - } - - if (sdlPresent) { - if (sdlDisplay) { - sprintf(displayutf8, "DISPLAY=%.24s", sdlDisplay); - VBOX_UTF8_TO_UTF16(displayutf8, &env); - VIR_FREE(sdlDisplay); - } - - VBOX_UTF8_TO_UTF16("sdl", &sessionType); - } - - if (vrdpPresent) { - VBOX_UTF8_TO_UTF16("vrdp", &sessionType); - } - - rc = data->vboxObj->vtbl->OpenRemoteSession(data->vboxObj, - data->vboxSession, - iid, - sessionType, - env, - &progress ); - if (NS_FAILED(rc)) { - vboxError(VIR_ERR_OPERATION_FAILED, "%s", - _("openremotesession failed, domain can't be started")); - ret = -1; - } else { - PRBool completed = 0; -#if VBOX_API_VERSION == 2002 - nsresult resultCode; -#else - PRInt32 resultCode; -#endif - progress->vtbl->WaitForCompletion(progress, -1); - rc = progress->vtbl->GetCompleted(progress, &completed); - if (NS_FAILED(rc)) { - /* error */ - ret = -1; - } - progress->vtbl->GetResultCode(progress, &resultCode); - if (NS_FAILED(resultCode)) { - /* error */ - ret = -1; - } else { - /* all ok set the domid */ - dom->id = i + 1; - ret = 0; - } - } - - VBOX_RELEASE(progress); - - data->vboxSession->vtbl->Close(data->vboxSession); - + ret = vboxStartMachine(dom, i, machine, iid); } else { vboxError(VIR_ERR_OPERATION_FAILED, "%s", _("machine is not in poweroff|saved|" @@ -3357,9 +3372,6 @@ static int vboxDomainCreateWithFlags(virDomainPtr dom, unsigned int flags) { for (i = 0; i < machineCnt; ++i) VBOX_RELEASE(machines[i]); - VBOX_UTF16_FREE(env); - VBOX_UTF16_FREE(sessionType); - cleanup: return ret; } -- 1.7.2.1
2010/8/19 Eric Blake <eblake@redhat.com>:
* src/vbox/vbox_tmpl.c (vboxDomainCreateWithFlags): Split... (vboxStartMachine): ...into new helper. ---
This function was just too unbearable with that much nested indentation. This should be a no-op refactoring.
Looks fine and I can still start a VirtualBox guest after applying this patch.
vboxDomainDefineXML is even worse, and contains the remaining sprintf instances in this file; I'll probably try to factor that one down as well.
Yes, unfortunately the driver code has many indentation levels and large functions in several places. ACK. Matthias
On 08/19/2010 03:44 PM, Matthias Bolte wrote:
2010/8/19 Eric Blake <eblake@redhat.com>:
* src/vbox/vbox_tmpl.c (vboxDomainCreateWithFlags): Split... (vboxStartMachine): ...into new helper. ---
This function was just too unbearable with that much nested indentation. This should be a no-op refactoring.
Looks fine and I can still start a VirtualBox guest after applying this patch.
vboxDomainDefineXML is even worse, and contains the remaining sprintf instances in this file; I'll probably try to factor that one down as well.
Yes, unfortunately the driver code has many indentation levels and large functions in several places.
ACK.
Thanks; pushed. I'm working on splitting vboxDomainDefineXML before tackling the sprintf uses in this file. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Work in progress: This gets 2 of the 5 suspect uses; I ran out of time today. * src/vbox/vbox_tmpl.c (vboxStartMachine): Use virAsprintf instead. --- I could check this in as is (after tweaking the commit comment) and do the other three sprintf uses in another patch, but I'd rather merge all sprintf changes in this file to a single commit. At any rate, reviewing this portion for any memory leaks now will be helpful. src/vbox/vbox_tmpl.c | 19 ++++++++++++++----- 1 files changed, 14 insertions(+), 5 deletions(-) diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index c28981c..6b9de6c 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -3162,7 +3162,6 @@ vboxStartMachine(virDomainPtr dom, int i, IMachine *machine, vboxIID *iid PRUnichar *valueDisplayUtf16 = NULL; char *valueDisplayUtf8 = NULL; IProgress *progress = NULL; - char displayutf8[32] = {0}; PRUnichar *env = NULL; PRUnichar *sessionType = NULL; nsresult rc; @@ -3242,8 +3241,13 @@ vboxStartMachine(virDomainPtr dom, int i, IMachine *machine, vboxIID *iid if (guiPresent) { if (guiDisplay) { - sprintf(displayutf8, "DISPLAY=%.24s", guiDisplay); - VBOX_UTF8_TO_UTF16(displayutf8, &env); + char *displayutf8; + if (virAsprintf(&displayutf8, "DISPLAY=%s", guiDisplay) < 0) + virReportOOMError(); + else { + VBOX_UTF8_TO_UTF16(displayutf8, &env); + VIR_FREE(displayutf8); + } VIR_FREE(guiDisplay); } @@ -3252,8 +3256,13 @@ vboxStartMachine(virDomainPtr dom, int i, IMachine *machine, vboxIID *iid if (sdlPresent) { if (sdlDisplay) { - sprintf(displayutf8, "DISPLAY=%.24s", sdlDisplay); - VBOX_UTF8_TO_UTF16(displayutf8, &env); + char *displayutf8; + if (virAsprintf(&displayutf8, "DISPLAY=%s", sdlDisplay) < 0) + virReportOOMError(); + else { + VBOX_UTF8_TO_UTF16(displayutf8, &env); + VIR_FREE(displayutf8); + } VIR_FREE(sdlDisplay); } -- 1.7.2.1
2010/8/19 Eric Blake <eblake@redhat.com>:
Work in progress: This gets 2 of the 5 suspect uses; I ran out of time today.
* src/vbox/vbox_tmpl.c (vboxStartMachine): Use virAsprintf instead. ---
I could check this in as is (after tweaking the commit comment) and do the other three sprintf uses in another patch, but I'd rather merge all sprintf changes in this file to a single commit. At any rate, reviewing this portion for any memory leaks now will be helpful.
src/vbox/vbox_tmpl.c | 19 ++++++++++++++----- 1 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index c28981c..6b9de6c 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -3162,7 +3162,6 @@ vboxStartMachine(virDomainPtr dom, int i, IMachine *machine, vboxIID *iid PRUnichar *valueDisplayUtf16 = NULL; char *valueDisplayUtf8 = NULL; IProgress *progress = NULL; - char displayutf8[32] = {0}; PRUnichar *env = NULL; PRUnichar *sessionType = NULL; nsresult rc; @@ -3242,8 +3241,13 @@ vboxStartMachine(virDomainPtr dom, int i, IMachine *machine, vboxIID *iid
if (guiPresent) { if (guiDisplay) { - sprintf(displayutf8, "DISPLAY=%.24s", guiDisplay); - VBOX_UTF8_TO_UTF16(displayutf8, &env); + char *displayutf8; + if (virAsprintf(&displayutf8, "DISPLAY=%s", guiDisplay) < 0) + virReportOOMError();
Why don't we abort the try to start the guest when hitting OOM? Starting the guest will probably fail because of OOM anyway. Also why switch to virAsprintf when snprintf would work too? Matthias
On 08/19/2010 04:14 PM, Matthias Bolte wrote:
if (guiPresent) { if (guiDisplay) { - sprintf(displayutf8, "DISPLAY=%.24s", guiDisplay); - VBOX_UTF8_TO_UTF16(displayutf8, &env); + char *displayutf8; + if (virAsprintf(&displayutf8, "DISPLAY=%s", guiDisplay) < 0) + virReportOOMError();
Why don't we abort the try to start the guest when hitting OOM? Starting the guest will probably fail because of OOM anyway.
The existing code didn't worry about it, but you are probably right that we might as well give up harder on the first OOM.
Also why switch to virAsprintf when snprintf would work too?
Even this sprintf is currently safe (the %.24s fits into the length allocated for guiDisplay), but fragile and arbitrarily limited. That is, the s[n]printf solution locks you into a particular length, and has to be revisited if someone ever has a reason why a 24-character display string is too short (and I can totally see someone complaining that their 25-byte FQDN is a reason to support a longer DISPLAY). At least snprintf only has one place to touch (the array declaration length) instead of two with sprintf (the array declaration and the %.24s in the format string), but virAsprintf gets rid of the length limitation once and for all. But I'll be reposting the patch after refactoring the other three instances in the file, so you can wait for a v2 of this patch. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
2010/8/20 Eric Blake <eblake@redhat.com>:
On 08/19/2010 04:14 PM, Matthias Bolte wrote:
if (guiPresent) { if (guiDisplay) { - sprintf(displayutf8, "DISPLAY=%.24s", guiDisplay); - VBOX_UTF8_TO_UTF16(displayutf8, &env); + char *displayutf8; + if (virAsprintf(&displayutf8, "DISPLAY=%s", guiDisplay) < 0) + virReportOOMError();
Why don't we abort the try to start the guest when hitting OOM? Starting the guest will probably fail because of OOM anyway.
The existing code didn't worry about it, but you are probably right that we might as well give up harder on the first OOM.
Also why switch to virAsprintf when snprintf would work too?
Even this sprintf is currently safe (the %.24s fits into the length allocated for guiDisplay), but fragile and arbitrarily limited. That is, the s[n]printf solution locks you into a particular length, and has to be revisited if someone ever has a reason why a 24-character display string is too short (and I can totally see someone complaining that their 25-byte FQDN is a reason to support a longer DISPLAY). At least snprintf only has one place to touch (the array declaration length) instead of two with sprintf (the array declaration and the %.24s in the format string), but virAsprintf gets rid of the length limitation once and for all.
Ah. Okay I missed that guiDisplay is actually arbitrary input and virAsprintf is better in that case. ACK for using virAsprintf here. Matthias
participants (3)
-
Eric Blake -
Matthias Bolte -
Stefan Berger