[libvirt] [PATCH 0/3] more storage cleanups
I'm still plugging away at getting gluster backing chains to work; here's a couple of independent cleanups that can be applied first. Eric Blake (3): storage: use simpler 'char *' storage: avoid short reads while chasing backing chain storage: reduce number of stat calls src/libvirt_private.syms | 1 + src/storage/storage_backend.c | 62 +++++++++++++++++-------------------- src/storage/storage_backend.h | 10 ++++-- src/storage/storage_backend_fs.c | 5 +-- src/storage/storage_backend_mpath.c | 7 +++-- src/storage/storage_backend_scsi.c | 7 +++-- src/util/virfile.c | 21 +++++++++++++ src/util/virfile.h | 9 ++++-- src/util/virstoragefile.c | 52 ++++++++++++++----------------- 9 files changed, 100 insertions(+), 74 deletions(-) -- 1.8.3.1
'unsigned char *' makes sense if you are doing math on bytes and don't want to worry about wraparound from a signed 'char'; but since all we are doing is memcmp() or virReadBufInt*[LB]E(), which are both safe on either type of char, and since read() prefers to operate on 'char *', it's simpler to avoid casts by just typing things as 'char *' from the get-go. [Technically, read can operate on an 'unsigned char *' thanks to the C rule that any pointer can be implicitly converted to 'char *' for legacy K&R compatibility; but where this patch saves us is if we try to use virfile.h functions that take 'char **' in order to allocate the buffer, where the compiler would barf on type mismatch.] * src/util/virstoragefile.c (FileTypeInfo): Avoid unsigned char. (cowGetBackingStore, qcow2GetBackingStoreFormat) (qcowXGetBackingStore, qcow1GetBackingStore) (qcow2GetBackingStore, vmdk4GetBackingStore, qedGetBackingStore) (virStorageFileMatchesMagic, virStorageFileMatchesVersion) (virStorageFileProbeFormatFromBuf, qcow2GetFeatures) (virStorageFileGetMetadataInternal) (virStorageFileProbeFormatFromFD): Simplify clients. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/util/virstoragefile.c | 42 +++++++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 8f04b78..589778c 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -102,23 +102,23 @@ struct FileTypeInfo { * where to find encryption mode, * -1 if encryption is not used */ int (*getBackingStore)(char **res, int *format, - const unsigned char *buf, size_t buf_size); + const char *buf, size_t buf_size); int (*getFeatures)(virBitmapPtr *features, int format, - unsigned char *buf, ssize_t len); + char *buf, ssize_t len); }; static int cowGetBackingStore(char **, int *, - const unsigned char *, size_t); + const char *, size_t); static int qcow1GetBackingStore(char **, int *, - const unsigned char *, size_t); + const char *, size_t); static int qcow2GetBackingStore(char **, int *, - const unsigned char *, size_t); + const char *, size_t); static int qcow2GetFeatures(virBitmapPtr *features, int format, - unsigned char *buf, ssize_t len); + char *buf, ssize_t len); static int vmdk4GetBackingStore(char **, int *, - const unsigned char *, size_t); + const char *, size_t); static int -qedGetBackingStore(char **, int *, const unsigned char *, size_t); +qedGetBackingStore(char **, int *, const char *, size_t); #define QCOWX_HDR_VERSION (4) #define QCOWX_HDR_BACKING_FILE_OFFSET (QCOWX_HDR_VERSION+4) @@ -252,7 +252,7 @@ verify(ARRAY_CARDINALITY(qcow2CompatibleFeatureArray) == static int cowGetBackingStore(char **res, int *format, - const unsigned char *buf, + const char *buf, size_t buf_size) { #define COW_FILENAME_MAXLEN 1024 @@ -274,7 +274,7 @@ cowGetBackingStore(char **res, static int qcow2GetBackingStoreFormat(int *format, - const unsigned char *buf, + const char *buf, size_t buf_size, size_t extension_start, size_t extension_end) @@ -329,7 +329,7 @@ done: static int qcowXGetBackingStore(char **res, int *format, - const unsigned char *buf, + const char *buf, size_t buf_size, bool isQCow2) { @@ -407,7 +407,7 @@ qcowXGetBackingStore(char **res, static int qcow1GetBackingStore(char **res, int *format, - const unsigned char *buf, + const char *buf, size_t buf_size) { int ret; @@ -424,7 +424,7 @@ qcow1GetBackingStore(char **res, static int qcow2GetBackingStore(char **res, int *format, - const unsigned char *buf, + const char *buf, size_t buf_size) { return qcowXGetBackingStore(res, format, buf, buf_size, true); @@ -434,7 +434,7 @@ qcow2GetBackingStore(char **res, static int vmdk4GetBackingStore(char **res, int *format, - const unsigned char *buf, + const char *buf, size_t buf_size) { static const char prefix[] = "parentFileNameHint=\""; @@ -495,7 +495,7 @@ cleanup: static int qedGetBackingStore(char **res, int *format, - const unsigned char *buf, + const char *buf, size_t buf_size) { unsigned long long flags; @@ -596,7 +596,7 @@ cleanup: static bool virStorageFileMatchesMagic(int format, - unsigned char *buf, + char *buf, size_t buflen) { int mlen; @@ -634,7 +634,7 @@ virStorageFileMatchesExtension(int format, static bool virStorageFileMatchesVersion(int format, - unsigned char *buf, + char *buf, size_t buflen) { int version; @@ -684,7 +684,7 @@ virBackingStoreIsFile(const char *backing) static int virStorageFileProbeFormatFromBuf(const char *path, - unsigned char *buf, + char *buf, size_t buflen) { int format = VIR_STORAGE_FILE_RAW; @@ -726,7 +726,7 @@ cleanup: static int qcow2GetFeatures(virBitmapPtr *features, int format, - unsigned char *buf, + char *buf, ssize_t len) { int version = -1; @@ -767,7 +767,7 @@ virStorageFileGetMetadataInternal(const char *path, int format) { virStorageFileMetadata *meta = NULL; - unsigned char *buf = NULL; + char *buf = NULL; ssize_t len = STORAGE_MAX_HEAD; virStorageFileMetadata *ret = NULL; struct stat sb; @@ -922,7 +922,7 @@ cleanup: int virStorageFileProbeFormatFromFD(const char *path, int fd) { - unsigned char *head; + char *head = NULL; ssize_t len = STORAGE_MAX_HEAD; int ret = -1; struct stat sb; -- 1.8.3.1
Our backing file chain code was not very robust to an ill-timed EINTR, which could lead to a short read causing us to randomly treat metadata differently than usual. But the existing virFileReadLimFD forces an error if we don't read the entire file, even though we only care about the header of the file. * src/util/virfile.h (virFileReadHeaderFD): New prototype. * src/util/virfile.c (virFileReadHeaderFD): New function. * src/libvirt_private.syms (virfile.h): Export it. * src/util/virstoragefile.c (virStorageFileGetMetadataInternal) (virStorageFileProbeFormatFromFD): Use it. Signed-off-by: Eric Blake <eblake@redhat.com> --- Found by inspection, rather than an actual EINTR, so I have no idea how likely the bug was to hit in practice. But better safe than sorry! src/libvirt_private.syms | 1 + src/util/virfile.c | 21 +++++++++++++++++++++ src/util/virfile.h | 9 ++++++--- src/util/virstoragefile.c | 10 ++-------- 4 files changed, 30 insertions(+), 11 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ad8a79d..398b0c0 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1208,6 +1208,7 @@ virFileOpenAs; virFileOpenTty; virFilePrintf; virFileReadAll; +virFileReadHeaderFD; virFileReadLimFD; virFileResolveAllLinks; virFileResolveLink; diff --git a/src/util/virfile.c b/src/util/virfile.c index 4882006..0e8d52d 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -1225,6 +1225,27 @@ saferead_lim(int fd, size_t max_len, size_t *length) return NULL; } + +/* A wrapper around saferead_lim that merely stops reading at the + * specified maximum size. */ +int +virFileReadHeaderFD(int fd, int maxlen, char **buf) +{ + size_t len; + char *s; + + if (maxlen <= 0) { + errno = EINVAL; + return -1; + } + s = saferead_lim(fd, maxlen, &len); + if (s == NULL) + return -1; + *buf = s; + return len; +} + + /* A wrapper around saferead_lim that maps a failure due to exceeding the maximum size limitation to EOVERFLOW. */ int diff --git a/src/util/virfile.h b/src/util/virfile.h index ff84719..10cf8bd 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -122,9 +122,12 @@ int virFileNBDDeviceAssociate(const char *file, int virFileDeleteTree(const char *dir); -int virFileReadLimFD(int fd, int maxlen, char **buf) ATTRIBUTE_RETURN_CHECK; - -int virFileReadAll(const char *path, int maxlen, char **buf) ATTRIBUTE_RETURN_CHECK; +int virFileReadHeaderFD(int fd, int maxlen, char **buf) + ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(3); +int virFileReadLimFD(int fd, int maxlen, char **buf) + ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(3); +int virFileReadAll(const char *path, int maxlen, char **buf) + ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3); int virFileWriteStr(const char *path, const char *str, mode_t mode) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 589778c..bbd5a7e 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -793,10 +793,7 @@ virStorageFileGetMetadataInternal(const char *path, goto cleanup; } - if (VIR_ALLOC_N(buf, len) < 0) - goto cleanup; - - if ((len = read(fd, buf, len)) < 0) { + if ((len = virFileReadHeaderFD(fd, len, &buf)) < 0) { virReportSystemError(errno, _("cannot read header '%s'"), path); goto cleanup; } @@ -939,15 +936,12 @@ virStorageFileProbeFormatFromFD(const char *path, int fd) return VIR_STORAGE_FILE_DIR; } - if (VIR_ALLOC_N(head, len) < 0) - return -1; - if (lseek(fd, 0, SEEK_SET) == (off_t)-1) { virReportSystemError(errno, _("cannot set to start of '%s'"), path); goto cleanup; } - if ((len = read(fd, head, len)) < 0) { + if ((len = virFileReadHeaderFD(fd, len, &head)) < 0) { virReportSystemError(errno, _("cannot read header '%s'"), path); goto cleanup; } -- 1.8.3.1
We are calling fstat() at least twice per storage volume in a directory storage pool; this is rather wasteful. Refactoring this is also a step towards making code reusable for gluster, where gluster can provide struct stat but cannot use fstat(). * src/storage/storage_backend.h (virStorageBackendVolOpenCheckMode) (virStorageBackendUpdateVolTargetInfoFD): Update signature. * src/storage/storage_backend.c (virStorageBackendVolOpenCheckMode): Pass stat results back. (virStorageBackendUpdateVolTargetInfoFD): Use existing stats. (virStorageBackendVolOpen, virStorageBackendUpdateVolTargetInfo): Update callers. * src/storage/storage_backend_fs.c (virStorageBackendProbeTarget): Likewise. * src/storage/storage_backend_scsi.c (virStorageBackendSCSIUpdateVolTargetInfo): Likewise. * src/storage/storage_backend_mpath.c (virStorageBackendMpathUpdateVolTargetInfo): Likewise. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/storage/storage_backend.c | 62 +++++++++++++++++-------------------- src/storage/storage_backend.h | 10 ++++-- src/storage/storage_backend_fs.c | 5 +-- src/storage/storage_backend_mpath.c | 7 +++-- src/storage/storage_backend_scsi.c | 7 +++-- 5 files changed, 49 insertions(+), 42 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index aa2ed28..4b1b00d 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1152,30 +1152,30 @@ virStorageBackendForType(int type) * volume is a dangling symbolic link. */ int -virStorageBackendVolOpenCheckMode(const char *path, unsigned int flags) +virStorageBackendVolOpenCheckMode(const char *path, struct stat *sb, + unsigned int flags) { int fd, mode = 0; - struct stat sb; char *base = last_component(path); - if (lstat(path, &sb) < 0) { + if (lstat(path, sb) < 0) { virReportSystemError(errno, _("cannot stat file '%s'"), path); return -1; } - if (S_ISFIFO(sb.st_mode)) { + if (S_ISFIFO(sb->st_mode)) { VIR_WARN("ignoring FIFO '%s'", path); return -2; - } else if (S_ISSOCK(sb.st_mode)) { + } else if (S_ISSOCK(sb->st_mode)) { VIR_WARN("ignoring socket '%s'", path); return -2; } if ((fd = open(path, O_RDONLY|O_NONBLOCK|O_NOCTTY)) < 0) { if ((errno == ENOENT || errno == ELOOP) && - S_ISLNK(sb.st_mode)) { + S_ISLNK(sb->st_mode)) { VIR_WARN("ignoring dangling symlink '%s'", path); return -2; } @@ -1186,7 +1186,7 @@ virStorageBackendVolOpenCheckMode(const char *path, unsigned int flags) return -1; } - if (fstat(fd, &sb) < 0) { + if (fstat(fd, sb) < 0) { virReportSystemError(errno, _("cannot stat file '%s'"), path); @@ -1194,13 +1194,13 @@ virStorageBackendVolOpenCheckMode(const char *path, unsigned int flags) return -1; } - if (S_ISREG(sb.st_mode)) + if (S_ISREG(sb->st_mode)) mode = VIR_STORAGE_VOL_OPEN_REG; - else if (S_ISCHR(sb.st_mode)) + else if (S_ISCHR(sb->st_mode)) mode = VIR_STORAGE_VOL_OPEN_CHAR; - else if (S_ISBLK(sb.st_mode)) + else if (S_ISBLK(sb->st_mode)) mode = VIR_STORAGE_VOL_OPEN_BLOCK; - else if (S_ISDIR(sb.st_mode)) { + else if (S_ISDIR(sb->st_mode)) { mode = VIR_STORAGE_VOL_OPEN_DIR; if (STREQ(base, ".") || @@ -1229,7 +1229,8 @@ virStorageBackendVolOpenCheckMode(const char *path, unsigned int flags) int virStorageBackendVolOpen(const char *path) { - return virStorageBackendVolOpenCheckMode(path, + struct stat sb; + return virStorageBackendVolOpenCheckMode(path, &sb, VIR_STORAGE_VOL_OPEN_DEFAULT); } @@ -1240,14 +1241,16 @@ virStorageBackendUpdateVolTargetInfo(virStorageVolTargetPtr target, unsigned int openflags) { int ret, fd; + struct stat sb; - if ((ret = virStorageBackendVolOpenCheckMode(target->path, + if ((ret = virStorageBackendVolOpenCheckMode(target->path, &sb, openflags)) < 0) return ret; fd = ret; ret = virStorageBackendUpdateVolTargetInfoFD(target, fd, + &sb, allocation, capacity); @@ -1298,35 +1301,28 @@ int virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol, int virStorageBackendUpdateVolTargetInfoFD(virStorageVolTargetPtr target, int fd, + struct stat *sb, unsigned long long *allocation, unsigned long long *capacity) { - struct stat sb; #if WITH_SELINUX security_context_t filecon = NULL; #endif - if (fstat(fd, &sb) < 0) { - virReportSystemError(errno, - _("cannot stat file '%s'"), - target->path); - return -1; - } - if (allocation) { - if (S_ISREG(sb.st_mode)) { + if (S_ISREG(sb->st_mode)) { #ifndef WIN32 - *allocation = (unsigned long long)sb.st_blocks * + *allocation = (unsigned long long)sb->st_blocks * (unsigned long long)DEV_BSIZE; #else - *allocation = sb.st_size; + *allocation = sb->st_size; #endif /* Regular files may be sparse, so logical size (capacity) is not same * as actual allocation above */ if (capacity) - *capacity = sb.st_size; - } else if (S_ISDIR(sb.st_mode)) { + *capacity = sb->st_size; + } else if (S_ISDIR(sb->st_mode)) { *allocation = 0; if (capacity) *capacity = 0; @@ -1351,16 +1347,16 @@ virStorageBackendUpdateVolTargetInfoFD(virStorageVolTargetPtr target, } } - target->perms.mode = sb.st_mode & S_IRWXUGO; - target->perms.uid = sb.st_uid; - target->perms.gid = sb.st_gid; + target->perms.mode = sb->st_mode & S_IRWXUGO; + target->perms.uid = sb->st_uid; + target->perms.gid = sb->st_gid; if (!target->timestamps && VIR_ALLOC(target->timestamps) < 0) return -1; - target->timestamps->atime = get_stat_atime(&sb); - target->timestamps->btime = get_stat_birthtime(&sb); - target->timestamps->ctime = get_stat_ctime(&sb); - target->timestamps->mtime = get_stat_mtime(&sb); + target->timestamps->atime = get_stat_atime(sb); + target->timestamps->btime = get_stat_birthtime(sb); + target->timestamps->ctime = get_stat_ctime(sb); + target->timestamps->mtime = get_stat_mtime(sb); VIR_FREE(target->perms.label); diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 3dc9c64..d8d3097 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -24,6 +24,8 @@ #ifndef __VIR_STORAGE_BACKEND_H__ # define __VIR_STORAGE_BACKEND_H__ +# include <sys/stat.h> + # include "internal.h" # include "storage_conf.h" # include "vircommand.h" @@ -108,9 +110,10 @@ enum { VIR_STORAGE_VOL_OPEN_CHAR |\ VIR_STORAGE_VOL_OPEN_BLOCK) -int virStorageBackendVolOpenCheckMode(const char *path, unsigned int flags) -ATTRIBUTE_RETURN_CHECK -ATTRIBUTE_NONNULL(1); +int virStorageBackendVolOpenCheckMode(const char *path, struct stat *sb, + unsigned int flags) + ATTRIBUTE_RETURN_CHECK + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); int virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol, int withCapacity); @@ -124,6 +127,7 @@ int virStorageBackendUpdateVolTargetInfo(virStorageVolTargetPtr target, unsigned int openflags); int virStorageBackendUpdateVolTargetInfoFD(virStorageVolTargetPtr target, int fd, + struct stat *sb, unsigned long long *allocation, unsigned long long *capacity); int diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 7ff950b..58fb7c1 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -70,18 +70,19 @@ virStorageBackendProbeTarget(virStorageVolTargetPtr target, int fd = -1; int ret = -1; virStorageFileMetadata *meta = NULL; + struct stat sb; *backingStore = NULL; *backingStoreFormat = VIR_STORAGE_FILE_AUTO; if (encryption) *encryption = NULL; - if ((ret = virStorageBackendVolOpenCheckMode(target->path, + if ((ret = virStorageBackendVolOpenCheckMode(target->path, &sb, VIR_STORAGE_VOL_FS_REFRESH_FLAGS)) < 0) goto error; /* Take care to propagate ret, it is not always -1 */ fd = ret; - if ((ret = virStorageBackendUpdateVolTargetInfoFD(target, fd, + if ((ret = virStorageBackendUpdateVolTargetInfoFD(target, fd, &sb, allocation, capacity)) < 0) { goto error; diff --git a/src/storage/storage_backend_mpath.c b/src/storage/storage_backend_mpath.c index 9a19484..1e65a8d 100644 --- a/src/storage/storage_backend_mpath.c +++ b/src/storage/storage_backend_mpath.c @@ -1,7 +1,7 @@ /* * storage_backend_mpath.c: storage backend for multipath handling * - * Copyright (C) 2009-2011 Red Hat, Inc. + * Copyright (C) 2009-2011, 2013 Red Hat, Inc. * Copyright (C) 2009-2008 Dave Allan * * This library is free software; you can redistribute it and/or @@ -46,13 +46,16 @@ virStorageBackendMpathUpdateVolTargetInfo(virStorageVolTargetPtr target, { int ret = -1; int fdret, fd = -1; + struct stat sb; - if ((fdret = virStorageBackendVolOpen(target->path)) < 0) + if ((fdret = virStorageBackendVolOpenCheckMode(target->path, &sb, + VIR_STORAGE_VOL_OPEN_DEFAULT)) < 0) goto out; fd = fdret; if (virStorageBackendUpdateVolTargetInfoFD(target, fd, + &sb, allocation, capacity) < 0) goto out; diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index 5769799..1426121 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -1,7 +1,7 @@ /* * storage_backend_scsi.c: storage backend for SCSI handling * - * Copyright (C) 2007-2008 Red Hat, Inc. + * Copyright (C) 2007-2008, 2013 Red Hat, Inc. * Copyright (C) 2007-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -138,13 +138,16 @@ virStorageBackendSCSIUpdateVolTargetInfo(virStorageVolTargetPtr target, { int fdret, fd = -1; int ret = -1; + struct stat sb; - if ((fdret = virStorageBackendVolOpen(target->path)) < 0) + if ((fdret = virStorageBackendVolOpenCheckMode(target->path, &sb, + VIR_STORAGE_VOL_OPEN_DEFAULT)) < 0) goto cleanup; fd = fdret; if (virStorageBackendUpdateVolTargetInfoFD(target, fd, + &sb, allocation, capacity) < 0) goto cleanup; -- 1.8.3.1
Future patches will want to learn metadata about a file using a buffer that was already parsed in order to probe the file's format. Rather than reopening and re-reading the file, it makes sense to separate getting file contents from actually parsing those contents. * src/util/virstoragefile.c (virStorageFileGetMetadataFromBuf) (virStorageFileGetMetadataFromFDInternal): New functions. (virStorageFileGetMetadataInternal): Hoist fstat() and read() into callers. (virStorageFileGetMetadataFromFD) (virStorageFileGetMetadataRecurse): Rework clients. * src/util/virstoragefile.h (virStorageFileGetMetadataFromBuf): New prototype. * src/libvirt_private.syms (virstoragefile.h): Export it. Signed-off-by: Eric Blake <eblake@redhat.com> --- The diffstat looks big, but a lot of that is due to duplicated doc comments. src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 118 +++++++++++++++++++++++++++++++++------------- src/util/virstoragefile.h | 4 ++ 3 files changed, 91 insertions(+), 32 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 398b0c0..c1189d7 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1724,6 +1724,7 @@ virStorageFileFormatTypeToString; virStorageFileFreeMetadata; virStorageFileGetLVMKey; virStorageFileGetMetadata; +virStorageFileGetMetadataFromBuf; virStorageFileGetMetadataFromFD; virStorageFileGetSCSIKey; virStorageFileIsClusterFS; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index bbd5a7e..c1faf41 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -757,47 +757,26 @@ qcow2GetFeatures(virBitmapPtr *features, } -/* 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. */ +/* Given a header in BUF with length LEN, as parsed from the file + * located at 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, + char *buf, + size_t len, const char *directory, int format) { virStorageFileMetadata *meta = NULL; - 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); + VIR_DEBUG("path=%s, buf=%p, len=%zu, directory=%s, format=%d", + path, buf, len, NULLSTR(directory), format); if (VIR_ALLOC(meta) < 0) 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 ((len = virFileReadHeaderFD(fd, len, &buf)) < 0) { - virReportSystemError(errno, _("cannot read header '%s'"), path); - goto cleanup; - } - if (format == VIR_STORAGE_FILE_AUTO) format = virStorageFileProbeFormatFromBuf(path, buf, len); @@ -898,7 +877,6 @@ done: cleanup: virStorageFileFreeMetadata(meta); - VIR_FREE(buf); return ret; } @@ -984,6 +962,81 @@ virStorageFileProbeFormat(const char *path, uid_t uid, gid_t gid) return ret; } + +/** + * virStorageFileGetMetadataFromBuf: + * @path: name of file, for error messages + * @buf: header bytes from @path + * @len: length of @buf + * @format: expected image format + * + * Extract metadata about the storage volume with the specified + * image format. If image format is VIR_STORAGE_FILE_AUTO, it + * will probe to automatically identify the format. Does not recurse. + * + * Callers are advised never to use VIR_STORAGE_FILE_AUTO as a + * format, since a malicious guest can turn a raw file into any + * other non-raw format at will. + * + * If the returned meta.backingStoreFormat is VIR_STORAGE_FILE_AUTO + * it indicates the image didn't specify an explicit format for its + * backing store. Callers are advised against probing for the + * backing store format in this case. + * + * Caller MUST free the result after use via virStorageFileFreeMetadata. + */ +virStorageFileMetadataPtr +virStorageFileGetMetadataFromBuf(const char *path, + char *buf, + size_t len, + int format) +{ + return virStorageFileGetMetadataInternal(path, buf, len, NULL, format); +} + + +/* Internal version that also supports a containing directory name. */ +static virStorageFileMetadataPtr +virStorageFileGetMetadataFromFDInternal(const char *path, + int fd, + const char *directory, + int format) +{ + char *buf = NULL; + ssize_t len = STORAGE_MAX_HEAD; + struct stat sb; + virStorageFileMetadataPtr ret = NULL; + + if (fstat(fd, &sb) < 0) { + virReportSystemError(errno, + _("cannot stat file '%s'"), + path); + return NULL; + } + + /* No header to probe for directories, but also no backing file */ + if (S_ISDIR(sb.st_mode)) { + ignore_value(VIR_ALLOC(ret)); + goto cleanup; + } + + if (lseek(fd, 0, SEEK_SET) == (off_t)-1) { + virReportSystemError(errno, _("cannot seek to start of '%s'"), path); + goto cleanup; + } + + if ((len = virFileReadHeaderFD(fd, len, &buf)) < 0) { + virReportSystemError(errno, _("cannot read header '%s'"), path); + goto cleanup; + } + + ret = virStorageFileGetMetadataInternal(path, buf, len, directory, format); +cleanup: + VIR_FREE(buf); + return ret; +} + + /** * virStorageFileGetMetadataFromFD: * @@ -1007,9 +1060,10 @@ virStorageFileGetMetadataFromFD(const char *path, int fd, int format) { - return virStorageFileGetMetadataInternal(path, fd, NULL, format); + return virStorageFileGetMetadataFromFDInternal(path, fd, NULL, format); } + /* Recursive workhorse for virStorageFileGetMetadata. */ static virStorageFileMetadataPtr virStorageFileGetMetadataRecurse(const char *path, const char *directory, @@ -1036,7 +1090,7 @@ virStorageFileGetMetadataRecurse(const char *path, const char *directory, return NULL; } - ret = virStorageFileGetMetadataInternal(path, fd, directory, format); + ret = virStorageFileGetMetadataFromFDInternal(path, fd, directory, format); if (VIR_CLOSE(fd) < 0) VIR_WARN("could not close file %s", path); diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index d5effa4..f3d9832 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -90,6 +90,10 @@ virStorageFileMetadataPtr virStorageFileGetMetadata(const char *path, virStorageFileMetadataPtr virStorageFileGetMetadataFromFD(const char *path, int fd, int format); +virStorageFileMetadataPtr virStorageFileGetMetadataFromBuf(const char *path, + char *buf, + size_t len, + int format); int virStorageFileChainGetBroken(virStorageFileMetadataPtr chain, char **broken_file); -- 1.8.3.1
This gets rid of another stat() per volume, as well as cutting bytes read in half, when populating the volumes of a directory pool during a pool refresh. Not to mention that it provides an interface that can let gluster pools also probe file types. * src/util/virstoragefile.h (virStorageFileProbeFormatFromFD): Delete. (virStorageFileProbeFormatFromBuf): New prototype. (VIR_STORAGE_MAX_HEADER): New constant, based on... * src/util/virstoragefile.c (STORAGE_MAX_HEAD): ...old name. (vmdk4GetBackingStore, virStorageFileGetMetadataInternal) (virStorageFileProbeFormat): Adjust clients. (virStorageFileProbeFormatFromFD): Delete. (virStorageFileProbeFormatFromBuf): Export. * src/storage/storage_backend_fs.c (virStorageBackendProbeTarget): Adjust client. * src/libvirt_private.syms (virstoragefile.h): Adjust exports. Signed-off-by: Eric Blake <eblake@redhat.com> --- This is the default 'git diff' output. The 'git diff --patience' output is slightly longer, but might be slightly easier to read. src/libvirt_private.syms | 2 +- src/storage/storage_backend_fs.c | 53 +++++++++++++++++--------- src/util/virstoragefile.c | 81 +++++++++++++--------------------------- src/util/virstoragefile.h | 12 +++++- 4 files changed, 72 insertions(+), 76 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c1189d7..76016ca 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1731,7 +1731,7 @@ virStorageFileIsClusterFS; virStorageFileIsSharedFS; virStorageFileIsSharedFSType; virStorageFileProbeFormat; -virStorageFileProbeFormatFromFD; +virStorageFileProbeFormatFromBuf; virStorageFileResize; diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 58fb7c1..24579b0 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -1,7 +1,7 @@ /* * storage_backend_fs.c: storage backend for FS and directory handling * - * Copyright (C) 2007-2012 Red Hat, Inc. + * Copyright (C) 2007-2013 Red Hat, Inc. * Copyright (C) 2007-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -71,6 +71,8 @@ virStorageBackendProbeTarget(virStorageVolTargetPtr target, int ret = -1; virStorageFileMetadata *meta = NULL; struct stat sb; + char *header = NULL; + ssize_t len = VIR_STORAGE_MAX_HEADER; *backingStore = NULL; *backingStoreFormat = VIR_STORAGE_FILE_AUTO; @@ -88,20 +90,33 @@ virStorageBackendProbeTarget(virStorageVolTargetPtr target, goto error; } - if ((target->format = virStorageFileProbeFormatFromFD(target->path, fd)) < 0) { - ret = -1; - goto error; - } + if (S_ISDIR(sb.st_mode)) { + target->format = VIR_STORAGE_FILE_DIR; + } else { + if ((len = virFileReadHeaderFD(fd, len, &header)) < 0) { + virReportSystemError(errno, _("cannot read header '%s'"), + target->path); + goto error; + } - if (!(meta = virStorageFileGetMetadataFromFD(target->path, fd, - target->format))) { - ret = -1; - goto error; + target->format = virStorageFileProbeFormatFromBuf(target->path, + header, len); + if (target->format < 0) { + ret = -1; + goto error; + } + + if (!(meta = virStorageFileGetMetadataFromBuf(target->path, + header, len, + target->format))) { + ret = -1; + goto error; + } } VIR_FORCE_CLOSE(fd); - if (meta->backingStore) { + if (meta && meta->backingStore) { *backingStore = meta->backingStore; meta->backingStore = NULL; if (meta->backingStoreFormat == VIR_STORAGE_FILE_AUTO && @@ -126,10 +141,10 @@ virStorageBackendProbeTarget(virStorageVolTargetPtr target, ret = 0; } - if (capacity && meta->capacity) + if (capacity && meta && meta->capacity) *capacity = meta->capacity; - if (encryption != NULL && meta->encrypted) { + if (encryption && meta && meta->encrypted) { if (VIR_ALLOC(*encryption) < 0) goto cleanup; @@ -150,24 +165,26 @@ virStorageBackendProbeTarget(virStorageVolTargetPtr target, } virBitmapFree(target->features); - target->features = meta->features; - meta->features = NULL; + if (meta) { + target->features = meta->features; + meta->features = NULL; + } - if (meta->compat) { + if (meta && meta->compat) { VIR_FREE(target->compat); target->compat = meta->compat; meta->compat = NULL; } - virStorageFileFreeMetadata(meta); - - return ret; + goto cleanup; error: VIR_FORCE_CLOSE(fd); cleanup: virStorageFileFreeMetadata(meta); + VIR_FREE(header); + virStorageFileFreeMetadata(meta); return ret; } diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index c1faf41..e45236f 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -148,11 +148,6 @@ qedGetBackingStore(char **, int *, const char *, size_t); #define QED_F_BACKING_FILE 0x01 #define QED_F_BACKING_FORMAT_NO_PROBE 0x04 -/* VMDK needs at least 20*512 B to find backing store, - * ISO has 5 Byte magic on offset 32769, - * other formats need less */ -#define STORAGE_MAX_HEAD 32769+5 - static struct FileTypeInfo const fileTypeInfo[] = { [VIR_STORAGE_FILE_NONE] = { 0, NULL, NULL, LV_LITTLE_ENDIAN, @@ -442,7 +437,7 @@ vmdk4GetBackingStore(char **res, size_t len; int ret = BACKING_STORE_ERROR; - if (VIR_ALLOC_N(desc, STORAGE_MAX_HEAD + 1) < 0) + if (VIR_ALLOC_N(desc, VIR_STORAGE_MAX_HEADER) < 0) goto cleanup; *res = NULL; @@ -460,8 +455,8 @@ vmdk4GetBackingStore(char **res, goto cleanup; } len = buf_size - 0x200; - if (len > STORAGE_MAX_HEAD) - len = STORAGE_MAX_HEAD; + if (len > VIR_STORAGE_MAX_HEADER) + len = VIR_STORAGE_MAX_HEADER; memcpy(desc, buf + 0x200, len); desc[len] = '\0'; start = strstr(desc, prefix); @@ -682,7 +677,7 @@ virBackingStoreIsFile(const char *backing) return true; } -static int +int virStorageFileProbeFormatFromBuf(const char *path, char *buf, size_t buflen) @@ -690,7 +685,7 @@ virStorageFileProbeFormatFromBuf(const char *path, int format = VIR_STORAGE_FILE_RAW; size_t i; int possibleFormat = VIR_STORAGE_FILE_RAW; - VIR_DEBUG("path=%s", path); + VIR_DEBUG("path=%s, buf=%p, buflen=%zu", path, buf, buflen); /* First check file magic */ for (i = 0; i < VIR_STORAGE_FILE_LAST; i++) { @@ -882,36 +877,41 @@ cleanup: /** - * virStorageFileProbeFormatFromFD: + * virStorageFileProbeFormat: * - * Probe for the format of 'fd' (which is an open file descriptor - * pointing to 'path'), returning the detected disk format. + * Probe for the format of 'path', returning the detected + * disk format. * * Callers are advised never to trust the returned 'format' * unless it is listed as VIR_STORAGE_FILE_RAW, since a - * malicious guest can turn a file into any other non-raw + * malicious guest can turn a raw file into any other non-raw * format at will. * * Best option: Don't use this function */ int -virStorageFileProbeFormatFromFD(const char *path, int fd) +virStorageFileProbeFormat(const char *path, uid_t uid, gid_t gid) { - char *head = NULL; - ssize_t len = STORAGE_MAX_HEAD; + int fd; int ret = -1; struct stat sb; + ssize_t len = VIR_STORAGE_MAX_HEADER; + char *header = NULL; - if (fstat(fd, &sb) < 0) { - virReportSystemError(errno, - _("cannot stat file '%s'"), - path); + if ((fd = virFileOpenAs(path, O_RDONLY, 0, uid, gid, 0)) < 0) { + virReportSystemError(-fd, _("Failed to open file '%s'"), path); return -1; } + if (fstat(fd, &sb) < 0) { + virReportSystemError(errno, _("cannot stat file '%s'"), path); + goto cleanup; + } + /* No header to probe for directories */ if (S_ISDIR(sb.st_mode)) { - return VIR_STORAGE_FILE_DIR; + ret = VIR_STORAGE_FILE_DIR; + goto cleanup; } if (lseek(fd, 0, SEEK_SET) == (off_t)-1) { @@ -919,44 +919,15 @@ virStorageFileProbeFormatFromFD(const char *path, int fd) goto cleanup; } - if ((len = virFileReadHeaderFD(fd, len, &head)) < 0) { + if ((len = virFileReadHeaderFD(fd, len, &header)) < 0) { virReportSystemError(errno, _("cannot read header '%s'"), path); goto cleanup; } - ret = virStorageFileProbeFormatFromBuf(path, head, len); + ret = virStorageFileProbeFormatFromBuf(path, header, len); cleanup: - VIR_FREE(head); - return ret; -} - - -/** - * virStorageFileProbeFormat: - * - * Probe for the format of 'path', returning the detected - * disk format. - * - * Callers are advised never to trust the returned 'format' - * unless it is listed as VIR_STORAGE_FILE_RAW, since a - * malicious guest can turn a raw file into any other non-raw - * format at will. - * - * Best option: Don't use this function - */ -int -virStorageFileProbeFormat(const char *path, uid_t uid, gid_t gid) -{ - int fd, ret; - - if ((fd = virFileOpenAs(path, O_RDONLY, 0, uid, gid, 0)) < 0) { - virReportSystemError(-fd, _("Failed to open file '%s'"), path); - return -1; - } - - ret = virStorageFileProbeFormatFromFD(path, fd); - + VIR_FREE(header); VIR_FORCE_CLOSE(fd); return ret; @@ -1003,7 +974,7 @@ virStorageFileGetMetadataFromFDInternal(const char *path, int format) { char *buf = NULL; - ssize_t len = STORAGE_MAX_HEAD; + ssize_t len = VIR_STORAGE_MAX_HEADER; struct stat sb; virStorageFileMetadataPtr ret = NULL; diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index f3d9832..7bd2fe0 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -27,6 +27,14 @@ # include "virbitmap.h" # include "virutil.h" +/* Minimum header size required to probe all known formats with + * virStorageFileProbeFormat, or obtain metadata from a known format. + * Rounded to multiple of 512 (ISO has a 5-byte magic at offset + * 32769). Some formats can be probed with fewer bytes. Although + * some formats theoretically permit metadata that can rely on offsets + * beyond this size, in practice that doesn't matter. */ +# define VIR_STORAGE_MAX_HEADER 0x8200 + enum virStorageFileFormat { VIR_STORAGE_FILE_AUTO_SAFE = -2, VIR_STORAGE_FILE_AUTO = -1, @@ -80,8 +88,8 @@ struct _virStorageFileMetadata { # endif int virStorageFileProbeFormat(const char *path, uid_t uid, gid_t gid); -int virStorageFileProbeFormatFromFD(const char *path, - int fd); +int virStorageFileProbeFormatFromBuf(const char *path, char *buf, + size_t buflen); virStorageFileMetadataPtr virStorageFileGetMetadata(const char *path, int format, -- 1.8.3.1
On 11/05/2013 02:32 PM, Eric Blake wrote:
This gets rid of another stat() per volume, as well as cutting bytes read in half, when populating the volumes of a directory pool during a pool refresh. Not to mention that it provides an interface that can let gluster pools also probe file types.
* src/util/virstoragefile.h (virStorageFileProbeFormatFromFD): Delete. (virStorageFileProbeFormatFromBuf): New prototype. (VIR_STORAGE_MAX_HEADER): New constant, based on... * src/util/virstoragefile.c (STORAGE_MAX_HEAD): ...old name. (vmdk4GetBackingStore, virStorageFileGetMetadataInternal) (virStorageFileProbeFormat): Adjust clients. (virStorageFileProbeFormatFromFD): Delete. (virStorageFileProbeFormatFromBuf): Export. * src/storage/storage_backend_fs.c (virStorageBackendProbeTarget): Adjust client. * src/libvirt_private.syms (virstoragefile.h): Adjust exports.
Signed-off-by: Eric Blake <eblake@redhat.com> ---
- virStorageFileFreeMetadata(meta); - - return ret; + goto cleanup;
error: VIR_FORCE_CLOSE(fd);
cleanup: virStorageFileFreeMetadata(meta); + VIR_FREE(header);
Up to here is fine...
+ virStorageFileFreeMetadata(meta);
...but I have no idea what I was thinking with this line of the patch :) I've already deleted it in my local tree, to avoid double frees. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 11/05/2013 02:11 PM, Eric Blake wrote:
I'm still plugging away at getting gluster backing chains to work; here's a couple of independent cleanups that can be applied first.
Eric Blake (3): storage: use simpler 'char *' storage: avoid short reads while chasing backing chain storage: reduce number of stat calls
src/libvirt_private.syms | 1 + src/storage/storage_backend.c | 62 +++++++++++++++++-------------------- src/storage/storage_backend.h | 10 ++++-- src/storage/storage_backend_fs.c | 5 +-- src/storage/storage_backend_mpath.c | 7 +++-- src/storage/storage_backend_scsi.c | 7 +++-- src/util/virfile.c | 21 +++++++++++++ src/util/virfile.h | 9 ++++-- src/util/virstoragefile.c | 52 ++++++++++++++----------------- 9 files changed, 100 insertions(+), 74 deletions(-)
ACK series (including your 5/3 update which I was just reading when I got the updated email). John
On 11/05/2013 05:48 PM, John Ferlan wrote:
On 11/05/2013 02:11 PM, Eric Blake wrote:
I'm still plugging away at getting gluster backing chains to work; here's a couple of independent cleanups that can be applied first.
Eric Blake (3): storage: use simpler 'char *' storage: avoid short reads while chasing backing chain storage: reduce number of stat calls
src/libvirt_private.syms | 1 + src/storage/storage_backend.c | 62 +++++++++++++++++-------------------- src/storage/storage_backend.h | 10 ++++-- src/storage/storage_backend_fs.c | 5 +-- src/storage/storage_backend_mpath.c | 7 +++-- src/storage/storage_backend_scsi.c | 7 +++-- src/util/virfile.c | 21 +++++++++++++ src/util/virfile.h | 9 ++++-- src/util/virstoragefile.c | 52 ++++++++++++++----------------- 9 files changed, 100 insertions(+), 74 deletions(-)
ACK series (including your 5/3 update which I was just reading when I got the updated email).
Thanks; I've pushed these now. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
John Ferlan