On Wed, 2011-03-09 at 16:07 +0000, Daniel P. Berrange wrote:

On Thu, Mar 03, 2011 at 09:06:25PM -0600, Jesse Cook wrote:

...

This patch enables the relative backing file path support provided by qemu-img create.

If the storage pool is not found with the specified path, check if the file exists relative to the pool where the new image will be created by prepending the storage pool path. --- src/storage/storage_backend.c | 32 ++++++++++++++++++++++++++++---- 1 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 2eede74..bb49f22 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -687,10 +687,34 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, return -1; } if (access(vol->backingStore.path, R_OK) != 0) { - virReportSystemError(errno, - _("inaccessible backing store volume %s"), - vol->backingStore.path); - return -1; + /* If the backing store image is not found with the specified path, + * check for the file relative to the pool path. */ + int accessRetCode = -1; + + char *absolutePath = NULL; + + virBuffer absPathBuf = VIR_BUFFER_INITIALIZER; + + virBufferVSprintf(&absPathBuf, + "%s/%s", + pool->def->target.path, + vol->backingStore.path); + + if (virBufferError(&absPathBuf)) { + virBufferFreeAndReset(&absPathBuf); + virReportOOMError(); + return -1; + } + + absolutePath = virBufferContentAndReset(&absPathBuf);

Since you're only doing one single virBufferVSprintf() call, using virBuffer is overkill. You can get away with the simpler

virAsprintf(&absolutePath, "%s/%s", pool->def->target.path, vol->backingStore.path);

Thank you. I will incorporate that into the new patch.

...

+ accessRetCode = access(absolutePath, R_OK); + VIR_FREE(absolutePath); + if (accessRetCode != 0) { + virReportSystemError(errno, + _("inaccessible backing store volume %s"), + vol->backingStore.path); + return -1; + } } }

Regards, Daniel

-- 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

On Wed, Mar 09, 2011 at 09:23:11AM -0700, Eric Blake wrote:

On 03/03/2011 08:06 PM, Jesse Cook wrote:

...

This patch enables the relative backing file path support provided by qemu-img create.

If the storage pool is not found with the specified path, check if the file exists relative to the pool where the new image will be created by prepending the storage pool path. --- src/storage/storage_backend.c | 32 ++++++++++++++++++++++++++++---- 1 files changed, 28 insertions(+), 4 deletions(-)

@@ -687,10 +687,34 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, return -1; } if (access(vol->backingStore.path, R_OK) != 0) { - virReportSystemError(errno, - _("inaccessible backing store volume %s"), - vol->backingStore.path); - return -1; + /* If the backing store image is not found with the specified path, + * check for the file relative to the pool path. */ + int accessRetCode = -1; + + char *absolutePath = NULL; + + virBuffer absPathBuf = VIR_BUFFER_INITIALIZER; + + virBufferVSprintf(&absPathBuf, + "%s/%s", + pool->def->target.path, + vol->backingStore.path);

I agree that we need to handle relative paths, but are they relative to the directory that owns the original qcow2 image, or are they relative to the cwd of the qemu process (not necessarily the same directory)? We need to make sure that we have the same interpretation of relative paths as qemu.

"Relative to the cwd of the qemu process" would be unworkably broken, because given an image file on its own, it would be impossible to resolve its backing store. Also every QEMU instance is run with cwd==/ currently. NB, in src/util/storage_file.c, we resolve backing store relative to the image which points to it, which is the same as resolving relative to the pool in this scenario

Meanwhile, shouldn't you only be computing an alternate name if vol->backingStore.path is relative? If it's absolute, then you would be generating /path/to/pool//path/to/backing, which is the wrong file compared to what qemu would be using.

Yep, it should check for a leading '/' before trying to resolve it. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|