[libvirt] [PATCH] Allow relative path for qemu backing file
This patch enables the relative backing file path support provided by qemu-img create. If a relative path is specified for the backing file, it is converted to an absolute path using the storage pool path. The absolute path is used to verify that the backing file exists. If the backing file exists, the relative path is allowed and will be provided to qemu-img create. This patch takes the place of a previous patch: http://www.redhat.com/archives/libvir-list/2011-March/msg00179.html --- src/storage/storage_backend.c | 18 +++++++++++++++++- 1 files changed, 17 insertions(+), 1 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 2eede74..abdc2dd 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -666,6 +666,9 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, } if (vol->backingStore.path) { + int accessRetCode = -1; + + char *absolutePath = NULL; /* XXX: Not strictly required: qemu-img has an option a different * backing store, not really sure what use it serves though, and it @@ -686,7 +689,20 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, vol->backingStore.format); return -1; } - if (access(vol->backingStore.path, R_OK) != 0) { + + /* Convert relative backing store paths to absolute paths for access + * validation. + */ + if ('/' != *(vol->backingStore.path)) { + virAsprintf(&absolutePath, "%s/%s", pool->def->target.path, + vol->backingStore.path); + + } else { + virAsprintf(&absolutePath, "%s", vol->backingStore.path); + } + accessRetCode = access(absolutePath, R_OK); + VIR_FREE(absolutePath); + if (accessRetCode != 0) { virReportSystemError(errno, _("inaccessible backing store volume %s"), vol->backingStore.path); -- 1.7.4.1
On 03/27/2011 07:30 PM, Jesse Cook wrote:
This patch enables the relative backing file path support provided by qemu-img create.
If a relative path is specified for the backing file, it is converted to an absolute path using the storage pool path. The absolute path is used to verify that the backing file exists. If the backing file exists, the relative path is allowed and will be provided to qemu-img create.
This patch takes the place of a previous patch: http://www.redhat.com/archives/libvir-list/2011-March/msg00179.html --- src/storage/storage_backend.c | 18 +++++++++++++++++- 1 files changed, 17 insertions(+), 1 deletions(-)
Sorry for not reviewing this in time for 0.9.0.
@@ -686,7 +689,20 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, vol->backingStore.format); return -1; } - if (access(vol->backingStore.path, R_OK) != 0) { + + /* Convert relative backing store paths to absolute paths for access + * validation. + */ + if ('/' != *(vol->backingStore.path)) { + virAsprintf(&absolutePath, "%s/%s", pool->def->target.path, + vol->backingStore.path); + + } else { + virAsprintf(&absolutePath, "%s", vol->backingStore.path);
strdup is more efficient here, and avoiding malloc in the first place even more so.
+ } + accessRetCode = access(absolutePath, R_OK);
This could segfault on OOM.
+ VIR_FREE(absolutePath); + if (accessRetCode != 0) { virReportSystemError(errno, _("inaccessible backing store volume %s"), vol->backingStore.path);
ACK with nits fixed; so here's what I squashed in before pushing. I also updated AUTHORS to pass 'make syntax-check'; feel free to let me know off-list if you prefer any alternate spelling there (especially since you sent v1 and v2 under different email aliases). diff --git i/src/storage/storage_backend.c w/src/storage/storage_backend.c index 5f5565b..8af2878 100644 --- i/src/storage/storage_backend.c +++ w/src/storage/storage_backend.c @@ -693,7 +693,6 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, if (vol->backingStore.path) { int accessRetCode = -1; - char *absolutePath = NULL; /* XXX: Not strictly required: qemu-img has an option a different @@ -719,14 +718,14 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, /* Convert relative backing store paths to absolute paths for access * validation. */ - if ('/' != *(vol->backingStore.path)) { + if ('/' != *(vol->backingStore.path) && virAsprintf(&absolutePath, "%s/%s", pool->def->target.path, - vol->backingStore.path); - - } else { - virAsprintf(&absolutePath, "%s", vol->backingStore.path); + vol->backingStore.path) < 0) { + virReportOOMError(); + return -1; } - accessRetCode = access(absolutePath, R_OK); + accessRetCode = access(absolutePath ? absolutePath + : vol->backingStore.path, R_OK); VIR_FREE(absolutePath); if (accessRetCode != 0) { virReportSystemError(errno, -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Mon, 2011-04-04 at 16:39 -0600, Eric Blake wrote:
On 03/27/2011 07:30 PM, Jesse Cook wrote:
This patch enables the relative backing file path support provided by qemu-img create.
If a relative path is specified for the backing file, it is converted to an absolute path using the storage pool path. The absolute path is used to verify that the backing file exists. If the backing file exists, the relative path is allowed and will be provided to qemu-img create.
This patch takes the place of a previous patch: http://www.redhat.com/archives/libvir-list/2011-March/msg00179.html --- src/storage/storage_backend.c | 18 +++++++++++++++++- 1 files changed, 17 insertions(+), 1 deletions(-)
Sorry for not reviewing this in time for 0.9.0.
No Problem. It took me a while to get around to the fix.
@@ -686,7 +689,20 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, vol->backingStore.format); return -1; } - if (access(vol->backingStore.path, R_OK) != 0) { + + /* Convert relative backing store paths to absolute paths for access + * validation. + */ + if ('/' != *(vol->backingStore.path)) { + virAsprintf(&absolutePath, "%s/%s", pool->def->target.path, + vol->backingStore.path); + + } else { + virAsprintf(&absolutePath, "%s", vol->backingStore.path);
strdup is more efficient here, and avoiding malloc in the first place even more so.
+ } + accessRetCode = access(absolutePath, R_OK);
This could segfault on OOM.
+ VIR_FREE(absolutePath);
I believe there needs to be a NULL check here or absolute paths and virAsprintf errors will segfault. I can patch if you don't beat me to it.
+ if (accessRetCode != 0) { virReportSystemError(errno, _("inaccessible backing store volume %s"), vol->backingStore.path);
ACK with nits fixed; so here's what I squashed in before pushing. I also updated AUTHORS to pass 'make syntax-check'; feel free to let me know off-list if you prefer any alternate spelling there (especially since you sent v1 and v2 under different email aliases).
diff --git i/src/storage/storage_backend.c w/src/storage/storage_backend.c index 5f5565b..8af2878 100644 --- i/src/storage/storage_backend.c +++ w/src/storage/storage_backend.c @@ -693,7 +693,6 @@ virStorageBackendCreateQemuImg(virConnectPtr conn,
if (vol->backingStore.path) { int accessRetCode = -1; - char *absolutePath = NULL;
/* XXX: Not strictly required: qemu-img has an option a different @@ -719,14 +718,14 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, /* Convert relative backing store paths to absolute paths for access * validation. */ - if ('/' != *(vol->backingStore.path)) { + if ('/' != *(vol->backingStore.path) && virAsprintf(&absolutePath, "%s/%s", pool->def->target.path, - vol->backingStore.path); - - } else { - virAsprintf(&absolutePath, "%s", vol->backingStore.path); + vol->backingStore.path) < 0) { + virReportOOMError(); + return -1; } - accessRetCode = access(absolutePath, R_OK); + accessRetCode = access(absolutePath ? absolutePath + : vol->backingStore.path, R_OK); VIR_FREE(absolutePath); if (accessRetCode != 0) { virReportSystemError(errno,
-- Jesse Cook Research Scientist EADS NA Defense Security & Systems Solutions, Inc. (DS3) Email: jesse.cook@eads-na-security.com
On 04/05/2011 10:55 AM, Jesse J Cook wrote:
+ if ('/' != *(vol->backingStore.path)) { + virAsprintf(&absolutePath, "%s/%s", pool->def->target.path, + vol->backingStore.path); + + } else { + virAsprintf(&absolutePath, "%s", vol->backingStore.path);
strdup is more efficient here, and avoiding malloc in the first place even more so.
+ } + accessRetCode = access(absolutePath, R_OK);
This could segfault on OOM.
+ VIR_FREE(absolutePath);
I believe there needs to be a NULL check here or absolute paths and virAsprintf errors will segfault. I can patch if you don't beat me to it.
How so? absolutePath was initialized as NULL; is only ever set to non-null by a successful virAsprintf, and VIR_FREE works correctly (no-op) on a NULL argument. Put another way, are you missing that VIR_FREE already has an embedded NULL check? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Tue, 2011-04-05 at 11:00 -0600, Eric Blake wrote:
On 04/05/2011 10:55 AM, Jesse J Cook wrote:
+ if ('/' != *(vol->backingStore.path)) { + virAsprintf(&absolutePath, "%s/%s", pool->def->target.path, + vol->backingStore.path); + + } else { + virAsprintf(&absolutePath, "%s", vol->backingStore.path);
strdup is more efficient here, and avoiding malloc in the first place even more so.
+ } + accessRetCode = access(absolutePath, R_OK);
This could segfault on OOM.
+ VIR_FREE(absolutePath);
I believe there needs to be a NULL check here or absolute paths and virAsprintf errors will segfault. I can patch if you don't beat me to it.
Disregard. The code is correct.
How so? absolutePath was initialized as NULL; is only ever set to non-null by a successful virAsprintf, and VIR_FREE works correctly (no-op) on a NULL argument. Put another way, are you missing that VIR_FREE already has an embedded NULL check?
Thank you for resolving my fundamental misunderstanding regarding the behaviour of free(3). I did not realize free((void*)0) is safe. -- Jesse Cook Research Scientist EADS NA Defense Security & Systems Solutions, Inc. (DS3) 1476 N. Green Mount Rd O'Fallon, Illinois 62269 Office: 618.206.4032 x436 Email: jesse.cook@eads-na-security.com http://www.eads-na-security.com
participants (3)
-
Eric Blake -
Jesse Cook -
Jesse J Cook