11:10 p.m.
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
11:10 p.m.
New subject: [libvirt] [PATCH 1/4] storage: factor out large integer reads
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
8:53 a.m.
New subject: [libvirt] [PATCH 1/4] storage: factor out large integer reads
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 :|
4:25 p.m.
New subject: [libvirt] [PATCH 1/4] storage: factor out large integer reads
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
11:10 p.m.
New subject: [libvirt] [PATCH 2/4] storage: rearrange functions
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(a)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(a)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
8:54 a.m.
New subject: [libvirt] [PATCH 2/4] storage: rearrange functions
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 :|
11:10 p.m.
New subject: [libvirt] [PATCH 3/4] storage: refactor metadata lookup
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
8:55 a.m.
New subject: [libvirt] [PATCH 3/4] storage: refactor metadata lookup
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 :|
7:36 a.m.
New subject: [libvirt] [PATCH 3/4] storage: refactor metadata lookup
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
11:10 p.m.
New subject: [libvirt] [PATCH 4/4] storage: don't follow backing chain symlinks too eagerly
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
6:38 a.m.
New subject: [libvirt] [PATCH 4/4] storage: don't follow backing chain symlinks too eagerly
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 :|
8:57 a.m.
New subject: [libvirt] [PATCH 4/4] storage: don't follow backing chain symlinks too eagerly
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 :|
4307
days inactive
4309
days old
11 comments
2 participants
participants (2)
-
Daniel P. Berrange -
Eric Blake