"Daniel P. Berrange" <berrange@xxxxxxxxxx> 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? -- Libvir-list mailing list Libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list