[libvirt] [PATCH] qemu: Search for disk in qemuDomainGetBlockInfo
The commit 89b6284fd94ce5b13ee6b002f9167f5d9074aa7a caused regression. Although we now allow users to input e.g. 'hda' instead of whole path, we still need to search for appropriate disk in VM definition. --- src/qemu/qemu_driver.c | 16 ++++++++++++++++ 1 files changed, 16 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1925ba5..fec4eeb 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7755,6 +7755,7 @@ static int qemuDomainGetBlockInfo(virDomainPtr dom, virStorageFileMetadata *meta = NULL; virDomainDiskDefPtr disk = NULL; struct stat sb; + int i; int format; const char *actual; @@ -7785,6 +7786,21 @@ static int qemuDomainGetBlockInfo(virDomainPtr dom, } path = actual; + /* Check the path belongs to this domain. */ + for (i = 0 ; i < vm->def->ndisks ; i++) { + if (vm->def->disks[i]->src != NULL && + STREQ (vm->def->disks[i]->src, path)) { + disk = vm->def->disks[i]; + break; + } + } + + if (!disk) { + qemuReportError(VIR_ERR_INVALID_ARG, + _("invalid path %s not assigned to domain"), path); + goto cleanup; + } + /* The path is correct, now try to open it and get its size. */ fd = open(path, O_RDONLY); if (fd == -1) { -- 1.7.3.4
On 09/08/2011 09:55 AM, Michal Privoznik wrote:
The commit 89b6284fd94ce5b13ee6b002f9167f5d9074aa7a caused regression. Although we now allow users to input e.g. 'hda' instead of whole path, we still need to search for appropriate disk in VM definition. --- src/qemu/qemu_driver.c | 16 ++++++++++++++++ 1 files changed, 16 insertions(+), 0 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1925ba5..fec4eeb 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7755,6 +7755,7 @@ static int qemuDomainGetBlockInfo(virDomainPtr dom, virStorageFileMetadata *meta = NULL; virDomainDiskDefPtr disk = NULL; struct stat sb; + int i; int format; const char *actual;
@@ -7785,6 +7786,21 @@ static int qemuDomainGetBlockInfo(virDomainPtr dom, } path = actual;
+ /* Check the path belongs to this domain. */ + for (i = 0 ; i< vm->def->ndisks ; i++) { + if (vm->def->disks[i]->src != NULL&& + STREQ (vm->def->disks[i]->src, path)) { + disk = vm->def->disks[i]; + break; + } + }
NACK. This is open-coding the effects of virDomainDiskIndexByName(). Instead, we should be doing something like: int i = virDomainDiskIndexByName(vm->def, path, false); if (i < 0) error; disk = vm->def->disks[i]; instead of virDomainDiskPathByName(). -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Regression introduced in commit 89b6284fd, due to an incorrect conversion to the new means of converting disk names back to the correct object. * src/qemu/qemu_driver.c (qemuDomainGetBlockInfo): Avoid NULL deref. --- src/qemu/qemu_driver.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1925ba5..f73270f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7755,8 +7755,8 @@ static int qemuDomainGetBlockInfo(virDomainPtr dom, virStorageFileMetadata *meta = NULL; virDomainDiskDefPtr disk = NULL; struct stat sb; + int i; int format; - const char *actual; virCheckFlags(0, -1); @@ -7778,12 +7778,12 @@ static int qemuDomainGetBlockInfo(virDomainPtr dom, } /* Check the path belongs to this domain. */ - if (!(actual = virDomainDiskPathByName(vm->def, path))) { + if ((i = virDomainDiskIndexByName(vm->def, path, false)) < 0) { qemuReportError(VIR_ERR_INVALID_ARG, _("invalid path %s not assigned to domain"), path); goto cleanup; } - path = actual; + disk = vm->def->disks[i]; /* The path is correct, now try to open it and get its size. */ fd = open(path, O_RDONLY); -- 1.7.4.4
On 08.09.2011 11:12, Eric Blake wrote:
Regression introduced in commit 89b6284fd, due to an incorrect conversion to the new means of converting disk names back to the correct object.
* src/qemu/qemu_driver.c (qemuDomainGetBlockInfo): Avoid NULL deref. --- src/qemu/qemu_driver.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1925ba5..f73270f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7755,8 +7755,8 @@ static int qemuDomainGetBlockInfo(virDomainPtr dom, virStorageFileMetadata *meta = NULL; virDomainDiskDefPtr disk = NULL; struct stat sb; + int i; int format; - const char *actual;
virCheckFlags(0, -1);
@@ -7778,12 +7778,12 @@ static int qemuDomainGetBlockInfo(virDomainPtr dom, }
/* Check the path belongs to this domain. */ - if (!(actual = virDomainDiskPathByName(vm->def, path))) { + if ((i = virDomainDiskIndexByName(vm->def, path, false)) < 0) { qemuReportError(VIR_ERR_INVALID_ARG, _("invalid path %s not assigned to domain"), path); goto cleanup; } - path = actual; + disk = vm->def->disks[i];
/* The path is correct, now try to open it and get its size. */ fd = open(path, O_RDONLY);
In fact, we need to update path as well, beacuse we use it later (as can be seen in context. but that is not the only place). so ACK with this nit fixed. Michal
On 09/08/2011 10:29 AM, Michal Privoznik wrote:
On 08.09.2011 11:12, Eric Blake wrote:
Regression introduced in commit 89b6284fd, due to an incorrect conversion to the new means of converting disk names back to the correct object.
- path = actual; + disk = vm->def->disks[i];
/* The path is correct, now try to open it and get its size. */ fd = open(path, O_RDONLY);
In fact, we need to update path as well, beacuse we use it later (as can be seen in context. but that is not the only place).
Ouch; you're right. I'm squashing this in: diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index f73270f..b94d1c4 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -7784,6 +7784,13 @@ static int qemuDomainGetBlockInfo(virDomainPtr dom, goto cleanup; } disk = vm->def->disks[i]; + if (!disk->src) { + qemuReportError(VIR_ERR_INVALID_ARG, + _("disk %s does not currently have a source assigned"), + path); + goto cleanup; + } + path = disk->src; /* The path is correct, now try to open it and get its size. */ fd = open(path, O_RDONLY);
so ACK with this nit fixed.
and pushing. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Michal Privoznik