3:52 p.m.
It's a good thing the latter while loop condition
could never be true -- otherwise it'd be an infloop.
From 319fd4536555d68316a2cb7967f1093be8de3945 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering(a)redhat.com>
Date: Mon, 17 May 2010 22:50:21 +0200
Subject: [PATCH] (qemuTeardownDiskCgroup): avoid dead code
* src/qemu/qemu_driver.c (qemuTeardownDiskCgroup): Convert
bogus while...while loop to the intended do...while loop.
---
src/qemu/qemu_driver.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 16a9646..3e44407 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2981,65 +2981,65 @@ static int qemuSetupDiskCgroup(virCgroupPtr cgroup,
goto cleanup;
}
}
memset(&meta, 0, sizeof(meta));
rc = virStorageFileGetMetadata(path, &meta);
if (path != disk->src)
VIR_FREE(path);
path = NULL;
if (rc < 0)
goto cleanup;
path = meta.backingStore;
} while (path != NULL);
ret = 0;
cleanup:
return ret;
}
static int qemuTeardownDiskCgroup(virCgroupPtr cgroup,
virDomainObjPtr vm,
virDomainDiskDefPtr disk)
{
char *path = disk->src;
int ret = -1;
- while (path != NULL) {
+ do {
virStorageFileMetadata meta;
int rc;
VIR_DEBUG("Process path %s for disk", path);
rc = virCgroupDenyDevicePath(cgroup, path);
if (rc != 0) {
/* Get this for non-block devices */
if (rc == -EINVAL) {
VIR_DEBUG("Ignoring EINVAL for %s", path);
} else {
virReportSystemError(-rc,
_("Unable to deny device %s for %s"),
path, vm->def->name);
if (path != disk->src)
VIR_FREE(path);
goto cleanup;
}
}
memset(&meta, 0, sizeof(meta));
rc = virStorageFileGetMetadata(path, &meta);
if (path != disk->src)
VIR_FREE(path);
path = NULL;
if (rc < 0)
goto cleanup;
path = meta.backingStore;
} while (path != NULL);
--
1.7.1.250.g7d1e8
3:55 p.m.
Jim Meyering wrote:
It's a good thing the latter while loop condition
could never be true -- otherwise it'd be an infloop.
>From 319fd4536555d68316a2cb7967f1093be8de3945 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering(a)redhat.com>
Date: Mon, 17 May 2010 22:50:21 +0200
Subject: [PATCH] (qemuTeardownDiskCgroup): avoid dead code
* src/qemu/qemu_driver.c (qemuTeardownDiskCgroup): Convert
bogus while...while loop to the intended do...while loop.
Wait a minute...
There's another, just like that:
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 3e44407..3a93418 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2962,7 +2962,7 @@ static int qemuSetupDiskCgroup(virCgroupPtr cgroup,
char *path = disk->src;
int ret = -1;
- while (path != NULL) {
+ do {
virStorageFileMetadata meta;
int rc;
Here's the combined patch:
From 8c53e149c499e0f10d2950b77dd459bd9f85b239 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering(a)redhat.com>
Date: Mon, 17 May 2010 22:50:21 +0200
Subject: [PATCH] (qemu*DiskCgroup): avoid dead code
* src/qemu/qemu_driver.c (qemuTeardownDiskCgroup): Convert
bogus while...while loop to the intended do...while loop.
(qemuSetupDiskCgroup): Likewise.
---
src/qemu/qemu_driver.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 16a9646..3a93418 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2933,113 +2933,113 @@ qemuDomainReAttachHostDevices(struct qemud_driver *driver,
virErrorPtr err = virGetLastError();
VIR_ERROR(_("Failed to reset PCI device: %s"),
err ? err->message : "");
virResetError(err);
}
}
for (i = 0; i < pciDeviceListCount(pcidevs); i++) {
pciDevice *dev = pciDeviceListGet(pcidevs, i);
qemudReattachManagedDevice(dev);
}
pciDeviceListFree(pcidevs);
}
static const char *const defaultDeviceACL[] = {
"/dev/null", "/dev/full", "/dev/zero",
"/dev/random", "/dev/urandom",
"/dev/ptmx", "/dev/kvm", "/dev/kqemu",
"/dev/rtc", "/dev/hpet", "/dev/net/tun",
NULL,
};
#define DEVICE_PTY_MAJOR 136
#define DEVICE_SND_MAJOR 116
static int qemuSetupDiskCgroup(virCgroupPtr cgroup,
virDomainObjPtr vm,
virDomainDiskDefPtr disk)
{
char *path = disk->src;
int ret = -1;
- while (path != NULL) {
+ do {
virStorageFileMetadata meta;
int rc;
VIR_DEBUG("Process path %s for disk", path);
rc = virCgroupAllowDevicePath(cgroup, path);
if (rc != 0) {
/* Get this for non-block devices */
if (rc == -EINVAL) {
VIR_DEBUG("Ignoring EINVAL for %s", path);
} else {
virReportSystemError(-rc,
_("Unable to allow device %s for %s"),
path, vm->def->name);
if (path != disk->src)
VIR_FREE(path);
goto cleanup;
}
}
memset(&meta, 0, sizeof(meta));
rc = virStorageFileGetMetadata(path, &meta);
if (path != disk->src)
VIR_FREE(path);
path = NULL;
if (rc < 0)
goto cleanup;
path = meta.backingStore;
} while (path != NULL);
ret = 0;
cleanup:
return ret;
}
static int qemuTeardownDiskCgroup(virCgroupPtr cgroup,
virDomainObjPtr vm,
virDomainDiskDefPtr disk)
{
char *path = disk->src;
int ret = -1;
- while (path != NULL) {
+ do {
virStorageFileMetadata meta;
int rc;
VIR_DEBUG("Process path %s for disk", path);
rc = virCgroupDenyDevicePath(cgroup, path);
if (rc != 0) {
/* Get this for non-block devices */
if (rc == -EINVAL) {
VIR_DEBUG("Ignoring EINVAL for %s", path);
} else {
virReportSystemError(-rc,
_("Unable to deny device %s for %s"),
path, vm->def->name);
if (path != disk->src)
VIR_FREE(path);
goto cleanup;
}
}
memset(&meta, 0, sizeof(meta));
rc = virStorageFileGetMetadata(path, &meta);
if (path != disk->src)
VIR_FREE(path);
path = NULL;
if (rc < 0)
goto cleanup;
path = meta.backingStore;
} while (path != NULL);
--
1.7.1.250.g7d1e8
4 p.m.
On 05/17/2010 02:52 PM, Jim Meyering wrote:
It's a good thing the latter while loop condition
could never be true -- otherwise it'd be an infloop.
static int qemuTeardownDiskCgroup(virCgroupPtr cgroup,
virDomainObjPtr vm,
virDomainDiskDefPtr disk)
{
char *path = disk->src;
int ret = -1;
- while (path != NULL) {
+ do {
virStorageFileMetadata meta;
int rc;
VIR_DEBUG("Process path %s for disk", path);
...
path = meta.backingStore;
} while (path != NULL);
Are we sure disk->src is guaranteed to be non-NULL on entry, or would
have been better to rewrite this as while{}/*nothing*/ instead of do{}while?
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
12:40 a.m.
Eric Blake wrote:
On 05/17/2010 02:52 PM, Jim Meyering wrote:
> It's a good thing the latter while loop condition
> could never be true -- otherwise it'd be an infloop.
>
> static int qemuTeardownDiskCgroup(virCgroupPtr cgroup,
> virDomainObjPtr vm,
> virDomainDiskDefPtr disk)
> {
> char *path = disk->src;
> int ret = -1;
>
> - while (path != NULL) {
> + do {
> virStorageFileMetadata meta;
> int rc;
>
> VIR_DEBUG("Process path %s for disk", path);
...
> path = meta.backingStore;
> } while (path != NULL);
Are we sure disk->src is guaranteed to be non-NULL on entry, or would
have been better to rewrite this as while{}/*nothing*/ instead of do{}while?
You're right.
disk->src may be NULL, as attested by surrounding code that tests for it
via NULLSTR upon failure of these functions.
Thanks!
Here's a better patch:
From 9fccf57a1c786612048964d7c5a54f5fc5e85d56 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering(a)redhat.com>
Date: Mon, 17 May 2010 22:50:21 +0200
Subject: [PATCH] (qemu*DiskCgroup): avoid dead code
* src/qemu/qemu_driver.c (qemuTeardownDiskCgroup): Remove
bogus empty-body while-loop.
(qemuSetupDiskCgroup): Likewise.
---
src/qemu/qemu_driver.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 16a9646..c29392d 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2992,11 +2992,11 @@ static int qemuSetupDiskCgroup(virCgroupPtr cgroup,
if (rc < 0)
goto cleanup;
path = meta.backingStore;
- } while (path != NULL);
+ }
ret = 0;
cleanup:
return ret;
@@ -3040,11 +3040,11 @@ static int qemuTeardownDiskCgroup(virCgroupPtr cgroup,
if (rc < 0)
goto cleanup;
path = meta.backingStore;
- } while (path != NULL);
+ }
ret = 0;
cleanup:
return ret;
--
1.7.1.250.g7d1e8
9:20 a.m.
On 05/17/2010 11:40 PM, Jim Meyering wrote:
Eric Blake wrote:
> On 05/17/2010 02:52 PM, Jim Meyering wrote:
>> It's a good thing the latter while loop condition
>> could never be true -- otherwise it'd be an infloop.
>>
>> char *path = disk->src;
>> int ret = -1;
>>
>> - while (path != NULL) {
>> + do {
>> virStorageFileMetadata meta;
>> int rc;
>>
>> VIR_DEBUG("Process path %s for disk", path);
> ...
>> path = meta.backingStore;
>> } while (path != NULL);
>
> Are we sure disk->src is guaranteed to be non-NULL on entry, or would
> have been better to rewrite this as while{}/*nothing*/ instead of do{}while?
You're right.
disk->src may be NULL, as attested by surrounding code that tests for it
via NULLSTR upon failure of these functions.
Thanks!
Here's a better patch:
ACK to v2.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
5303
days inactive
5304
days old
4 comments
2 participants
participants (2)
-
Eric Blake -
Jim Meyering