2:38 p.m.
See https://bugzilla.redhat.com/show_bug.cgi?id=903248 for
more gory details. This fixes a regression introduced in
commit 8250783.
V1 was here:
https://www.redhat.com/archives/libvir-list/2013-February/msg00340.html
changes since then:
rebase to latest, add a testsuite, split one patch into two for
easier review, and push already-acked changes.
Eric Blake (5):
storage: rearrange functions
storage: prepare for refactoring
storage: refactor metadata lookup
storage: don't follow backing chain symlinks too eagerly
storage: test backing chain traversal
.gitignore | 1 +
src/util/virstoragefile.c | 314 +++++++++++++-------------
src/util/virstoragefile.h | 1 +
tests/Makefile.am | 6 +
tests/virstoragetest.c | 546 ++++++++++++++++++++++++++++++++++++++++++++++
5 files changed, 720 insertions(+), 148 deletions(-)
create mode 100644 tests/virstoragetest.c
--
1.8.1.2
2:38 p.m.
New subject: [libvirt] [PATCHv2 1/5] 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 dc28539..f2cbaa1 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -617,6 +617,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,
@@ -707,47 +748,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.2
3:13 p.m.
New subject: [libvirt] [PATCHv2 1/5] storage: rearrange functions
On 02/15/13 21:38, 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(-)
Trivial. ACK.
Peter
2:38 p.m.
New subject: [libvirt] [PATCHv2 2/5] storage: prepare for refactoring
virStorageFileGetMetadataFromFD is the only caller of
virStorageFileGetMetadataFromBuf; and it doesn't care about the
difference between a return of 0 (total success) or 1
(metadata was inconsistent, but pointer was populated as best
as possible); only about a return of -1 (could not read metadata
or out of memory). Changing the return type, and normalizing
the variable names used, will make merging the functions easier
in the next commit.
* src/util/virstoragefile.c (virStorageFileGetMetadataFromBuf):
Change return value, and rename some variables.
(virStorageFileGetMetadataFromFD): Rename some variables.
---
src/util/virstoragefile.c | 50 +++++++++++++++++++++++++----------------------
1 file changed, 27 insertions(+), 23 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index f2cbaa1..83b00e2 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -657,13 +657,15 @@ cleanup:
}
-static int
+static virStorageFileMetadataPtr
virStorageFileGetMetadataFromBuf(int format,
const char *path,
unsigned char *buf,
- size_t buflen,
+ size_t len,
virStorageFileMetadata *meta)
{
+ virStorageFileMetadata *ret = NULL;
+
VIR_DEBUG("path=%s format=%d", path, format);
/* XXX we should consider moving virStorageBackendUpdateVolInfo
@@ -671,14 +673,13 @@ virStorageFileGetMetadataFromBuf(int format,
*/
if (format <= VIR_STORAGE_FILE_NONE ||
format >= VIR_STORAGE_FILE_LAST ||
- !fileTypeInfo[format].magic) {
- return 0;
- }
+ !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 = virReadBufInt64LE(buf +
@@ -689,7 +690,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;
}
@@ -704,14 +705,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) {
@@ -719,7 +720,7 @@ virStorageFileGetMetadataFromBuf(int format,
if (meta->backingStore == NULL) {
virReportOOMError();
VIR_FREE(backing);
- return -1;
+ goto cleanup;
}
if (virBackingStoreIsFile(backing)) {
meta->backingStoreIsFile = true;
@@ -744,7 +745,10 @@ virStorageFileGetMetadataFromBuf(int format,
}
}
- return 0;
+done:
+ ret = meta;
+cleanup:
+ return ret;
}
@@ -858,7 +862,7 @@ virStorageFileGetMetadataFromFD(const char *path,
int format)
{
virStorageFileMetadata *meta = NULL;
- unsigned char *head = NULL;
+ unsigned char *buf = NULL;
ssize_t len = STORAGE_MAX_HEAD;
virStorageFileMetadata *ret = NULL;
struct stat sb;
@@ -884,18 +888,18 @@ virStorageFileGetMetadataFromFD(const char *path,
goto cleanup;
}
- if (VIR_ALLOC_N(head, len) < 0) {
+ if (VIR_ALLOC_N(buf, len) < 0) {
virReportOOMError();
goto cleanup;
}
- if ((len = read(fd, head, len)) < 0) {
+ 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, head, len);
+ format = virStorageFileProbeFormatFromBuf(path, buf, len);
if (format <= VIR_STORAGE_FILE_NONE ||
format >= VIR_STORAGE_FILE_LAST) {
@@ -904,14 +908,14 @@ virStorageFileGetMetadataFromFD(const char *path,
goto cleanup;
}
- if (virStorageFileGetMetadataFromBuf(format, path, head, len, meta) < 0)
+ if (!virStorageFileGetMetadataFromBuf(format, path, buf, len, meta))
goto cleanup;
ret = meta;
meta = NULL;
cleanup:
virStorageFileFreeMetadata(meta);
- VIR_FREE(head);
+ VIR_FREE(buf);
return ret;
}
--
1.8.1.2
3:22 p.m.
New subject: [libvirt] [PATCHv2 2/5] storage: prepare for refactoring
On 02/15/13 21:38, Eric Blake wrote:
virStorageFileGetMetadataFromFD is the only caller of
virStorageFileGetMetadataFromBuf; and it doesn't care about the
difference between a return of 0 (total success) or 1
(metadata was inconsistent, but pointer was populated as best
as possible); only about a return of -1 (could not read metadata
or out of memory). Changing the return type, and normalizing
the variable names used, will make merging the functions easier
in the next commit.
* src/util/virstoragefile.c (virStorageFileGetMetadataFromBuf):
Change return value, and rename some variables.
(virStorageFileGetMetadataFromFD): Rename some variables.
---
src/util/virstoragefile.c | 50 +++++++++++++++++++++++++----------------------
1 file changed, 27 insertions(+), 23 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index f2cbaa1..83b00e2 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -657,13 +657,15 @@ cleanup:
}
-static int
+static virStorageFileMetadataPtr
virStorageFileGetMetadataFromBuf(int format,
const char *path,
unsigned char *buf,
- size_t buflen,
+ size_t len,
virStorageFileMetadata *meta)
{
+ virStorageFileMetadata *ret = NULL;
+
VIR_DEBUG("path=%s format=%d", path, format);
/* XXX we should consider moving virStorageBackendUpdateVolInfo
@@ -671,14 +673,13 @@ virStorageFileGetMetadataFromBuf(int format,
*/
if (format <= VIR_STORAGE_FILE_NONE ||
format >= VIR_STORAGE_FILE_LAST ||
- !fileTypeInfo[format].magic) {
- return 0;
- }
+ !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 = virReadBufInt64LE(buf +
@@ -689,7 +690,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;
}
@@ -704,14 +705,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) {
@@ -719,7 +720,7 @@ virStorageFileGetMetadataFromBuf(int format,
if (meta->backingStore == NULL) {
virReportOOMError();
VIR_FREE(backing);
- return -1;
+ goto cleanup;
}
if (virBackingStoreIsFile(backing)) {
meta->backingStoreIsFile = true;
@@ -744,7 +745,10 @@ virStorageFileGetMetadataFromBuf(int format,
}
}
- return 0;
+done:
+ ret = meta;
Um, is the ret variable really needed here? The only value it can take
is "meta" and return it right after. If this isn't needed in the next
patches I'd rather go for "return meta" here.
+cleanup:
Rename this to "error"
+ return ret;
And "return NULL" instead;
}
ACK if:
1) this change will be needed later
2) tweaked according to my suggestions
Peter
3:26 p.m.
New subject: [libvirt] [PATCHv2 2/5] storage: prepare for refactoring
On 02/15/2013 02:22 PM, Peter Krempa wrote:
> - return 0;
> +done:
> + ret = meta;
Um, is the ret variable really needed here? The only value it can take
is "meta" and return it right after. If this isn't needed in the next
patches I'd rather go for "return meta" here.
> +cleanup:
Rename this to "error"
> + return ret;
And "return NULL" instead;
Next patch turns moves the allocation into this function, at which point
the tail end becomes:
done:
ret = meta;
meta = NULL;
cleanup:
virStorageFileFreeMetadata(meta);
VIR_FREE(buf);
return ret;
> }
>
>
ACK if:
1) this change will be needed later
2) tweaked according to my suggestions
I'm going with option 1 - the labels make more sense once patch 3 is in
place.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
2:38 p.m.
New subject: [libvirt] [PATCHv2 3/5] 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
* 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 | 122 +++++++++++++++++++++-------------------------
1 file changed, 56 insertions(+), 66 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 83b00e2..d741d3d 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -658,22 +658,63 @@ cleanup:
static virStorageFileMetadataPtr
-virStorageFileGetMetadataFromBuf(int format,
- const char *path,
- unsigned char *buf,
- size_t len,
- virStorageFileMetadata *meta)
+virStorageFileGetMetadataInternal(const char *path,
+ int fd,
+ int 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);
- VIR_DEBUG("path=%s format=%d", path, 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)
+ if (!fileTypeInfo[format].magic)
goto done;
/* Optionally extract capacity from file */
@@ -747,7 +788,11 @@ virStorageFileGetMetadataFromBuf(int format,
done:
ret = meta;
+ meta = NULL;
+
cleanup:
+ virStorageFileFreeMetadata(meta);
+ VIR_FREE(buf);
return ret;
}
@@ -861,62 +906,7 @@ virStorageFileGetMetadataFromFD(const char *path,
int fd,
int format)
{
- virStorageFileMetadata *meta = NULL;
- unsigned char *buf = 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(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;
- }
-
- if (!virStorageFileGetMetadataFromBuf(format, path, buf, len, meta))
- goto cleanup;
- ret = meta;
- meta = NULL;
-
-cleanup:
- virStorageFileFreeMetadata(meta);
- VIR_FREE(buf);
- return ret;
+ return virStorageFileGetMetadataInternal(path, fd, format);
}
/* Recursive workhorse for virStorageFileGetMetadata. */
@@ -945,7 +935,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.2
3:37 p.m.
New subject: [libvirt] [PATCHv2 3/5] storage: refactor metadata lookup
On 02/15/13 21:38, Eric Blake wrote:
ACK.
Peter
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
* 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 | 122 +++++++++++++++++++++-------------------------
1 file changed, 56 insertions(+), 66 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 83b00e2..d741d3d 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -658,22 +658,63 @@ cleanup:
static virStorageFileMetadataPtr
-virStorageFileGetMetadataFromBuf(int format,
- const char *path,
- unsigned char *buf,
- size_t len,
- virStorageFileMetadata *meta)
+virStorageFileGetMetadataInternal(const char *path,
+ int fd,
+ int 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);
- VIR_DEBUG("path=%s format=%d", path, 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);
Preexisting, but: s/header/header of/
+ 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)
+ if (!fileTypeInfo[format].magic)
goto done;
/* Optionally extract capacity from file */
@@ -747,7 +788,11 @@ virStorageFileGetMetadataFromBuf(int format,
done:
ret = meta;
+ meta = NULL;
+
cleanup:
+ virStorageFileFreeMetadata(meta);
+ VIR_FREE(buf);
return ret;
}
Ah, now the change makes sense.
2:38 p.m.
New subject: [libvirt] [PATCHv2 4/5] 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 d741d3d..5bb2b58 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -497,44 +497,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;
}
@@ -657,9 +669,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;
@@ -766,8 +782,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 */
@@ -906,13 +925,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;
@@ -935,7 +954,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);
@@ -947,6 +966,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,
@@ -994,7 +1014,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;
@@ -1014,6 +1034,7 @@ virStorageFileFreeMetadata(virStorageFileMetadata *meta)
virStorageFileFreeMetadata(meta->backingMeta);
VIR_FREE(meta->backingStore);
VIR_FREE(meta->backingStoreRaw);
+ VIR_FREE(meta->directory);
VIR_FREE(meta);
}
@@ -1316,7 +1337,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.2
4:08 p.m.
New subject: [libvirt] [PATCHv2 4/5] storage: don't follow backing chain symlinks too eagerly
On 02/15/13 21:38, 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(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index d741d3d..5bb2b58 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -497,44 +497,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) {
I'd go for (d_len == 0) here so that it will not be confused with pointers.
+ start = ".";
+ d_len = 1;
+ }
+ if (virAsprintf(&combined, "%.*s/%s", (int)d_len, start, path)
< 0) {
+ virReportOOMError();
+ goto cleanup;
+ }
}
Otherwise looks reasonable.
ACK.
Peter
4:42 p.m.
New subject: [libvirt] [PATCHv2 4/5] storage: don't follow backing chain symlinks too eagerly
On 02/15/2013 03:08 PM, Peter Krempa wrote:
On 02/15/13 21:38, 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:
and that chain is more-or-less in patch 5/5.
> + } else if (!d_len) {
I'd go for (d_len == 0) here so that it will not be confused with pointers.
Done.
> + start = ".";
> + d_len = 1;
> + }
> + if (virAsprintf(&combined, "%.*s/%s", (int)d_len, start,
> path) < 0) {
> + virReportOOMError();
> + goto cleanup;
> + }
> }
Otherwise looks reasonable.
ACK.
Will push shortly. Thanks for the review.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
2:38 p.m.
New subject: [libvirt] [PATCHv2 5/5] storage: test backing chain traversal
Testing our backing chain handling will make it much easier to
ensure that we avoid issues in the future. If only I had written
this test before I first caused several regressions...
* tests/virstoragetest.c: New test.
* tests/Makefile.am (test_programs): Build it.
* .gitignore: Ignore new files.
---
.gitignore | 1 +
tests/Makefile.am | 6 +
tests/virstoragetest.c | 546 +++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 553 insertions(+)
create mode 100644 tests/virstoragetest.c
diff --git a/.gitignore b/.gitignore
index 8afbf33..23fdc91 100644
--- a/.gitignore
+++ b/.gitignore
@@ -184,6 +184,7 @@
/tests/virnet*test
/tests/virportallocatortest
/tests/virshtest
+/tests/virstoragetest
/tests/virstringtest
/tests/virtimetest
/tests/viruritest
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 7d0a88e..cafdae0 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -100,6 +100,7 @@ test_programs = virshtest sockettest \
virstringtest \
virportallocatortest \
sysinfotest \
+ virstoragetest \
$(NULL)
if WITH_GNUTLS
@@ -565,6 +566,11 @@ virstringtest_SOURCES = \
virstringtest_CFLAGS = -Dabs_builddir="\"$(abs_builddir)\""
$(AM_CFLAGS)
virstringtest_LDADD = $(LDADDS)
+virstoragetest_SOURCES = \
+ virstoragetest.c testutils.h testutils.c
+virstoragetest_CFLAGS = -Dabs_builddir="\"$(abs_builddir)\""
$(AM_CFLAGS)
+virstoragetest_LDADD = $(LDADDS)
+
virlockspacetest_SOURCES = \
virlockspacetest.c testutils.h testutils.c
virlockspacetest_CFLAGS = -Dabs_builddir="\"$(abs_builddir)\""
$(AM_CFLAGS)
diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
new file mode 100644
index 0000000..6b4ca99
--- /dev/null
+++ b/tests/virstoragetest.c
@@ -0,0 +1,546 @@
+/*
+ * Copyright (C) 2013 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library. If not, see
+ * <http://www.gnu.org/licenses/>;.
+ *
+ * Author: Eric Blake <eblake(a)redhat.com>
+ */
+
+#include <config.h>
+
+#include <stdlib.h>
+
+#include "testutils.h"
+#include "vircommand.h"
+#include "virerror.h"
+#include "virlog.h"
+#include "virstoragefile.h"
+
+#define VIR_FROM_THIS VIR_FROM_NONE
+
+#define datadir abs_builddir "/virstoragedata"
+
+/* This test creates the following files, all in datadir:
+
+ * raw: 1024-byte raw file
+ * qcow2: qcow2 file with 'raw' as backing
+ * wrap: qcow2 file with 'qcow2' as backing
+ * qed: qed file with 'raw' as backing
+ * sub/link1: symlink to qcow2
+ * sub/link2: symlink to wrap
+ *
+ * Relative names to these files are known at compile time, but absolute
+ * and canonical names depend on where the test is run; for convenience,
+ * we pre-populate the computation of these names for use during the test.
+*/
+
+static char *qemuimg;
+static char *absraw;
+static char *canonraw;
+static char *absqcow2;
+static char *canonqcow2;
+static char *abswrap;
+static char *absqed;
+static char *abslink2;
+
+static void
+testCleanupImages(void)
+{
+ virCommandPtr cmd;
+
+ VIR_FREE(qemuimg);
+ VIR_FREE(absraw);
+ VIR_FREE(canonraw);
+ VIR_FREE(absqcow2);
+ VIR_FREE(canonqcow2);
+ VIR_FREE(abswrap);
+ VIR_FREE(absqed);
+ VIR_FREE(abslink2);
+
+ if (chdir(abs_builddir) < 0) {
+ fprintf(stderr, "unable to return to correct directory, refusing to "
+ "clean up %s\n", datadir);
+ return;
+ }
+
+ cmd = virCommandNewArgList("rm", "-rf", datadir, NULL);
+ ignore_value(virCommandRun(cmd, NULL));
+ virCommandFree(cmd);
+}
+
+static int
+testPrepImages(void)
+{
+ int ret = EXIT_FAILURE;
+ virCommandPtr cmd = NULL;
+
+ qemuimg = virFindFileInPath("kvm-img");
+ if (!qemuimg)
+ qemuimg = virFindFileInPath("qemu-img");
+ if (!qemuimg) {
+ fprintf(stderr, "qemu-img missing or too old; skipping this test\n");
+ return EXIT_AM_SKIP;
+ }
+
+ if (virAsprintf(&absraw, "%s/raw", datadir) < 0 ||
+ virAsprintf(&absqcow2, "%s/qcow2", datadir) < 0 ||
+ virAsprintf(&abswrap, "%s/wrap", datadir) < 0 ||
+ virAsprintf(&absqed, "%s/qed", datadir) < 0 ||
+ virAsprintf(&abslink2, "%s/sub/link2", datadir) < 0) {
+ virReportOOMError();
+ goto cleanup;
+ }
+
+ if (virFileMakePath(datadir "/sub") < 0) {
+ fprintf(stderr, "unable to create directory %s\n", datadir
"/sub");
+ goto cleanup;
+ }
+
+ if (chdir(datadir) < 0) {
+ fprintf(stderr, "unable to test relative backing chains\n");
+ goto cleanup;
+ }
+
+ /* I'm lazy enough to use a shell one-liner instead of open/write/close */
+ virCommandFree(cmd);
+ cmd = virCommandNewArgList("sh", "-c", "printf %1024d 0 >
raw", NULL);
+ if (virCommandRun(cmd, NULL) < 0)
+ goto cleanup;
+ if (!(canonraw = canonicalize_file_name(absraw))) {
+ virReportOOMError();
+ goto cleanup;
+ }
+
+ /* Create a qcow2 wrapping relative raw; later on, we modify its
+ * metadata to test other configurations */
+ virCommandFree(cmd);
+ cmd = virCommandNewArgList("qemu-img", "create", "-f",
"qcow2",
+ "-obacking_file=raw,backing_fmt=raw",
"qcow2",
+ NULL);
+ if (virCommandRun(cmd, NULL) < 0)
+ goto cleanup;
+ /* Make sure our later uses of 'qemu-img rebase' will work */
+ virCommandFree(cmd);
+ cmd = virCommandNewArgList("qemu-img", "rebase", "-u",
"-f", "qcow2",
+ "-F", "raw", "-b",
"raw", "qcow2", NULL);
+ if (virCommandRun(cmd, NULL) < 0) {
+ fprintf(stderr, "qemu-img is too old; skipping this test\n");
+ ret = EXIT_AM_SKIP;
+ goto cleanup;
+ }
+ if (!(canonqcow2 = canonicalize_file_name(absqcow2))) {
+ virReportOOMError();
+ goto cleanup;
+ }
+
+ /* Create a second qcow2 wrapping the first, to be sure that we
+ * can correctly avoid insecure probing. */
+ virCommandFree(cmd);
+ cmd = virCommandNewArgList("qemu-img", "create", "-f",
"qcow2", NULL);
+ virCommandAddArgFormat(cmd, "-obacking_file=%s,backing_fmt=qcow2",
+ absqcow2);
+ virCommandAddArg(cmd, "wrap");
+ if (virCommandRun(cmd, NULL) < 0)
+ goto cleanup;
+
+ /* Create a qed file. */
+ virCommandFree(cmd);
+ cmd = virCommandNewArgList("qemu-img", "create", "-f",
"qed", NULL);
+ virCommandAddArgFormat(cmd, "-obacking_file=%s,backing_fmt=raw",
+ absraw);
+ virCommandAddArg(cmd, "qed");
+ if (virCommandRun(cmd, NULL) < 0)
+ goto cleanup;
+
+ /* Create some symlinks in a sub-directory. */
+ if (symlink("../qcow2", datadir "/sub/link1") < 0 ||
+ symlink("../wrap", datadir "/sub/link2") < 0) {
+ fprintf(stderr, "unable to create symlink");
+ goto cleanup;
+ }
+
+ ret = 0;
+cleanup:
+ virCommandFree(cmd);
+ if (ret)
+ testCleanupImages();
+ return ret;
+}
+
+typedef struct _testFileData testFileData;
+struct _testFileData
+{
+ const char *expBackingStore;
+ const char *expBackingStoreRaw;
+ const char *expDirectory;
+ enum virStorageFileFormat expFormat;
+ bool expIsFile;
+ unsigned long long expCapacity;
+ bool expEncrypted;
+};
+
+enum {
+ EXP_PASS = 0,
+ EXP_FAIL = 1,
+ EXP_WARN = 2,
+ ALLOW_PROBE = 4,
+};
+
+struct testChainData
+{
+ const char *start;
+ enum virStorageFileFormat format;
+ const testFileData *files;
+ int nfiles;
+ unsigned int flags;
+};
+
+static int
+testStorageChain(const void *args)
+{
+ const struct testChainData *data = args;
+ int ret = -1;
+ virStorageFileMetadataPtr meta;
+ virStorageFileMetadataPtr elt;
+ int i = 0;
+
+ meta = virStorageFileGetMetadata(data->start, data->format, -1, -1,
+ (data->flags & ALLOW_PROBE) != 0);
+ if (!meta) {
+ if (data->flags & EXP_FAIL) {
+ virResetLastError();
+ ret = 0;
+ }
+ goto cleanup;
+ } else if (data->flags & EXP_FAIL) {
+ fprintf(stderr, "call should have failed\n");
+ goto cleanup;
+ }
+ if (data->flags & EXP_WARN)
+ virResetLastError();
+
+ elt = meta;
+ while (elt) {
+ char *expect = NULL;
+ char *actual = NULL;
+
+ if (i == data->nfiles) {
+ fprintf(stderr, "probed chain was too long\n");
+ goto cleanup;
+ }
+
+ if (virAsprintf(&expect,
+ "store:%s\nraw:%s\ndirectory:%s\nother:%d %d %lld %d",
+ NULLSTR(data->files[i].expBackingStore),
+ NULLSTR(data->files[i].expBackingStoreRaw),
+ NULLSTR(data->files[i].expDirectory),
+ data->files[i].expFormat,
+ data->files[i].expIsFile,
+ data->files[i].expCapacity,
+ data->files[i].expEncrypted) < 0 ||
+ virAsprintf(&actual,
+ "store:%s\nraw:%s\ndirectory:%s\nother:%d %d %lld %d",
+ NULLSTR(elt->backingStore),
+ NULLSTR(elt->backingStoreRaw),
+ NULLSTR(elt->directory),
+ elt->backingStoreFormat, elt->backingStoreIsFile,
+ elt->capacity, elt->encrypted) < 0) {
+ virReportOOMError();
+ VIR_FREE(expect);
+ VIR_FREE(actual);
+ goto cleanup;
+ }
+ if (STRNEQ(expect, actual)) {
+ virtTestDifference(stderr, expect, actual);
+ VIR_FREE(expect);
+ VIR_FREE(actual);
+ goto cleanup;
+ }
+ VIR_FREE(expect);
+ VIR_FREE(actual);
+ elt = elt->backingMeta;
+ i++;
+ }
+ if (i != data->nfiles) {
+ fprintf(stderr, "probed chain was too short\n");
+ goto cleanup;
+ }
+
+ ret = 0;
+cleanup:
+ virStorageFileFreeMetadata(meta);
+ return ret;
+}
+
+static int
+mymain(void)
+{
+ int ret;
+ virCommandPtr cmd = NULL;
+
+ /* Prep some files with qemu-img; if that is not found on PATH, or
+ * if it lacks support for qcow2 and qed, skip this test. */
+ if ((ret = testPrepImages()) != 0)
+ return ret;
+
+#define TEST_ONE_CHAIN(id, start, format, chain, flags) \
+ do { \
+ struct testChainData data = { \
+ start, format, chain, ARRAY_CARDINALITY(chain), flags, \
+ }; \
+ if (virtTestRun("Storage backing chain " id, 1, \
+ testStorageChain, &data) < 0) \
+ ret = -1; \
+ } while (0)
+
+#define TEST_CHAIN(id, relstart, absstart, format, chain1, flags1, \
+ chain2, flags2, chain3, flags3, chain4, flags4) \
+ do { \
+ TEST_ONE_CHAIN(#id "a", relstart, format, chain1, flags1); \
+ TEST_ONE_CHAIN(#id "b", relstart, format, chain2, flags2); \
+ TEST_ONE_CHAIN(#id "c", absstart, format, chain3, flags3); \
+ TEST_ONE_CHAIN(#id "d", absstart, format, chain4, flags4); \
+ } while (0)
+
+ /* Expected details about files in chains */
+ const testFileData raw = {
+ NULL, NULL, NULL, VIR_STORAGE_FILE_NONE, false, 0, false,
+ };
+ const testFileData qcow2_relback_relstart = {
+ canonraw, "raw", ".", VIR_STORAGE_FILE_RAW, true, 1024,
false,
+ };
+ const testFileData qcow2_relback_absstart = {
+ canonraw, "raw", datadir, VIR_STORAGE_FILE_RAW, true, 1024, false,
+ };
+ const testFileData qcow2_absback = {
+ canonraw, absraw, datadir, VIR_STORAGE_FILE_RAW, true, 1024, false,
+ };
+ const testFileData qcow2_as_probe = {
+ canonraw, absraw, datadir, VIR_STORAGE_FILE_AUTO, true, 1024, false,
+ };
+ const testFileData qcow2_bogus = {
+ NULL, datadir "/bogus", datadir, VIR_STORAGE_FILE_NONE,
+ false, 1024, false,
+ };
+ const testFileData qcow2_protocol = {
+ "nbd:example.org:6000", NULL, NULL, VIR_STORAGE_FILE_RAW,
+ false, 1024, false,
+ };
+ const testFileData wrap = {
+ canonqcow2, absqcow2, datadir, VIR_STORAGE_FILE_QCOW2,
+ true, 1024, false,
+ };
+ const testFileData wrap_as_raw = {
+ canonqcow2, absqcow2, datadir, VIR_STORAGE_FILE_RAW,
+ true, 1024, false,
+ };
+ const testFileData wrap_as_probe = {
+ canonqcow2, absqcow2, datadir, VIR_STORAGE_FILE_AUTO,
+ true, 1024, false,
+ };
+ const testFileData qed = {
+ canonraw, absraw, datadir, VIR_STORAGE_FILE_RAW,
+ true, 1024, false,
+ };
+ const testFileData link1_rel = {
+ canonraw, "../raw", "sub/../sub/..", VIR_STORAGE_FILE_RAW,
+ true, 1024, false,
+ };
+ const testFileData link1_abs = {
+ canonraw, "../raw", datadir "/sub/../sub/..",
VIR_STORAGE_FILE_RAW,
+ true, 1024, false,
+ };
+ const testFileData link2_rel = {
+ canonqcow2, "../sub/link1", "sub/../sub",
VIR_STORAGE_FILE_QCOW2,
+ true, 1024, false,
+ };
+ const testFileData link2_abs = {
+ canonqcow2, "../sub/link1", datadir "/sub/../sub",
+ VIR_STORAGE_FILE_QCOW2, true, 1024, false,
+ };
+
+ /* The actual tests, in several groups. */
+
+ /* Missing file */
+ const testFileData chain0[] = { };
+ TEST_ONE_CHAIN("0", "bogus", VIR_STORAGE_FILE_RAW, chain0,
EXP_FAIL);
+
+ /* Raw image, whether with right format or no specified format */
+ const testFileData chain1[] = { raw };
+ TEST_CHAIN(1, "raw", absraw, VIR_STORAGE_FILE_RAW,
+ chain1, EXP_PASS,
+ chain1, ALLOW_PROBE | EXP_PASS,
+ chain1, EXP_PASS,
+ chain1, ALLOW_PROBE | EXP_PASS);
+ TEST_CHAIN(2, "raw", absraw, VIR_STORAGE_FILE_AUTO,
+ chain1, EXP_PASS,
+ chain1, ALLOW_PROBE | EXP_PASS,
+ chain1, EXP_PASS,
+ chain1, ALLOW_PROBE | EXP_PASS);
+
+ /* Qcow2 file with relative raw backing, format provided */
+ const testFileData chain3a[] = { qcow2_relback_relstart, raw };
+ const testFileData chain3c[] = { qcow2_relback_absstart, raw };
+ const testFileData chain4a[] = { raw };
+ TEST_CHAIN(3, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2,
+ chain3a, EXP_PASS,
+ chain3a, ALLOW_PROBE | EXP_PASS,
+ chain3c, EXP_PASS,
+ chain3c, ALLOW_PROBE | EXP_PASS);
+ TEST_CHAIN(4, "qcow2", absqcow2, VIR_STORAGE_FILE_AUTO,
+ chain4a, EXP_PASS,
+ chain3a, ALLOW_PROBE | EXP_PASS,
+ chain4a, EXP_PASS,
+ chain3c, ALLOW_PROBE | EXP_PASS);
+
+ /* Rewrite qcow2 file to use absolute backing name */
+ virCommandFree(cmd);
+ cmd = virCommandNewArgList(qemuimg, "rebase", "-u",
"-f", "qcow2",
+ "-F", "raw", "-b", absraw,
"qcow2", NULL);
+ if (virCommandRun(cmd, NULL) < 0)
+ ret = -1;
+
+ /* Qcow2 file with raw as absolute backing, backing format provided */
+ const testFileData chain5[] = { qcow2_absback, raw };
+ const testFileData chain6[] = { raw };
+ TEST_CHAIN(5, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2,
+ chain5, EXP_PASS,
+ chain5, ALLOW_PROBE | EXP_PASS,
+ chain5, EXP_PASS,
+ chain5, ALLOW_PROBE | EXP_PASS);
+ TEST_CHAIN(6, "qcow2", absqcow2, VIR_STORAGE_FILE_AUTO,
+ chain6, EXP_PASS,
+ chain5, ALLOW_PROBE | EXP_PASS,
+ chain6, EXP_PASS,
+ chain5, ALLOW_PROBE | EXP_PASS);
+
+ /* Wrapped file access */
+ const testFileData chain7[] = { wrap, qcow2_absback, raw };
+ TEST_CHAIN(7, "wrap", abswrap, VIR_STORAGE_FILE_QCOW2,
+ chain7, EXP_PASS,
+ chain7, ALLOW_PROBE | EXP_PASS,
+ chain7, EXP_PASS,
+ chain7, ALLOW_PROBE | EXP_PASS);
+
+ /* Rewrite qcow2 and wrap file to omit backing file type */
+ virCommandFree(cmd);
+ cmd = virCommandNewArgList(qemuimg, "rebase", "-u",
"-f", "qcow2",
+ "-b", absraw, "qcow2", NULL);
+ if (virCommandRun(cmd, NULL) < 0)
+ ret = -1;
+
+ virCommandFree(cmd);
+ cmd = virCommandNewArgList(qemuimg, "rebase", "-u",
"-f", "qcow2",
+ "-b", absqcow2, "wrap", NULL);
+ if (virCommandRun(cmd, NULL) < 0)
+ ret = -1;
+
+ /* Qcow2 file with raw as absolute backing, backing format omitted */
+ const testFileData chain8a[] = { wrap_as_raw, raw };
+ const testFileData chain8b[] = { wrap_as_probe, qcow2_as_probe, raw };
+ TEST_CHAIN(8, "wrap", abswrap, VIR_STORAGE_FILE_QCOW2,
+ chain8a, EXP_PASS,
+ chain8b, ALLOW_PROBE | EXP_PASS,
+ chain8a, EXP_PASS,
+ chain8b, ALLOW_PROBE | EXP_PASS);
+
+ /* Rewrite qcow2 to a missing backing file, with backing type */
+ virCommandFree(cmd);
+ cmd = virCommandNewArgList(qemuimg, "rebase", "-u",
"-f", "qcow2",
+ "-F", "qcow2", "-b", datadir
"/bogus",
+ "qcow2", NULL);
+ if (virCommandRun(cmd, NULL) < 0)
+ ret = -1;
+
+ /* Qcow2 file with missing backing file but specified type */
+ const testFileData chain9[] = { qcow2_bogus };
+ TEST_CHAIN(9, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2,
+ chain9, EXP_WARN,
+ chain9, ALLOW_PROBE | EXP_WARN,
+ chain9, EXP_WARN,
+ chain9, ALLOW_PROBE | EXP_WARN);
+
+ /* Rewrite qcow2 to a missing backing file, without backing type */
+ virCommandFree(cmd);
+ cmd = virCommandNewArgList(qemuimg, "rebase", "-u",
"-f", "qcow2",
+ "-b", datadir "/bogus",
"qcow2", NULL);
+ if (virCommandRun(cmd, NULL) < 0)
+ ret = -1;
+
+ /* Qcow2 file with missing backing file and no specified type */
+ const testFileData chain10[] = { qcow2_bogus };
+ TEST_CHAIN(10, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2,
+ chain10, EXP_WARN,
+ chain10, ALLOW_PROBE | EXP_WARN,
+ chain10, EXP_WARN,
+ chain10, ALLOW_PROBE | EXP_WARN);
+
+ /* Rewrite qcow2 to use an nbd: protocol as backend */
+ virCommandFree(cmd);
+ cmd = virCommandNewArgList(qemuimg, "rebase", "-u",
"-f", "qcow2",
+ "-F", "raw", "-b",
"nbd:example.org:6000",
+ "qcow2", NULL);
+ if (virCommandRun(cmd, NULL) < 0)
+ ret = -1;
+
+ /* Qcow2 file with backing protocol instead of file */
+ const testFileData chain11[] = { qcow2_protocol };
+ TEST_CHAIN(11, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2,
+ chain11, EXP_WARN,
+ chain11, ALLOW_PROBE | EXP_WARN,
+ chain11, EXP_WARN,
+ chain11, ALLOW_PROBE | EXP_WARN);
+
+ /* qed file */
+ const testFileData chain12a[] = { raw };
+ const testFileData chain12b[] = { qed, raw };
+ TEST_CHAIN(12, "qed", absqed, VIR_STORAGE_FILE_AUTO,
+ chain12a, EXP_PASS,
+ chain12b, ALLOW_PROBE | EXP_PASS,
+ chain12a, EXP_PASS,
+ chain12b, ALLOW_PROBE | EXP_PASS);
+
+ /* Rewrite qcow2 and wrap file to use backing names relative to a
+ * symlink from a different directory */
+ virCommandFree(cmd);
+ cmd = virCommandNewArgList(qemuimg, "rebase", "-u",
"-f", "qcow2",
+ "-F", "raw", "-b",
"../raw", "qcow2", NULL);
+ if (virCommandRun(cmd, NULL) < 0)
+ ret = -1;
+
+ virCommandFree(cmd);
+ cmd = virCommandNewArgList(qemuimg, "rebase", "-u",
"-f", "qcow2",
+ "-F", "qcow2", "-b",
"../sub/link1", "wrap",
+ NULL);
+ if (virCommandRun(cmd, NULL) < 0)
+ ret = -1;
+
+ /* Behavior of symlinks to qcow2 with relative backing files */
+ const testFileData chain13a[] = { link2_rel, link1_rel, raw };
+ const testFileData chain13c[] = { link2_abs, link1_abs, raw };
+ TEST_CHAIN(13, "sub/link2", abslink2, VIR_STORAGE_FILE_QCOW2,
+ chain13a, EXP_PASS,
+ chain13a, ALLOW_PROBE | EXP_PASS,
+ chain13c, EXP_PASS,
+ chain13c, ALLOW_PROBE | EXP_PASS);
+
+ /* Final cleanup */
+ testCleanupImages();
+ virCommandFree(cmd);
+
+ return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
+}
+
+VIRT_TEST_MAIN(mymain)
--
1.8.1.2
4:26 p.m.
New subject: [libvirt] [PATCHv2 5/5] storage: test backing chain traversal
On 02/15/13 21:38, Eric Blake wrote:
Testing our backing chain handling will make it much easier to
ensure that we avoid issues in the future. If only I had written
this test before I first caused several regressions...
* tests/virstoragetest.c: New test.
* tests/Makefile.am (test_programs): Build it.
* .gitignore: Ignore new files.
---
.gitignore | 1 +
tests/Makefile.am | 6 +
tests/virstoragetest.c | 546 +++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 553 insertions(+)
create mode 100644 tests/virstoragetest.c
I just skimmed through this test as I don't feel well enough to review
it thoroughly. It seems OK to me and it compiles and runs okay so I give
a weak ACK.
Peter
4:43 p.m.
New subject: [libvirt] [PATCHv2 5/5] storage: test backing chain traversal
On 02/15/2013 03:26 PM, Peter Krempa wrote:
On 02/15/13 21:38, Eric Blake wrote:
> Testing our backing chain handling will make it much easier to
> ensure that we avoid issues in the future. If only I had written
> this test before I first caused several regressions...
>
> * tests/virstoragetest.c: New test.
> * tests/Makefile.am (test_programs): Build it.
> * .gitignore: Ignore new files.
> ---
> .gitignore | 1 +
> tests/Makefile.am | 6 +
> tests/virstoragetest.c | 546
> +++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 553 insertions(+)
> create mode 100644 tests/virstoragetest.c
I just skimmed through this test as I don't feel well enough to review
it thoroughly. It seems OK to me and it compiles and runs okay so I give
a weak ACK.
I did one more thing to strengthen the test; I'm squashing this before
pushing.
diff --git i/tests/virstoragetest.c w/tests/virstoragetest.c
index 6b4ca99..9da58f3 100644
--- i/tests/virstoragetest.c
+++ w/tests/virstoragetest.c
@@ -228,8 +228,16 @@ testStorageChain(const void *args)
fprintf(stderr, "call should have failed\n");
goto cleanup;
}
- if (data->flags & EXP_WARN)
+ if (data->flags & EXP_WARN) {
+ if (!virGetLastError()) {
+ fprintf(stderr, "call should have warned\n");
+ goto cleanup;
+ }
virResetLastError();
+ } else if (virGetLastError()) {
+ fprintf(stderr, "call should not have warned\n");
+ goto cleanup;
+ }
elt = meta;
while (elt) {
@@ -498,10 +506,10 @@ mymain(void)
/* Qcow2 file with backing protocol instead of file */
const testFileData chain11[] = { qcow2_protocol };
TEST_CHAIN(11, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2,
- chain11, EXP_WARN,
- chain11, ALLOW_PROBE | EXP_WARN,
- chain11, EXP_WARN,
- chain11, ALLOW_PROBE | EXP_WARN);
+ chain11, EXP_PASS,
+ chain11, ALLOW_PROBE | EXP_PASS,
+ chain11, EXP_PASS,
+ chain11, ALLOW_PROBE | EXP_PASS);
/* qed file */
const testFileData chain12a[] = { raw };
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
4299
days inactive
4299
days old
13 comments
2 participants
participants (2)
-
Eric Blake -
Peter Krempa