[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] PATCH: Support storage copy on write volumes



"Daniel P. Berrange" <berrange 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?


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]