6:45 p.m.
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
6:45 p.m.
New subject: [libvirt] [PATCH 01/10] uml: fix logic bug in checking reply length
* 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
3:47 p.m.
New subject: [libvirt] [PATCH 01/10] uml: fix logic bug in checking reply length
2010/8/19 Eric Blake <eblake(a)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
3:52 p.m.
New subject: [libvirt] [PATCH 01/10] uml: fix logic bug in checking reply length
On 08/19/2010 02:47 PM, Matthias Bolte wrote:
2010/8/19 Eric Blake <eblake(a)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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
6:45 p.m.
New subject: [libvirt] [PATCH 02/10] nwfilter: use consistent OOM reporting
* 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
1:41 p.m.
New subject: [libvirt] [PATCH 02/10] nwfilter: use consistent OOM reporting
libvir-list-bounces(a)redhat.com wrote on 08/18/2010 07:45:05 PM:
libvir-list-bounces(a)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
2:21 p.m.
New subject: [libvirt] [PATCH 02/10] nwfilter: use consistent OOM reporting
On 08/19/2010 12:41 PM, Stefan Berger wrote:
libvir-list-bounces(a)redhat.com wrote on 08/18/2010 07:45:05 PM:
> libvir-list-bounces(a)redhat.com
>
> * src/nwfilter/nwfilter_driver.c (nwfilterLog): Delete.
> (nwfilterDriverStartup): Use virReportOOMError instead.
ACK.
Thanks; pushed.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
6:45 p.m.
New subject: [libvirt] [PATCH 03/10] build: delete dead comment
* 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
3:53 p.m.
New subject: [libvirt] [PATCH 03/10] build: delete dead comment
2010/8/19 Eric Blake <eblake(a)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
6:45 p.m.
New subject: [libvirt] [PATCH 04/10] maint: whitespace cleanups
* 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
3:59 p.m.
New subject: [libvirt] [PATCH 04/10] maint: whitespace cleanups
2010/8/19 Eric Blake <eblake(a)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
5:26 p.m.
New subject: [libvirt] [PATCH 04/10] maint: whitespace cleanups
On 08/19/2010 02:59 PM, Matthias Bolte wrote:
2010/8/19 Eric Blake <eblake(a)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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
6:45 p.m.
New subject: [libvirt] [PATCH 05/10] storage: avoid s[n]printf
* 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
4:10 p.m.
New subject: [libvirt] [PATCH 05/10] storage: avoid s[n]printf
2010/8/19 Eric Blake <eblake(a)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
5:26 p.m.
New subject: [libvirt] [PATCH 05/10] storage: avoid s[n]printf
On 08/19/2010 03:10 PM, Matthias Bolte wrote:
2010/8/19 Eric Blake <eblake(a)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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
6:45 p.m.
New subject: [libvirt] [PATCH 06/10] squash to dead comment
* 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
4:11 p.m.
New subject: [libvirt] [PATCH 06/10] squash to dead comment
2010/8/19 Eric Blake <eblake(a)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
5:26 p.m.
New subject: [libvirt] [PATCH 06/10] squash to dead comment
On 08/19/2010 03:11 PM, Matthias Bolte wrote:
2010/8/19 Eric Blake <eblake(a)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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
6:45 p.m.
New subject: [libvirt] [PATCH 07/10] xenapi: avoid sprintf
* 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
4:14 p.m.
New subject: [libvirt] [PATCH 07/10] xenapi: avoid sprintf
2010/8/19 Eric Blake <eblake(a)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
5:26 p.m.
New subject: [libvirt] [PATCH 07/10] xenapi: avoid sprintf
On 08/19/2010 03:14 PM, Matthias Bolte wrote:
2010/8/19 Eric Blake <eblake(a)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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
6:45 p.m.
New subject: [libvirt] [PATCH 08/10] vbox: add location used in rpmfusion release
* 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
4:17 p.m.
New subject: [libvirt] [PATCH 08/10] vbox: add location used in rpmfusion release
2010/8/19 Eric Blake <eblake(a)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
5:26 p.m.
New subject: [libvirt] [PATCH 08/10] vbox: add location used in rpmfusion release
On 08/19/2010 03:17 PM, Matthias Bolte wrote:
2010/8/19 Eric Blake <eblake(a)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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
6:45 p.m.
New subject: [libvirt] [PATCH 09/10] vbox: factor a large function
* 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
4:44 p.m.
New subject: [libvirt] [PATCH 09/10] vbox: factor a large function
2010/8/19 Eric Blake <eblake(a)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
6:24 p.m.
New subject: [libvirt] [PATCH 09/10] vbox: factor a large function
On 08/19/2010 03:44 PM, Matthias Bolte wrote:
2010/8/19 Eric Blake <eblake(a)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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
6:45 p.m.
New subject: [libvirt] [PATCH 10/10] vbox: avoid sprintf
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
5:14 p.m.
New subject: [libvirt] [PATCH 10/10] vbox: avoid sprintf
2010/8/19 Eric Blake <eblake(a)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
6:30 p.m.
New subject: [libvirt] [PATCH 10/10] vbox: avoid sprintf
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
4:16 a.m.
New subject: [libvirt] [PATCH 10/10] vbox: avoid sprintf
2010/8/20 Eric Blake <eblake(a)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
5212
days inactive
5214
days old
30 comments
3 participants
participants (3)
-
Eric Blake -
Matthias Bolte -
Stefan Berger