On 10/02/16 12:43, Nikolay Shirokovskiy wrote:

On 09.02.2016 16:52, Olga Krishtal wrote:

...

Deletes whole directory of a ploop volume. To delete ploop image it has to be unmounted.

Signed-off-by: Olga Krishtal <okrishtal@virtuozzo.com> --- src/storage/storage_backend.c | 6 ++++++ src/storage/storage_backend.h | 2 ++ src/storage/storage_backend_fs.c | 5 +++++ 3 files changed, 13 insertions(+)

diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 039a540..34c9d40 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -871,6 +871,12 @@ int virStorageBackendCreatePloop(virConnectPtr conn ATTRIBUTE_UNUSED, return ret; }

+int virStorageBackendDeletePloop(virConnectPtr conn ATTRIBUTE_UNUSED, + virStorageVolDefPtr vol) +{ + return virFileDeleteTree(vol->target.path); +} + enum { QEMU_IMG_BACKING_FORMAT_NONE = 0, QEMU_IMG_BACKING_FORMAT_FLAG, diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 852d6ed..7d354c5 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -115,6 +115,8 @@ int virStorageBackendCreatePloop(virConnectPtr conn, virStorageVolDefPtr inputvol, unsigned int flags);

+int virStorageBackendDeletePloop(virConnectPtr conn, + virStorageVolDefPtr vol);

virStorageBackendBuildVolFrom virStorageBackendGetBuildVolFromFunction(virStorageVolDefPtr vol, diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 80c7e9e..2290096 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -1245,6 +1245,11 @@ virStorageBackendFileSystemVolDelete(virConnectPtr conn ATTRIBUTE_UNUSED, switch ((virStorageVolType) vol->type) { case VIR_STORAGE_VOL_FILE: case VIR_STORAGE_VOL_DIR: + if (vol->target.format == VIR_STORAGE_FILE_PLOOP) { + if (virStorageBackendDeletePloop(conn, vol) < 0) + return -1; + break; break at this level is not easy to follow. Can we introduce a function which will have another switch inside for types of volume types?

If I introduce one more function, we will suffer from one extra call every vol-delete call. Moreover, it won't change the appearance: case VIR_STORAGE_VOL_FILE: case VIR_STORAGE_VOL_DIR: + if (!extra_switch_func()) { + return -1; + break So, it looks the same.

...

+ } if (virFileRemove(vol->target.path, vol->target.perms->uid, vol->target.perms->gid) < 0) { /* Silently ignore failures where the vol has already gone away */

On 09.02.2016 16:52, Olga Krishtal wrote:

In case of ploop volume, target path of the volume is the path to the directory that contains image file named root.hds and DiskDescriptor.xml. While using uploadVol and downloadVol callbacks we need to open root.hds itself. To accomplish this goal we must change path from path/to/ploop directory to path/to/ploop/root.hds

Signed-off-by: Olga Krishtal <okrishtal@virtuozzo.com> --- src/storage/storage_backend.c | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 34c9d40..c455908 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -2025,12 +2025,23 @@ virStorageBackendVolUploadLocal(virConnectPtr conn ATTRIBUTE_UNUSED, unsigned long long len, unsigned int flags) { + char *path = NULL; + char *target_path = vol->target.path; + int ret; + virCheckFlags(0, -1); + if (vol->target.format == VIR_STORAGE_FILE_PLOOP) { + if (virAsprintf(&path, "%s/%s", vol->target.path, "root.hds") < 0) + return -1; + target_path = path; + }

/* Not using O_CREAT because the file is required to already exist at * this point */ - return virFDStreamOpenBlockDevice(stream, vol->target.path, + ret = virFDStreamOpenBlockDevice(stream, target_path, offset, len, O_WRONLY); + VIR_FREE(path); + return ret; }

How this upload/download API is used? Can we upload "hds" file and go out of sync with disk descriptor?

int @@ -2042,10 +2053,21 @@ virStorageBackendVolDownloadLocal(virConnectPtr conn ATTRIBUTE_UNUSED, unsigned long long len, unsigned int flags) { + char *path = NULL; + char *target_path = vol->target.path; + int ret; + virCheckFlags(0, -1); + if (vol->target.format == VIR_STORAGE_FILE_PLOOP) { + if (virAsprintf(&path, "%s/%s", vol->target.path, "root.hds") < 0) + return -1; + target_path = path; + }

- return virFDStreamOpenBlockDevice(stream, vol->target.path, + ret = virFDStreamOpenBlockDevice(stream, target_path, offset, len, O_RDONLY); + VIR_FREE(path); + return ret; }

On 09.02.2016 16:52, Olga Krishtal wrote:

To change the size of ploop image file we use ploop resize cmd that takes 2 args: new size and path/to/DiskDescriptor.xml

Signed-off-by: Olga Krishtal <okrishtal@virtuozzo.com> --- src/storage/storage_backend.c | 28 ++++++++++++++++++++++++++++ src/storage/storage_backend.h | 5 +++++ src/storage/storage_backend_fs.c | 2 ++ 3 files changed, 35 insertions(+)

diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index c455908..fb4d1b9 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -877,6 +877,34 @@ int virStorageBackendDeletePloop(virConnectPtr conn ATTRIBUTE_UNUSED, return virFileDeleteTree(vol->target.path); }

+int virStoragePloopResize(virConnectPtr conn ATTRIBUTE_UNUSED, + virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, + virStorageVolDefPtr vol, + unsigned long long capacity)

we can just omit 'conn' and 'pool' in arguments

+{ + int ret;

initialize to -1, then it would be easier to goto to cleanup.

+ char *path = NULL; + char *size = NULL; + virCommandPtr cmd = NULL; + + if (virAsprintf(&path, "%s/%s", vol->target.path, "DiskDescriptor.xml") < 0) + return -1; + + if (virAsprintf(&size, "%lluM", VIR_DIV_UP(capacity, 512)) < 0) {

You use suffix 'M' here thus you need to divide by 1024 * 1024 or leave the suffix.

+ ret = -1; + goto cleanup; + }

use virCommandAddArgFormat to add 'path' and 'size'. And "%s/DiskDescriptor.xml") by the way we can put all constants into #defines

+ cmd = virCommandNewArgList("ploop", "resize", "-s", size, path, NULL); + + ret = virCommandRun(cmd, NULL); + + cleanup: + virCommandFree(cmd); + VIR_FREE(path); + VIR_FREE(size); + return ret; +} + enum { QEMU_IMG_BACKING_FORMAT_NONE = 0, QEMU_IMG_BACKING_FORMAT_FLAG, diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 7d354c5..1de8dfe 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -118,6 +118,11 @@ int virStorageBackendCreatePloop(virConnectPtr conn, int virStorageBackendDeletePloop(virConnectPtr conn, virStorageVolDefPtr vol);

+int virStoragePloopResize(virConnectPtr conn, + virStoragePoolObjPtr pool, + virStorageVolDefPtr vol, + unsigned long long capacity); + virStorageBackendBuildVolFrom virStorageBackendGetBuildVolFromFunction(virStorageVolDefPtr vol, virStorageVolDefPtr inputvol); diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 2290096..c2d148d 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -1378,6 +1378,8 @@ virStorageBackendFileSystemVolResize(virConnectPtr conn ATTRIBUTE_UNUSED, if (vol->target.format == VIR_STORAGE_FILE_RAW) { return virStorageFileResize(vol->target.path, capacity, vol->target.allocation, pre_allocate); + } else if (vol->target.format == VIR_STORAGE_FILE_PLOOP) { + return virStoragePloopResize(conn, pool, vol, capacity); } else { if (pre_allocate) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",

On 09.02.2016 16:52, Olga Krishtal wrote:

Returns error in case of vol-wipe cmd for a ploop volume

Signed-off-by: Olga Krishtal <okrishtal@virtuozzo.com> --- src/storage/storage_backend.c | 6 ++++++ 1 file changed, 6 insertions(+)

diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index fb4d1b9..21dd96d 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -2217,6 +2217,12 @@ virStorageBackendVolWipeLocal(virConnectPtr conn ATTRIBUTE_UNUSED, VIR_DEBUG("Wiping volume with path '%s' and algorithm %u", vol->target.path, algorithm);

+ if (vol->target.format == VIR_STORAGE_FILE_PLOOP) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("wiping for ploop volumes is not supported")); + goto cleanup; + } + fd = open(vol->target.path, O_RDWR); if (fd == -1) { virReportSystemError(errno,

ACK

On 09.02.2016 16:52, Olga Krishtal wrote:

To update information about ploop volumes inside the a single pool we need to be sure that it is the ploop directory and not some other directory. Ploop volume directory obligatory contains root.hds - image file and disk descriptor - DiskDescriptor.xml. If path to a volume is a path to some directory, we check the existance of this files.

The capacity of a ploop volume is obtained via offset in the header file: https://openvz.org/Ploop/format

Signed-off-by: Olga Krishtal <okrishtal@virtuozzo.com> --- src/storage/storage_backend.c | 90 ++++++++++++++++++++++++++--------- src/storage/storage_backend.h | 2 +- src/storage/storage_backend_fs.c | 6 ++- src/storage/storage_backend_logical.c | 2 +- src/util/virstoragefile.c | 8 +++- 5 files changed, 81 insertions(+), 27 deletions(-)

diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 21dd96d..0c31f32 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1584,6 +1584,25 @@ virStorageBackendDetectBlockVolFormatFD(virStorageSourcePtr target, return 0; }

+static bool virStorageBackendIsPloopDir(char *path) +{ + char *file = NULL; + bool ret = false; + + if (virAsprintf(&file, "%s/%s", path, "root.hds") < 0) + return ret; + if (!virFileExists(file)) + goto cleanup; + VIR_FREE(file); + if (virAsprintf(&file, "%s/%s", path, "DiskDescriptor.xml") < 0) + goto cleanup; + if (!virFileExists(file)) + goto cleanup; + ret = true; + cleanup: + VIR_FREE(file); + return ret; +}

/* * Allows caller to silently ignore files with improper mode @@ -1592,29 +1611,35 @@ virStorageBackendDetectBlockVolFormatFD(virStorageSourcePtr target, * return -2 if file mode is unexpected or the volume is a dangling * symbolic link. */ + +#define FAILED_STAT(path, ret) { \ + if (errno == ENOENT) { \ + if (noerror) { \ + VIR_WARN("ignoring missing file '%s'", path);\ + ret = -2; \ + } \ + virReportError(VIR_ERR_NO_STORAGE_VOL, \ + _("no storage vol with matching path '%s'"), path); \ + ret = -1; \ + } \ + virReportSystemError(errno, \ + _("cannot stat file '%s'"), path);\ + ret = -1;\ +} + int -virStorageBackendVolOpen(const char *path, struct stat *sb, +virStorageBackendVolOpen(virStorageSourcePtr target, struct stat *sb, unsigned int flags) { - int fd, mode = 0; - char *base = last_component(path); + int fd, mode = 0, ret = 0; + char *base = last_component(target->path); bool noerror = (flags & VIR_STORAGE_VOL_OPEN_NOERROR); + char *path = target->path; + char *ploop_path = NULL;

if (lstat(path, sb) < 0) { - if (errno == ENOENT) { - if (noerror) { - VIR_WARN("ignoring missing file '%s'", path); - return -2; - } - virReportError(VIR_ERR_NO_STORAGE_VOL, - _("no storage vol with matching path '%s'"), - path); - return -1; - } - virReportSystemError(errno, - _("cannot stat file '%s'"), - path); - return -1; + FAILED_STAT(path, ret); + return ret; }

if (S_ISFIFO(sb->st_mode)) { @@ -1633,6 +1658,18 @@ virStorageBackendVolOpen(const char *path, struct stat *sb, virReportError(VIR_ERR_INTERNAL_ERROR, _("Volume path '%s' is a socket"), path); return -1; + } else if (S_ISDIR(sb->st_mode)) { + if (virStorageBackendIsPloopDir(path)) { + if (virAsprintf(&ploop_path, "%s/%s", target->path, "root.hds") < 0) + return -1; + path = ploop_path; + target->format = VIR_STORAGE_FILE_PLOOP; + if (lstat(path, sb) < 0) { + FAILED_STAT(path, ret); + VIR_FREE(ploop_path); + return ret; + } + } }

/* O_NONBLOCK should only matter during open() for fifos and @@ -1729,6 +1766,7 @@ virStorageBackendVolOpen(const char *path, struct stat *sb, return -1; }

+ VIR_FREE(ploop_path); return fd; }

I don't think this function is a good place to detect volume format. It is basically just a wrapper for virFileOpenAs and its function get an fd for a volume. Adding "root.hds" here would be enough. virStorageFileGetMetadataInternal?? looks like place that could detect ploops by directory structure.

@@ -1756,8 +1794,10 @@ virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, virStorageSourcePtr meta = NULL; char *buf = NULL; ssize_t len = VIR_STORAGE_MAX_HEADER; + char *path = NULL; + char *target_path = target->path;

- if ((ret = virStorageBackendVolOpen(target->path, &sb, openflags)) < 0) + if ((ret = virStorageBackendVolOpen(target, &sb, openflags)) < -1) goto cleanup; fd = ret;

@@ -1772,7 +1812,7 @@ virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, }

if (lseek(fd, 0, SEEK_SET) == (off_t)-1) { - virReportSystemError(errno, _("cannot seek to start of '%s'"), target->path); + virReportSystemError(errno, _("cannot seek to start of '%s'"), target_path); ret = -1; goto cleanup; } @@ -1780,18 +1820,23 @@ virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, if ((len = virFileReadHeaderFD(fd, len, &buf)) < 0) { if (readflags & VIR_STORAGE_VOL_READ_NOERROR) { VIR_WARN("ignoring failed header read for '%s'", - target->path); + target_path); ret = -2; } else { virReportSystemError(errno, _("cannot read header '%s'"), - target->path); + target_path); ret = -1; } goto cleanup; }

- if (!(meta = virStorageFileGetMetadataFromBuf(target->path, buf, len, target->format, + if (target->format == VIR_STORAGE_FILE_PLOOP) { + if (virAsprintf(&path, "%s/%s", target->path, "root.hds") < 0) + return -1; + target_path = path; + } + if (!(meta = virStorageFileGetMetadataFromBuf(target_path, buf, len, target->format, NULL))) { ret = -1; goto cleanup; @@ -1811,6 +1856,7 @@ virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, virStorageSourceFree(meta); VIR_FORCE_CLOSE(fd); VIR_FREE(buf); + VIR_FREE(path); return ret; }

diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 1de8dfe..7dca559 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -206,7 +206,7 @@ enum { # define VIR_STORAGE_VOL_OPEN_DEFAULT (VIR_STORAGE_VOL_OPEN_REG |\ VIR_STORAGE_VOL_OPEN_BLOCK)

-int virStorageBackendVolOpen(const char *path, struct stat *sb, +int virStorageBackendVolOpen(virStorageSourcePtr target, struct stat *sb, unsigned int flags) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index c2d148d..c340c69 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -75,7 +75,7 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, if (encryption) *encryption = NULL;

- if ((rc = virStorageBackendVolOpen(target->path, &sb, + if ((rc = virStorageBackendVolOpen(target, &sb, VIR_STORAGE_VOL_FS_PROBE_FLAGS)) < 0) return rc; /* Take care to propagate rc, it is not always -1 */ fd = rc; @@ -83,6 +83,10 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, if (virStorageBackendUpdateVolTargetInfoFD(target, fd, &sb) < 0) goto cleanup;

+ if (target->format == VIR_STORAGE_FILE_PLOOP) { + ret = 0; + goto cleanup; + } if (S_ISDIR(sb.st_mode)) { target->format = VIR_STORAGE_FILE_DIR; ret = 0; diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index ba26223..aaace5b 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -955,7 +955,7 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn, virCommandFree(cmd); cmd = NULL;

- if ((fd = virStorageBackendVolOpen(vol->target.path, &sb, + if ((fd = virStorageBackendVolOpen(&vol->target, &sb, VIR_STORAGE_VOL_OPEN_DEFAULT)) < 0) goto error;

diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 101070f..d92da30 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -184,6 +184,10 @@ qedGetBackingStore(char **, int *, const char *, size_t); #define QED_F_BACKING_FILE 0x01 #define QED_F_BACKING_FORMAT_NO_PROBE 0x04

+/* Location of ploop image size in the header file * + * https://openvz.org/Ploop/format */ +#define PLOOP_IMAGE_SIZE_OFFSET 36 +#define PLOOP_SIZE_MULTIPLIER 512

static struct FileTypeInfo const fileTypeInfo[] = { [VIR_STORAGE_FILE_NONE] = { 0, NULL, NULL, LV_LITTLE_ENDIAN, @@ -236,8 +240,8 @@ static struct FileTypeInfo const fileTypeInfo[] = { -1, {0}, 0, 0, 0, 0, NULL, NULL }, [VIR_STORAGE_FILE_VHD] = { 0, NULL, NULL, LV_LITTLE_ENDIAN, -1, {0}, 0, 0, 0, 0, NULL, NULL }, - [VIR_STORAGE_FILE_PLOOP] = { 0, NULL, NULL, LV_LITTLE_ENDIAN, - -1, {0}, 0, 0, 0, 0, NULL, NULL }, + [VIR_STORAGE_FILE_PLOOP] = { 0, "ploop", NULL, LV_LITTLE_ENDIAN, + -1, {0}, PLOOP_IMAGE_SIZE_OFFSET, 0, PLOOP_SIZE_MULTIPLIER, 0, NULL, NULL },

this is not true, we have not "ploop" magic.

/* All formats with a backing store probe below here */ [VIR_STORAGE_FILE_COW] = {