[libvirt] [PATCH 0/4] fix yet another regression due to my backingChain rewrite
https://bugzilla.redhat.com/show_bug.cgi?id=903248 details a regression that I caused, which is tickled by the odd way in which VDSM uses (abuses?) symlinks to qcow2 files with relative backing file names. The first two patches are quite trivial; the third a bit longer but still straightforward; and the meat of the series is the fourth patch. Meanwhile, I know of at least one other regression that I still need to fix in the same area of code: https://bugzilla.redhat.com/show_bug.cgi?id=896685 is caused when cgroups are not present (such as in qemu:///session); I hope to patch that one tomorrow. Eric Blake (4): storage: factor out large integer reads storage: rearrange functions storage: refactor metadata lookup storage: don't follow backing chain symlinks too eagerly src/util/virstoragefile.c | 475 +++++++++++++++++++++++----------------------- src/util/virstoragefile.h | 1 + 2 files changed, 240 insertions(+), 236 deletions(-) -- 1.8.1
This makes code easier to read, by avoiding lines longer than 80 columns and removing the repetition from the callers. * src/util/virstoragefile.c (virRead64BE, virRead64LE) (virRead32BE, virRead32LE): New helper functions. (qedGetHeaderUL, qedGetHeaderULL): Delete in favor of more generic name. (qcow2GetBackingStoreFormat, qcowXGetBackingStore) (qedGetBackingStore, virStorageFileMatchesVersion) (virStorageFileGetMetadataInternal): Use them. --- src/util/virstoragefile.c | 159 +++++++++++++++++++++------------------------- 1 file changed, 72 insertions(+), 87 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 6e2d61e..e7ab226 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -208,6 +208,58 @@ static struct FileTypeInfo const fileTypeInfo[] = { }; verify(ARRAY_CARDINALITY(fileTypeInfo) == VIR_STORAGE_FILE_LAST); +/* Read 8 bytes at BUF as a big-endian 64-bit number. Caller is + responsible to avoid reading beyond array bounds. */ +static unsigned long long +virRead64BE(const unsigned char *buf) +{ + return (((uint64_t)buf[0] << 56) | + ((uint64_t)buf[1] << 48) | + ((uint64_t)buf[2] << 40) | + ((uint64_t)buf[3] << 32) | + ((uint64_t)buf[4] << 24) | + ((uint64_t)buf[5] << 16) | + ((uint64_t)buf[6] << 8) | + (uint64_t)buf[7]); +} + +/* Read 8 bytes at BUF as a little-endian 64-bit number. Caller is + responsible to avoid reading beyond array bounds. */ +static unsigned long long +virRead64LE(const unsigned char *buf) +{ + return ((uint64_t)buf[0] | + ((uint64_t)buf[1] << 8) | + ((uint64_t)buf[2] << 16) | + ((uint64_t)buf[3] << 24) | + ((uint64_t)buf[4] << 32) | + ((uint64_t)buf[5] << 40) | + ((uint64_t)buf[6] << 48) | + ((uint64_t)buf[7] << 56)); +} + +/* Read 4 bytes at BUF as a big-endian 32-bit number. Caller is + responsible to avoid reading beyond array bounds. */ +static unsigned int +virRead32BE(const unsigned char *buf) +{ + return ((buf[0] << 24) | + (buf[1] << 16) | + (buf[2] << 8) | + buf[3]); +} + +/* Read 4 bytes at BUF as a little-endian 32-bit number. Caller is + responsible to avoid reading beyond array bounds. */ +static unsigned int +virRead32LE(const unsigned char *buf) +{ + return (buf[0] | + (buf[1] << 8) | + (buf[2] << 16) | + (buf[3] << 24)); +} + static int cowGetBackingStore(char **res, int *format, @@ -255,16 +307,8 @@ qcow2GetBackingStoreFormat(int *format, */ while (offset < (buf_size-8) && offset < (extension_end-8)) { - unsigned int magic = - (buf[offset] << 24) + - (buf[offset+1] << 16) + - (buf[offset+2] << 8) + - (buf[offset+3]); - unsigned int len = - (buf[offset+4] << 24) + - (buf[offset+5] << 16) + - (buf[offset+6] << 8) + - (buf[offset+7]); + unsigned int magic = virRead32BE(buf + offset); + unsigned int len = virRead32BE(buf + offset + 4); offset += 8; @@ -312,20 +356,10 @@ qcowXGetBackingStore(char **res, if (buf_size < QCOWX_HDR_BACKING_FILE_OFFSET+8+4) return BACKING_STORE_INVALID; - offset = (((unsigned long long)buf[QCOWX_HDR_BACKING_FILE_OFFSET] << 56) - | ((unsigned long long)buf[QCOWX_HDR_BACKING_FILE_OFFSET+1] << 48) - | ((unsigned long long)buf[QCOWX_HDR_BACKING_FILE_OFFSET+2] << 40) - | ((unsigned long long)buf[QCOWX_HDR_BACKING_FILE_OFFSET+3] << 32) - | ((unsigned long long)buf[QCOWX_HDR_BACKING_FILE_OFFSET+4] << 24) - | ((unsigned long long)buf[QCOWX_HDR_BACKING_FILE_OFFSET+5] << 16) - | ((unsigned long long)buf[QCOWX_HDR_BACKING_FILE_OFFSET+6] << 8) - | buf[QCOWX_HDR_BACKING_FILE_OFFSET+7]); /* QCowHeader.backing_file_offset */ + offset = virRead64BE(buf + QCOWX_HDR_BACKING_FILE_OFFSET); if (offset > buf_size) return BACKING_STORE_INVALID; - size = ((buf[QCOWX_HDR_BACKING_FILE_SIZE] << 24) - | (buf[QCOWX_HDR_BACKING_FILE_SIZE+1] << 16) - | (buf[QCOWX_HDR_BACKING_FILE_SIZE+2] << 8) - | buf[QCOWX_HDR_BACKING_FILE_SIZE+3]); /* QCowHeader.backing_file_size */ + size = virRead32BE(buf + QCOWX_HDR_BACKING_FILE_SIZE); if (size == 0) { if (format) *format = VIR_STORAGE_FILE_NONE; @@ -468,28 +502,6 @@ cleanup: return ret; } -static unsigned long -qedGetHeaderUL(const unsigned char *loc) -{ - return (((unsigned long)loc[3] << 24) | - ((unsigned long)loc[2] << 16) | - ((unsigned long)loc[1] << 8) | - ((unsigned long)loc[0] << 0)); -} - -static unsigned long long -qedGetHeaderULL(const unsigned char *loc) -{ - return (((unsigned long long)loc[7] << 56) | - ((unsigned long long)loc[6] << 48) | - ((unsigned long long)loc[5] << 40) | - ((unsigned long long)loc[4] << 32) | - ((unsigned long long)loc[3] << 24) | - ((unsigned long long)loc[2] << 16) | - ((unsigned long long)loc[1] << 8) | - ((unsigned long long)loc[0] << 0)); -} - static int qedGetBackingStore(char **res, int *format, @@ -503,7 +515,7 @@ qedGetBackingStore(char **res, /* Check if this image has a backing file */ if (buf_size < QED_HDR_FEATURES_OFFSET+8) return BACKING_STORE_INVALID; - flags = qedGetHeaderULL(buf + QED_HDR_FEATURES_OFFSET); + flags = virRead64LE(buf + QED_HDR_FEATURES_OFFSET); if (!(flags & QED_F_BACKING_FILE)) { *format = VIR_STORAGE_FILE_NONE; return BACKING_STORE_OK; @@ -512,10 +524,10 @@ qedGetBackingStore(char **res, /* Parse the backing file */ if (buf_size < QED_HDR_BACKING_FILE_OFFSET+8) return BACKING_STORE_INVALID; - offset = qedGetHeaderUL(buf + QED_HDR_BACKING_FILE_OFFSET); + offset = virRead32LE(buf + QED_HDR_BACKING_FILE_OFFSET); if (offset > buf_size) return BACKING_STORE_INVALID; - size = qedGetHeaderUL(buf + QED_HDR_BACKING_FILE_SIZE); + size = virRead32LE(buf + QED_HDR_BACKING_FILE_SIZE); if (size == 0) return BACKING_STORE_OK; if (offset + size > buf_size || offset + size < offset) @@ -633,19 +645,10 @@ virStorageFileMatchesVersion(int format, if ((fileTypeInfo[format].versionOffset + 4) > buflen) return false; - if (fileTypeInfo[format].endian == LV_LITTLE_ENDIAN) { - version = - (buf[fileTypeInfo[format].versionOffset+3] << 24) | - (buf[fileTypeInfo[format].versionOffset+2] << 16) | - (buf[fileTypeInfo[format].versionOffset+1] << 8) | - (buf[fileTypeInfo[format].versionOffset]); - } else { - version = - (buf[fileTypeInfo[format].versionOffset] << 24) | - (buf[fileTypeInfo[format].versionOffset+1] << 16) | - (buf[fileTypeInfo[format].versionOffset+2] << 8) | - (buf[fileTypeInfo[format].versionOffset+3]); - } + if (fileTypeInfo[format].endian == LV_LITTLE_ENDIAN) + version = virRead32LE(buf + fileTypeInfo[format].versionOffset); + else + version = virRead32BE(buf + fileTypeInfo[format].versionOffset); VIR_DEBUG("Compare detected version %d vs expected version %d", version, fileTypeInfo[format].versionNumber); @@ -687,29 +690,15 @@ virStorageFileGetMetadataFromBuf(int format, if ((fileTypeInfo[format].sizeOffset + 8) > buflen) return 1; - if (fileTypeInfo[format].endian == LV_LITTLE_ENDIAN) { - meta->capacity = - ((unsigned long long)buf[fileTypeInfo[format].sizeOffset+7] << 56) | - ((unsigned long long)buf[fileTypeInfo[format].sizeOffset+6] << 48) | - ((unsigned long long)buf[fileTypeInfo[format].sizeOffset+5] << 40) | - ((unsigned long long)buf[fileTypeInfo[format].sizeOffset+4] << 32) | - ((unsigned long long)buf[fileTypeInfo[format].sizeOffset+3] << 24) | - ((unsigned long long)buf[fileTypeInfo[format].sizeOffset+2] << 16) | - ((unsigned long long)buf[fileTypeInfo[format].sizeOffset+1] << 8) | - ((unsigned long long)buf[fileTypeInfo[format].sizeOffset]); - } else { - meta->capacity = - ((unsigned long long)buf[fileTypeInfo[format].sizeOffset] << 56) | - ((unsigned long long)buf[fileTypeInfo[format].sizeOffset+1] << 48) | - ((unsigned long long)buf[fileTypeInfo[format].sizeOffset+2] << 40) | - ((unsigned long long)buf[fileTypeInfo[format].sizeOffset+3] << 32) | - ((unsigned long long)buf[fileTypeInfo[format].sizeOffset+4] << 24) | - ((unsigned long long)buf[fileTypeInfo[format].sizeOffset+5] << 16) | - ((unsigned long long)buf[fileTypeInfo[format].sizeOffset+6] << 8) | - ((unsigned long long)buf[fileTypeInfo[format].sizeOffset+7]); - } + if (fileTypeInfo[format].endian == LV_LITTLE_ENDIAN) + meta->capacity = virRead64LE(buf + + fileTypeInfo[format].sizeOffset); + else + meta->capacity = virRead64BE(buf + + fileTypeInfo[format].sizeOffset); /* Avoid unlikely, but theoretically possible overflow */ - if (meta->capacity > (ULLONG_MAX / fileTypeInfo[format].sizeMultiplier)) + if (meta->capacity > (ULLONG_MAX / + fileTypeInfo[format].sizeMultiplier)) return 1; meta->capacity *= fileTypeInfo[format].sizeMultiplier; } @@ -717,11 +706,7 @@ virStorageFileGetMetadataFromBuf(int format, if (fileTypeInfo[format].qcowCryptOffset != -1) { int crypt_format; - crypt_format = - (buf[fileTypeInfo[format].qcowCryptOffset] << 24) | - (buf[fileTypeInfo[format].qcowCryptOffset+1] << 16) | - (buf[fileTypeInfo[format].qcowCryptOffset+2] << 8) | - (buf[fileTypeInfo[format].qcowCryptOffset+3]); + crypt_format = virRead32BE(buf + fileTypeInfo[format].qcowCryptOffset); meta->encrypted = crypt_format != 0; } -- 1.8.1
On Wed, Feb 06, 2013 at 10:10:17PM -0700, Eric Blake wrote:
This makes code easier to read, by avoiding lines longer than 80 columns and removing the repetition from the callers.
* src/util/virstoragefile.c (virRead64BE, virRead64LE) (virRead32BE, virRead32LE): New helper functions. (qedGetHeaderUL, qedGetHeaderULL): Delete in favor of more generic name. (qcow2GetBackingStoreFormat, qcowXGetBackingStore) (qedGetBackingStore, virStorageFileMatchesVersion) (virStorageFileGetMetadataInternal): Use them. --- src/util/virstoragefile.c | 159 +++++++++++++++++++++------------------------- 1 file changed, 72 insertions(+), 87 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 6e2d61e..e7ab226 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -208,6 +208,58 @@ static struct FileTypeInfo const fileTypeInfo[] = { }; verify(ARRAY_CARDINALITY(fileTypeInfo) == VIR_STORAGE_FILE_LAST);
+/* Read 8 bytes at BUF as a big-endian 64-bit number. Caller is + responsible to avoid reading beyond array bounds. */ +static unsigned long long +virRead64BE(const unsigned char *buf) +{ + return (((uint64_t)buf[0] << 56) | + ((uint64_t)buf[1] << 48) | + ((uint64_t)buf[2] << 40) | + ((uint64_t)buf[3] << 32) | + ((uint64_t)buf[4] << 24) | + ((uint64_t)buf[5] << 16) | + ((uint64_t)buf[6] << 8) | + (uint64_t)buf[7]); +} + +/* Read 8 bytes at BUF as a little-endian 64-bit number. Caller is + responsible to avoid reading beyond array bounds. */ +static unsigned long long +virRead64LE(const unsigned char *buf) +{ + return ((uint64_t)buf[0] | + ((uint64_t)buf[1] << 8) | + ((uint64_t)buf[2] << 16) | + ((uint64_t)buf[3] << 24) | + ((uint64_t)buf[4] << 32) | + ((uint64_t)buf[5] << 40) | + ((uint64_t)buf[6] << 48) | + ((uint64_t)buf[7] << 56)); +} + +/* Read 4 bytes at BUF as a big-endian 32-bit number. Caller is + responsible to avoid reading beyond array bounds. */ +static unsigned int +virRead32BE(const unsigned char *buf) +{ + return ((buf[0] << 24) | + (buf[1] << 16) | + (buf[2] << 8) | + buf[3]); +} + +/* Read 4 bytes at BUF as a little-endian 32-bit number. Caller is + responsible to avoid reading beyond array bounds. */ +static unsigned int +virRead32LE(const unsigned char *buf) +{ + return (buf[0] | + (buf[1] << 8) | + (buf[2] << 16) | + (buf[3] << 24)); +}
How about putting these helpful APIs in some other src/util/ file as macros. Either virutil.h, or perhaps virinttypes.h or virendian.h ? Probably with a name like "virReadBufInt{32,64}{BE,LE}" ACK to this patch regardless though. 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 :|
On 02/07/2013 07:53 AM, Daniel P. Berrange wrote:
On Wed, Feb 06, 2013 at 10:10:17PM -0700, Eric Blake wrote:
This makes code easier to read, by avoiding lines longer than 80 columns and removing the repetition from the callers.
How about putting these helpful APIs in some other src/util/ file as macros. Either virutil.h, or perhaps virinttypes.h or virendian.h ?
Indeed, that would be nicer, because then it works on both 'char*' and 'unsigned char *' (don't know why virstoragefile uses unsigned, when most of our other files use plain char).
Probably with a name like "virReadBufInt{32,64}{BE,LE}"
ACK to this patch regardless though.
I'll go ahead and post a v2, with a testsuite addition; it may be a regression that I'm fixing, but I'd rather get it right than rushed. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
No semantic change; done so the next patch doesn't need a forward declaration of a static function. * src/util/virstoragefile.c (virStorageFileProbeFormatFromBuf): Hoist earlier. --- src/util/virstoragefile.c | 82 +++++++++++++++++++++++------------------------ 1 file changed, 41 insertions(+), 41 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index e7ab226..cfd424d 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -668,6 +668,47 @@ virBackingStoreIsFile(const char *backing) } static int +virStorageFileProbeFormatFromBuf(const char *path, + unsigned char *buf, + size_t buflen) +{ + int format = VIR_STORAGE_FILE_RAW; + int i; + int possibleFormat = VIR_STORAGE_FILE_RAW; + VIR_DEBUG("path=%s", path); + + /* First check file magic */ + for (i = 0 ; i < VIR_STORAGE_FILE_LAST ; i++) { + if (virStorageFileMatchesMagic(i, buf, buflen)) { + if (!virStorageFileMatchesVersion(i, buf, buflen)) { + possibleFormat = i; + continue; + } + format = i; + goto cleanup; + } + } + + if (possibleFormat != VIR_STORAGE_FILE_RAW) + VIR_WARN("File %s matches %s magic, but version is wrong. " + "Please report new version to libvir-list@redhat.com", + path, virStorageFileFormatTypeToString(possibleFormat)); + + /* No magic, so check file extension */ + for (i = 0 ; i < VIR_STORAGE_FILE_LAST ; i++) { + if (virStorageFileMatchesExtension(i, path)) { + format = i; + goto cleanup; + } + } + +cleanup: + VIR_DEBUG("format=%d", format); + return format; +} + + +static int virStorageFileGetMetadataFromBuf(int format, const char *path, unsigned char *buf, @@ -757,47 +798,6 @@ virStorageFileGetMetadataFromBuf(int format, } -static int -virStorageFileProbeFormatFromBuf(const char *path, - unsigned char *buf, - size_t buflen) -{ - int format = VIR_STORAGE_FILE_RAW; - int i; - int possibleFormat = VIR_STORAGE_FILE_RAW; - VIR_DEBUG("path=%s", path); - - /* First check file magic */ - for (i = 0 ; i < VIR_STORAGE_FILE_LAST ; i++) { - if (virStorageFileMatchesMagic(i, buf, buflen)) { - if (!virStorageFileMatchesVersion(i, buf, buflen)) { - possibleFormat = i; - continue; - } - format = i; - goto cleanup; - } - } - - if (possibleFormat != VIR_STORAGE_FILE_RAW) - VIR_WARN("File %s matches %s magic, but version is wrong. " - "Please report new version to libvir-list@redhat.com", - path, virStorageFileFormatTypeToString(possibleFormat)); - - /* No magic, so check file extension */ - for (i = 0 ; i < VIR_STORAGE_FILE_LAST ; i++) { - if (virStorageFileMatchesExtension(i, path)) { - format = i; - goto cleanup; - } - } - -cleanup: - VIR_DEBUG("format=%d", format); - return format; -} - - /** * virStorageFileProbeFormatFromFD: * -- 1.8.1
On Wed, Feb 06, 2013 at 10:10:18PM -0700, Eric Blake wrote:
No semantic change; done so the next patch doesn't need a forward declaration of a static function.
* src/util/virstoragefile.c (virStorageFileProbeFormatFromBuf): Hoist earlier. --- src/util/virstoragefile.c | 82 +++++++++++++++++++++++------------------------ 1 file changed, 41 insertions(+), 41 deletions(-)
ACK 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 :|
Prior to this patch, we had the callchains: external users \-> virStorageFileGetMetadataFromFD \-> virStorageFileGetMetadataFromBuf virStorageFileGetMetadataRecurse \-> virStorageFileGetMetadataFromFD \-> virStorageFileGetMetadataFromBuf However, a future patch wants to add an additional parameter to the bottom of the chain, for use by virStorageFileGetMetadataRecurse, without affecting existing external callers. Since there is only a single caller of the internal function, we can repurpose it to fit our needs, with this patch giving us: external users \-> virStorageFileGetMetadataFromFD \-> virStorageFileGetMetadataInternal virStorageFileGetMetadataRecurse / \-> virStorageFileGetMetadataInternal Note that no callers cared whether virStorageFileGetMetadataFromBuf returned 0 (success) or 1 (able to read, but not able to decipher), only whether there was failure (not able to read). * src/util/virstoragefile.c (virStorageFileGetMetadataFromFD): Move most of the guts... (virStorageFileGetMetadataFromBuf): ...here, and rename... (virStorageFileGetMetadataInternal): ...to this. (virStorageFileGetMetadataRecurse): Use internal helper. --- src/util/virstoragefile.c | 156 ++++++++++++++++++++++------------------------ 1 file changed, 75 insertions(+), 81 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index cfd424d..1a4557e 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -708,28 +708,70 @@ cleanup: } -static int -virStorageFileGetMetadataFromBuf(int format, - const char *path, - unsigned char *buf, - size_t buflen, - virStorageFileMetadata *meta) +static virStorageFileMetadataPtr +virStorageFileGetMetadataInternal(const char *path, + int fd, + int format) { - VIR_DEBUG("path=%s format=%d", path, format); + virStorageFileMetadata *meta = NULL; + unsigned char *buf = NULL; + ssize_t len = STORAGE_MAX_HEAD; + virStorageFileMetadata *ret = NULL; + struct stat sb; + + VIR_DEBUG("path=%s, fd=%d, format=%d", path, fd, format); + + if (VIR_ALLOC(meta) < 0) { + virReportOOMError(); + return NULL; + } + + if (fstat(fd, &sb) < 0) { + virReportSystemError(errno, + _("cannot stat file '%s'"), + path); + goto cleanup; + } + + /* No header to probe for directories, but also no backing file */ + if (S_ISDIR(sb.st_mode)) + return meta; + + if (lseek(fd, 0, SEEK_SET) == (off_t)-1) { + virReportSystemError(errno, _("cannot seek to start of '%s'"), path); + goto cleanup; + } + + if (VIR_ALLOC_N(buf, len) < 0) { + virReportOOMError(); + goto cleanup; + } + + if ((len = read(fd, buf, len)) < 0) { + virReportSystemError(errno, _("cannot read header '%s'"), path); + goto cleanup; + } + + if (format == VIR_STORAGE_FILE_AUTO) + format = virStorageFileProbeFormatFromBuf(path, buf, len); + + if (format <= VIR_STORAGE_FILE_NONE || + format >= VIR_STORAGE_FILE_LAST) { + virReportSystemError(EINVAL, _("unknown storage file format %d"), + format); + goto cleanup; + } /* XXX we should consider moving virStorageBackendUpdateVolInfo * code into this method, for non-magic files */ - if (format <= VIR_STORAGE_FILE_NONE || - format >= VIR_STORAGE_FILE_LAST || - !fileTypeInfo[format].magic) { - return 0; - } + if (!fileTypeInfo[format].magic) + goto done; /* Optionally extract capacity from file */ if (fileTypeInfo[format].sizeOffset != -1) { - if ((fileTypeInfo[format].sizeOffset + 8) > buflen) - return 1; + if ((fileTypeInfo[format].sizeOffset + 8) > len) + goto done; if (fileTypeInfo[format].endian == LV_LITTLE_ENDIAN) meta->capacity = virRead64LE(buf + @@ -740,7 +782,7 @@ virStorageFileGetMetadataFromBuf(int format, /* Avoid unlikely, but theoretically possible overflow */ if (meta->capacity > (ULLONG_MAX / fileTypeInfo[format].sizeMultiplier)) - return 1; + goto done; meta->capacity *= fileTypeInfo[format].sizeMultiplier; } @@ -754,14 +796,14 @@ virStorageFileGetMetadataFromBuf(int format, if (fileTypeInfo[format].getBackingStore != NULL) { char *backing; int backingFormat; - int ret = fileTypeInfo[format].getBackingStore(&backing, - &backingFormat, - buf, buflen); - if (ret == BACKING_STORE_INVALID) - return 1; + int store = fileTypeInfo[format].getBackingStore(&backing, + &backingFormat, + buf, len); + if (store == BACKING_STORE_INVALID) + goto done; - if (ret == BACKING_STORE_ERROR) - return -1; + if (store == BACKING_STORE_ERROR) + goto cleanup; meta->backingStoreIsFile = false; if (backing != NULL) { @@ -769,7 +811,7 @@ virStorageFileGetMetadataFromBuf(int format, if (meta->backingStore == NULL) { virReportOOMError(); VIR_FREE(backing); - return -1; + goto cleanup; } if (virBackingStoreIsFile(backing)) { meta->backingStoreIsFile = true; @@ -794,7 +836,14 @@ virStorageFileGetMetadataFromBuf(int format, } } - return 0; +done: + ret = meta; + meta = NULL; + +cleanup: + virStorageFileFreeMetadata(meta); + VIR_FREE(buf); + return ret; } @@ -907,62 +956,7 @@ virStorageFileGetMetadataFromFD(const char *path, int fd, int format) { - virStorageFileMetadata *meta = NULL; - unsigned char *head = NULL; - ssize_t len = STORAGE_MAX_HEAD; - virStorageFileMetadata *ret = NULL; - struct stat sb; - - if (VIR_ALLOC(meta) < 0) { - virReportOOMError(); - return NULL; - } - - if (fstat(fd, &sb) < 0) { - virReportSystemError(errno, - _("cannot stat file '%s'"), - path); - goto cleanup; - } - - /* No header to probe for directories, but also no backing file */ - if (S_ISDIR(sb.st_mode)) - return meta; - - if (lseek(fd, 0, SEEK_SET) == (off_t)-1) { - virReportSystemError(errno, _("cannot seek to start of '%s'"), path); - goto cleanup; - } - - if (VIR_ALLOC_N(head, len) < 0) { - virReportOOMError(); - goto cleanup; - } - - if ((len = read(fd, head, len)) < 0) { - virReportSystemError(errno, _("cannot read header '%s'"), path); - goto cleanup; - } - - if (format == VIR_STORAGE_FILE_AUTO) - format = virStorageFileProbeFormatFromBuf(path, head, len); - - if (format <= VIR_STORAGE_FILE_NONE || - format >= VIR_STORAGE_FILE_LAST) { - virReportSystemError(EINVAL, _("unknown storage file format %d"), - format); - goto cleanup; - } - - if (virStorageFileGetMetadataFromBuf(format, path, head, len, meta) < 0) - goto cleanup; - ret = meta; - meta = NULL; - -cleanup: - virStorageFileFreeMetadata(meta); - VIR_FREE(head); - return ret; + return virStorageFileGetMetadataInternal(path, fd, format); } /* Recursive workhorse for virStorageFileGetMetadata. */ @@ -991,7 +985,7 @@ virStorageFileGetMetadataRecurse(const char *path, int format, return NULL; } - ret = virStorageFileGetMetadataFromFD(path, fd, format); + ret = virStorageFileGetMetadataInternal(path, fd, format); if (VIR_CLOSE(fd) < 0) VIR_WARN("could not close file %s", path); -- 1.8.1
On Wed, Feb 06, 2013 at 10:10:19PM -0700, Eric Blake wrote:
Prior to this patch, we had the callchains: external users \-> virStorageFileGetMetadataFromFD \-> virStorageFileGetMetadataFromBuf virStorageFileGetMetadataRecurse \-> virStorageFileGetMetadataFromFD \-> virStorageFileGetMetadataFromBuf
However, a future patch wants to add an additional parameter to the bottom of the chain, for use by virStorageFileGetMetadataRecurse, without affecting existing external callers. Since there is only a single caller of the internal function, we can repurpose it to fit our needs, with this patch giving us:
external users \-> virStorageFileGetMetadataFromFD \-> virStorageFileGetMetadataInternal virStorageFileGetMetadataRecurse / \-> virStorageFileGetMetadataInternal
Note that no callers cared whether virStorageFileGetMetadataFromBuf returned 0 (success) or 1 (able to read, but not able to decipher), only whether there was failure (not able to read).
* src/util/virstoragefile.c (virStorageFileGetMetadataFromFD): Move most of the guts... (virStorageFileGetMetadataFromBuf): ...here, and rename... (virStorageFileGetMetadataInternal): ...to this. (virStorageFileGetMetadataRecurse): Use internal helper. --- src/util/virstoragefile.c | 156 ++++++++++++++++++++++------------------------ 1 file changed, 75 insertions(+), 81 deletions(-)
ACK, a little complex to follow though. 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 :|
On 02/07/2013 07:55 AM, Daniel P. Berrange wrote:
However, a future patch wants to add an additional parameter to the bottom of the chain, for use by virStorageFileGetMetadataRecurse, without affecting existing external callers. Since there is only a single caller of the internal function, we can repurpose it to fit our needs, with this patch giving us:
ACK, a little complex to follow though.
In my v2, I'll split the refactor into two parts - variable renaming, vs. code motion, so it is hopefully easier to follow. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
If you have a qcow2 file /path1/to/file pointed to by symlink /path2/symlink, and pass qemu /path2/symlink, then qemu treats a relative backing file in the qcow2 metadata as being relative to /path2, not /path1/to. Yes, this means that it is possible to create a qcow2 file where the choice of WHICH directory and symlink you access its contents from will then determine WHICH backing file (if any) you actually find; the results can be rather screwy, but we have to match what qemu does. Libvirt and qemu default to creating absolute backing file names, so most users don't hit this. But at least VDSM uses symlinks and relative backing names alongside the --reuse-external flags to libvirt snapshot operations, with the result that libvirt was failing to follow the intended chain of backing files, and then backing files were not granted the necessary sVirt permissions to be opened by qemu. See https://bugzilla.redhat.com/show_bug.cgi?id=903248 for more gory details. This fixes a regression introduced in commit 8250783. I tested this patch by creating the following chain: ls /home/eblake/Downloads/Fedora.iso # raw file for base cd /var/lib/libvirt/images qemu-img create -f qcow2 \ -obacking_file=/home/eblake/Downloads/Fedora.iso,backing_fmt=raw one mkdir sub cd sub ln -s ../one onelink qemu-img create -f qcow2 \ -obacking_file=../sub/onelink,backing_fmt=qcow2 two mv two .. ln -s ../two twolink qemu-img create -f qcow2 \ -obacking_file=../sub/twolink,backing_fmt=qcow2 three mv three .. ln -s ../three threelink then pointing my domain at /var/lib/libvirt/images/sub/threelink. Prior to this patch, I got complaints about missing backing files; afterwards, I was able to verify that the backing chain (and hence DAC and SELinux relabels) of the entire chain worked. * src/util/virstoragefile.h (_virStorageFileMetadata): Add directory member. * src/util/virstoragefile.c (absolutePathFromBaseFile): Drop, replaced by... (virFindBackingFile): ...better function. (virStorageFileGetMetadataInternal): Add an argument. (virStorageFileGetMetadataFromFD, virStorageFileChainLookup) (virStorageFileGetMetadata): Update callers. --- src/util/virstoragefile.c | 88 ++++++++++++++++++++++++++++++----------------- src/util/virstoragefile.h | 1 + 2 files changed, 57 insertions(+), 32 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 1a4557e..555a911 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -548,44 +548,56 @@ qedGetBackingStore(char **res, } /** - * Return an absolute path corresponding to PATH, which is absolute or relative - * to the directory containing BASE_FILE, or NULL on error + * Given a starting point START (either an original file name, or the + * directory containing the original name, depending on START_IS_DIR) + * and a possibly relative backing file NAME, compute the relative + * DIRECTORY (optional) and CANONICAL (mandatory) location of the + * backing file. Return 0 on success, negative on error. */ -static char * -absolutePathFromBaseFile(const char *base_file, const char *path) +static int ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(5) +virFindBackingFile(const char *start, bool start_is_dir, const char *path, + char **directory, char **canonical) { - char *res = NULL; - char *tmp = NULL; - size_t d_len = dir_len(base_file); + char *combined = NULL; + int ret = -1; - /* If path is already absolute, or if dirname(base_file) is ".", - just return a copy of path. */ - if (*path == '/' || d_len == 0) { - if (!(res = canonicalize_file_name(path))) - virReportSystemError(errno, - _("Can't canonicalize path '%s'"), path); + if (*path == '/') { + /* Safe to cast away const */ + combined = (char *)path; + } else { + size_t d_len = start_is_dir ? strlen(start) : dir_len(start); - goto cleanup; + if (d_len > INT_MAX) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("name too long: '%s'"), start); + goto cleanup; + } else if (!d_len) { + start = "."; + d_len = 1; + } + if (virAsprintf(&combined, "%.*s/%s", (int)d_len, start, path) < 0) { + virReportOOMError(); + goto cleanup; + } } - /* Ensure that the following cast-to-int is valid. */ - if (d_len > INT_MAX) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Directory name too long: '%s'"), base_file); + if (directory && !(*directory = mdir_name(combined))) { + virReportOOMError(); goto cleanup; } - if (virAsprintf(&tmp, "%.*s/%s", (int) d_len, base_file, path) < 0) { - virReportOOMError(); + if (!(*canonical = canonicalize_file_name(combined))) { + virReportSystemError(errno, + _("Can't canonicalize path '%s'"), path); goto cleanup; } - if (!(res = canonicalize_file_name(tmp))) - virReportSystemError(errno, _("Can't canonicalize path '%s'"), path); + ret = 0; cleanup: - VIR_FREE(tmp); - return res; + if (combined != path) + VIR_FREE(combined); + return ret; } @@ -708,9 +720,13 @@ cleanup: } +/* Given a file descriptor FD open on PATH, and optionally opened from + * a given DIRECTORY, return metadata about that file, assuming it has + * the given FORMAT. */ static virStorageFileMetadataPtr virStorageFileGetMetadataInternal(const char *path, int fd, + const char *directory, int format) { virStorageFileMetadata *meta = NULL; @@ -816,8 +832,11 @@ virStorageFileGetMetadataInternal(const char *path, if (virBackingStoreIsFile(backing)) { meta->backingStoreIsFile = true; meta->backingStoreRaw = meta->backingStore; - meta->backingStore = absolutePathFromBaseFile(path, backing); - if (meta->backingStore == NULL) { + meta->backingStore = NULL; + if (virFindBackingFile(directory ? directory : path, + !!directory, backing, + &meta->directory, + &meta->backingStore) < 0) { /* the backing file is (currently) unavailable, treat this * file as standalone: * backingStoreRaw is kept to mark broken image chains */ @@ -956,13 +975,13 @@ virStorageFileGetMetadataFromFD(const char *path, int fd, int format) { - return virStorageFileGetMetadataInternal(path, fd, format); + return virStorageFileGetMetadataInternal(path, fd, NULL, format); } /* Recursive workhorse for virStorageFileGetMetadata. */ static virStorageFileMetadataPtr -virStorageFileGetMetadataRecurse(const char *path, int format, - uid_t uid, gid_t gid, +virStorageFileGetMetadataRecurse(const char *path, const char *directory, + int format, uid_t uid, gid_t gid, bool allow_probe, virHashTablePtr cycle) { int fd; @@ -985,7 +1004,7 @@ virStorageFileGetMetadataRecurse(const char *path, int format, return NULL; } - ret = virStorageFileGetMetadataInternal(path, fd, format); + ret = virStorageFileGetMetadataInternal(path, fd, directory, format); if (VIR_CLOSE(fd) < 0) VIR_WARN("could not close file %s", path); @@ -997,6 +1016,7 @@ virStorageFileGetMetadataRecurse(const char *path, int format, ret->backingStoreFormat = VIR_STORAGE_FILE_AUTO; format = ret->backingStoreFormat; ret->backingMeta = virStorageFileGetMetadataRecurse(ret->backingStore, + ret->directory, format, uid, gid, allow_probe, @@ -1044,7 +1064,7 @@ virStorageFileGetMetadata(const char *path, int format, if (format <= VIR_STORAGE_FILE_NONE) format = allow_probe ? VIR_STORAGE_FILE_AUTO : VIR_STORAGE_FILE_RAW; - ret = virStorageFileGetMetadataRecurse(path, format, uid, gid, + ret = virStorageFileGetMetadataRecurse(path, NULL, format, uid, gid, allow_probe, cycle); virHashFree(cycle); return ret; @@ -1064,6 +1084,7 @@ virStorageFileFreeMetadata(virStorageFileMetadata *meta) virStorageFileFreeMetadata(meta->backingMeta); VIR_FREE(meta->backingStore); VIR_FREE(meta->backingStoreRaw); + VIR_FREE(meta->directory); VIR_FREE(meta); } @@ -1366,7 +1387,10 @@ virStorageFileChainLookup(virStorageFileMetadataPtr chain, const char *start, STREQ(name, owner->backingStore)) { break; } else if (owner->backingStoreIsFile) { - char *absName = absolutePathFromBaseFile(*parent, name); + char *absName = NULL; + if (virFindBackingFile(owner->directory, true, name, + NULL, &absName) < 0) + goto error; if (absName && STREQ(absName, owner->backingStore)) { VIR_FREE(absName); break; diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 821d07e..d7b4508 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -56,6 +56,7 @@ typedef virStorageFileMetadata *virStorageFileMetadataPtr; struct _virStorageFileMetadata { char *backingStore; /* Canonical name (absolute file, or protocol) */ char *backingStoreRaw; /* If file, original name, possibly relative */ + char *directory; /* The directory containing basename(backingStoreRaw) */ int backingStoreFormat; /* enum virStorageFileFormat */ bool backingStoreIsFile; virStorageFileMetadataPtr backingMeta; -- 1.8.1
On Wed, Feb 06, 2013 at 10:10:20PM -0700, Eric Blake wrote:
If you have a qcow2 file /path1/to/file pointed to by symlink /path2/symlink, and pass qemu /path2/symlink, then qemu treats a relative backing file in the qcow2 metadata as being relative to /path2, not /path1/to. Yes, this means that it is possible to create a qcow2 file where the choice of WHICH directory and symlink you access its contents from will then determine WHICH backing file (if any) you actually find; the results can be rather screwy, but we have to match what qemu does.
Libvirt and qemu default to creating absolute backing file names, so most users don't hit this. But at least VDSM uses symlinks and relative backing names alongside the --reuse-external flags to libvirt snapshot operations, with the result that libvirt was failing to follow the intended chain of backing files, and then backing files were not granted the necessary sVirt permissions to be opened by qemu.
See https://bugzilla.redhat.com/show_bug.cgi?id=903248 for more gory details. This fixes a regression introduced in commit 8250783.
I tested this patch by creating the following chain:
ls /home/eblake/Downloads/Fedora.iso # raw file for base cd /var/lib/libvirt/images qemu-img create -f qcow2 \ -obacking_file=/home/eblake/Downloads/Fedora.iso,backing_fmt=raw one mkdir sub cd sub ln -s ../one onelink qemu-img create -f qcow2 \ -obacking_file=../sub/onelink,backing_fmt=qcow2 two mv two .. ln -s ../two twolink qemu-img create -f qcow2 \ -obacking_file=../sub/twolink,backing_fmt=qcow2 three mv three .. ln -s ../three threelink
then pointing my domain at /var/lib/libvirt/images/sub/threelink. Prior to this patch, I got complaints about missing backing files; afterwards, I was able to verify that the backing chain (and hence DAC and SELinux relabels) of the entire chain worked.
Could you look at creating some kind of test suite for these APIs. We could easily call 'qemu-img' to create a bunch of different files for testing, and then run the APIs against them & validate the results. 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 :|
On Wed, Feb 06, 2013 at 10:10:20PM -0700, Eric Blake wrote:
If you have a qcow2 file /path1/to/file pointed to by symlink /path2/symlink, and pass qemu /path2/symlink, then qemu treats a relative backing file in the qcow2 metadata as being relative to /path2, not /path1/to. Yes, this means that it is possible to create a qcow2 file where the choice of WHICH directory and symlink you access its contents from will then determine WHICH backing file (if any) you actually find; the results can be rather screwy, but we have to match what qemu does.
Libvirt and qemu default to creating absolute backing file names, so most users don't hit this. But at least VDSM uses symlinks and relative backing names alongside the --reuse-external flags to libvirt snapshot operations, with the result that libvirt was failing to follow the intended chain of backing files, and then backing files were not granted the necessary sVirt permissions to be opened by qemu.
See https://bugzilla.redhat.com/show_bug.cgi?id=903248 for more gory details. This fixes a regression introduced in commit 8250783.
I tested this patch by creating the following chain:
ls /home/eblake/Downloads/Fedora.iso # raw file for base cd /var/lib/libvirt/images qemu-img create -f qcow2 \ -obacking_file=/home/eblake/Downloads/Fedora.iso,backing_fmt=raw one mkdir sub cd sub ln -s ../one onelink qemu-img create -f qcow2 \ -obacking_file=../sub/onelink,backing_fmt=qcow2 two mv two .. ln -s ../two twolink qemu-img create -f qcow2 \ -obacking_file=../sub/twolink,backing_fmt=qcow2 three mv three .. ln -s ../three threelink
then pointing my domain at /var/lib/libvirt/images/sub/threelink. Prior to this patch, I got complaints about missing backing files; afterwards, I was able to verify that the backing chain (and hence DAC and SELinux relabels) of the entire chain worked.
* src/util/virstoragefile.h (_virStorageFileMetadata): Add directory member. * src/util/virstoragefile.c (absolutePathFromBaseFile): Drop, replaced by... (virFindBackingFile): ...better function. (virStorageFileGetMetadataInternal): Add an argument. (virStorageFileGetMetadataFromFD, virStorageFileChainLookup) (virStorageFileGetMetadata): Update callers. --- src/util/virstoragefile.c | 88 ++++++++++++++++++++++++++++++----------------- src/util/virstoragefile.h | 1 + 2 files changed, 57 insertions(+), 32 deletions(-)
ACK only on the basis that you tested it, since I find it hard to follow the code well enough to verify it does the right thing with relative symlinks, without actually testing 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 :|
participants (2)
-
Daniel P. Berrange -
Eric Blake