On 09.02.2016 16:52, Olga Krishtal wrote:
> These callbacks let us to create ploop volumes in directory pools.
> If a ploop volume was created via buildVol callback, then this volume
> is an empty ploop device with DiskDescriptor.xml.
> If the volume was created via .buildFrom - then its content is the
> same as input volume.
>
> Ploop volume consists of ploop image file and a corresponding
> DiskDescriptor.xml file.
>
> Signed-off-by: Olga Krishtal <okrishtal@xxxxxxxxxxxxx>
> ---
> src/storage/storage_backend.c | 101 ++++++++++++++++++++++++++++++++++++++-
> src/storage/storage_backend.h | 8 ++++
> src/storage/storage_backend_fs.c | 2 +
> 3 files changed, 110 insertions(+), 1 deletion(-)
>
> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
> index c07b642..039a540 100644
> --- a/src/storage/storage_backend.c
> +++ b/src/storage/storage_backend.c
> @@ -773,6 +773,104 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool,
> return ret;
> }
>
> +/* Set up directory for ploop volume. If function fails,
> + * volume won't be created.
> + */
> +
> +static int
> +virVolCreateDirForPloop(virStoragePoolObjPtr pool ATTRIBUTE_UNUSED,
> + virStorageVolDefPtr vol)
> +{
> + int err;
> + if ((err = virDirCreate(vol->target.path,
> + (vol->target.perms->mode == (mode_t) -1 ?
> + VIR_STORAGE_DEFAULT_VOL_PERM_MODE:
> + vol->target.perms->mode),
inconvinient indentation, should at least be:
if ((err = virDirCreate(vol->target.path,
(vol->target.perms->mode == (mode_t) -1 ?
VIR_STORAGE_DEFAULT_VOL_PERM_MODE:
vol->target.perms->mode),
....
that is extra space to indent every nesting. Or you
can calc this parameter outside as in other places.
> + vol->target.perms->uid,
> + vol->target.perms->gid,
> + 0)) < 0) {
> + return -1;
> + }
> + return 0;
> +}
looks like not too useful to be a distinct function.
> +
> +int virStorageBackendCreatePloop(virConnectPtr conn ATTRIBUTE_UNUSED,
> + virStoragePoolObjPtr pool,
> + virStorageVolDefPtr vol,
> + virStorageVolDefPtr inputvol,
> + unsigned int flags)
arguments indentation
> +{
> + int ret = 0;
better set this to -1, it's common idiom
> + char *size = NULL;
> + char *path = NULL;
> + virCommandPtr cmd = NULL;
> + char *create_tool = NULL;
> +
> + virCheckFlags(0, -1);
> +
> + create_tool = virFindFileInPath("ploop");
> + if (!create_tool && !inputvol) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + "%s", _("unable to find ploop, please install "
> + "ploop tools"));
> + }
consider next indentation:
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("unable to find ploop, please install ploop tools"));
and return -1!
> +
> + if (vol->target.format != VIR_STORAGE_FILE_PLOOP) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("unsupported storage vol type %d"),
> + vol->target.format);
> + return -1;
> + }
looks likes this check will never trigger, could be removed
> + if (inputvol && inputvol->target.format != VIR_STORAGE_FILE_PLOOP) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("unsupported input storage vol type %d"),
> + inputvol->target.format);
> + return -1;
> + }
> +
> + if (vol->target.encryption != NULL) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + "%s", _("encrypted ploop volumes are not supported with "
> + "ploop init"));
consider this indentation
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("encrypted ploop volumes are not supported with "
"ploop init"));
> + return -1;
> + }
> +
> + if (vol->target.backingStore != NULL) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("copy-on-write ploop volumes are not yet supported"));
indentation
> + return -1;
> + }
> +
> + if (!inputvol) {
> + if (virVolCreateDirForPloop(pool, vol) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + "%s", _("error creating directory for ploop volume"));
indentation
> + return -1;
> + }
> + if (virAsprintf(&path, "%s/%s", vol->target.path, "root.hds") < 0)
"%s/root.hds"
> + return -1;
> +
> + if (virAsprintf(&size, "%lluM", VIR_DIV_UP(vol->target.capacity, (1024*1024))) < 0) {
spaces in "1024 * 1024"
line length
> + ret = -1;
> + goto cleanup;
> + }
> +
> + cmd = virCommandNewArgList(create_tool, "init", "-s", size, "-t", "ext4", path, NULL);
line length
> + } else {
> + vol->target.capacity = inputvol->target.capacity;
qcow2 code does not set this, why do we need this?
> + cmd = virCommandNewArgList("cp", "-r", inputvol->target.path, vol->target.path, NULL);
line length
> + }
> + ret = virCommandRun(cmd, NULL);
> +
> + cleanup:
we need to cleanup filesystem too.
1. 'cp' does not cleanup by itself
2. if input volume is NULL we can create directory and then fail to init ploop and we leave directory on disk
on exit.
> +
> + virCommandFree(cmd);
> + VIR_FREE(size);
> + VIR_FREE(path);
> + VIR_FREE(create_tool);
> + return ret;
> +}
> +
> enum {
> QEMU_IMG_BACKING_FORMAT_NONE = 0,
> QEMU_IMG_BACKING_FORMAT_FLAG,
> @@ -1280,7 +1378,8 @@ virStorageBackendGetBuildVolFromFunction(virStorageVolDefPtr vol,
> vol->target.format != VIR_STORAGE_FILE_RAW) ||
> (inputvol->type == VIR_STORAGE_VOL_FILE &&
> inputvol->target.format != VIR_STORAGE_FILE_RAW)) {
> -
> + if (vol->target.format == VIR_STORAGE_FILE_PLOOP)
> + return virStorageBackendCreatePloop;
> if ((tool_type = virStorageBackendFindFSImageTool(NULL)) < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("creation of non-raw file images is "
> diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h
> index 20e6079..852d6ed 100644
> --- a/src/storage/storage_backend.h
> +++ b/src/storage/storage_backend.h
> @@ -108,6 +108,14 @@ int virStorageBackendCreateRaw(virConnectPtr conn,
> virStorageVolDefPtr vol,
> virStorageVolDefPtr inputvol,
> unsigned int flags);
> +
> +int virStorageBackendCreatePloop(virConnectPtr conn,
> + virStoragePoolObjPtr pool,
> + virStorageVolDefPtr vol,
> + virStorageVolDefPtr inputvol,
> + unsigned int flags);
> +
> +
> virStorageBackendBuildVolFrom
> virStorageBackendGetBuildVolFromFunction(virStorageVolDefPtr vol,
> virStorageVolDefPtr inputvol);
> diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
> index 692c9ff..80c7e9e 100644
> --- a/src/storage/storage_backend_fs.c
> +++ b/src/storage/storage_backend_fs.c
> @@ -1177,6 +1177,8 @@ _virStorageBackendFileSystemVolBuild(virConnectPtr conn,
> create_func = virStorageBackendCreateRaw;
> } else if (vol->target.format == VIR_STORAGE_FILE_DIR) {
> create_func = createFileDir;
> + } else if (vol->target.format == VIR_STORAGE_FILE_PLOOP) {
> + create_func = virStorageBackendCreatePloop;
> } else if ((tool_type = virStorageBackendFindFSImageTool(NULL)) != -1) {
> create_func = virStorageBackendFSImageToolTypeToFunc(tool_type);
>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list