[libvirt] [PATCH] (qemuTeardownDiskCgroup): avoid dead code
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@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
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@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@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
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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@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
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Jim Meyering