"Daniel P. Berrange" <berrange(a)redhat.com> wrote:
This is a follow up on Miloslav's proposal to add copy on write
support to the storage APIs, changing the XML to that described
here:
http://www.redhat.com/archives/libvir-list/2009-January/msg00231.html
In addition to the original QCOW/VMDK support, I have done an impl which
can extract the backing store for LVM volumes
This looks fine, but I'd feel a lot better about it
if there were a few tests of the new bits.
A few questions/suggestions:
diff --git a/src/storage_backend.c b/src/storage_backend.c
...
@@ -235,38 +263,38 @@ virStorageBackendUpdateVolInfoFD(virConn
continue;
if (memcmp(buffer+disk_types[i].offset, &disk_types[i].magic,
disk_types[i].length) == 0) {
- vol->target.format = disk_types[i].part_table_type;
+ target->format = disk_types[i].part_table_type;
break;
}
}
}
- vol->target.perms.mode = sb.st_mode;
- vol->target.perms.uid = sb.st_uid;
- vol->target.perms.gid = sb.st_gid;
+ target->perms.mode = sb.st_mode & (0x200-1);
How about S_IRWXUGO:
target->perms.mode = sb.st_mode & S_IRWXUGO;
+ target->perms.uid = sb.st_uid;
+ target->perms.gid = sb.st_gid;
...
diff --git a/src/storage_backend_fs.c b/src/storage_backend_fs.c
...
+static int
+cowGetBackingStore(virConnectPtr conn,
+ char **res,
+ const unsigned char *buf,
+ size_t buf_size)
+{
+ size_t len;
+
+ *res = NULL;
+ if (buf_size < 4+4+1024)
+ return BACKING_STORE_INVALID;
+ if (buf[4+4] == '\0') /* cow_header_v2.backing_file[0] */
+ return BACKING_STORE_OK;
+
+ len = 1024;
+ if (VIR_ALLOC_N(*res, len + 1) < 0) {
+ virStorageReportError(conn, VIR_ERR_NO_MEMORY, _("backing store
path"));
+ return BACKING_STORE_ERROR;
+ }
+ memcpy(*res, buf + 4+4, len); /* cow_header_v2.backing_file */
+ (*res)[len] = '\0';
+ if (VIR_REALLOC_N(*res, strlen(*res) + 1) < 0) {
Is this just-copied 1024-byte block of data guaranteed
not to contain any NUL bytes? Or maybe you just want that
NUL-terminated string?
+ /* Ignore failure */
+ }
+ return BACKING_STORE_OK;
+}
...
@@ -846,17 +1081,36 @@ virStorageBackendFileSystemVolCreate(vir
vol->target.format);
return -1;
}
+ if (vol->backingStore.path) {
+ if ((backingType =
virStorageVolFormatFileSystemTypeToString(vol->backingStore.format)) == NULL) {
+ virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
+ _("unknown storage vol backing store type
%d"),
+ vol->backingStore.format);
+ return -1;
+ }
+ if (access(vol->backingStore.path, R_OK) != 0) {
+ virReportSystemError(conn, errno,
+ _("inaccessible backing store volume
%s"),
+ vol->backingStore.path);
+ return -1;
+ }
+ }
/* Size in KB */
snprintf(size, sizeof(size), "%llu", vol->capacity/1024);
- imgargv[0] = QEMU_IMG;
- imgargv[1] = "create";
- imgargv[2] = "-f";
- imgargv[3] = type;
- imgargv[4] = vol->target.path;
- imgargv[5] = size;
- imgargv[6] = NULL;
+ i = 0;
+ imgargv[i++] = QEMU_IMG;
+ imgargv[i++] = "create";
+ imgargv[i++] = "-f";
+ imgargv[i++] = type;
+ if (vol->backingStore.path != NULL) {
+ imgargv[i++] = "-b";
+ imgargv[i++] = vol->backingStore.path;
+ }
+ imgargv[i++] = vol->target.path;
+ imgargv[i++] = size;
+ imgargv[i++] = NULL;
Add an assertion like
assert (i < ARRAY_CARDINALITY (imgargv));
Otherwise, it's far too easy to forget to update the "9"
above when adding a new option here.
...
diff --git a/src/storage_backend_iscsi.c
b/src/storage_backend_iscsi.c
--- a/src/storage_backend_iscsi.c
+++ b/src/storage_backend_iscsi.c
@@ -226,7 +226,11 @@ virStorageBackendISCSINewLun(virConnectP
VIR_FREE(devpath);
- if (virStorageBackendUpdateVolInfoFD(conn, vol, fd, 1) < 0)
+ if (virStorageBackendUpdateVolTargetInfoFD(conn,
+ &vol->target,
+ fd,
+ &vol->allocation,
+ &vol->capacity) < 0)
goto cleanup;
/* XXX use unique iSCSI id instead */
diff --git a/src/storage_backend_logical.c b/src/storage_backend_logical.c
--- a/src/storage_backend_logical.c
+++ b/src/storage_backend_logical.c
@@ -116,8 +116,22 @@ virStorageBackendLogicalMakeVol(virConne
strcat(vol->target.path, vol->name);
}
+ if (groups[1] && !STREQ(groups[1], "")) {
+ if (VIR_ALLOC_N(vol->backingStore.path, strlen(pool->def->target.path)
+
+ 1 + strlen(groups[1]) + 1) < 0) {
+ virStorageReportError(conn, VIR_ERR_NO_MEMORY, "%s",
_("volume"));
+ return -1;
+ }
+ strcpy(vol->backingStore.path, pool->def->target.path);
+ strcat(vol->backingStore.path, "/");
+ strcat(vol->backingStore.path, groups[1]);
How about using virAsprintf in place of VIR_ALLOC_N + strcpy + strcat?