[libvirt] [PATCH 6/6] storage:dir: adapts .refreshVol .refreshPool for ploop volumes
Nikolay Shirokovskiy
nshirokovskiy at virtuozzo.com
Thu Feb 18 09:29:38 UTC 2016
On 17.02.2016 14:40, 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 at virtuozzo.com>
> ---
> src/storage/storage_backend.c | 92 ++++++++++++++++++++++++++---------
> src/storage/storage_backend.h | 2 +-
> src/storage/storage_backend_fs.c | 6 ++-
> src/storage/storage_backend_logical.c | 2 +-
> src/util/virstoragefile.c | 9 +++-
> 5 files changed, 84 insertions(+), 27 deletions(-)
>
> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
> index 60e6bd0..0e3c99c 100644
> --- a/src/storage/storage_backend.c
> +++ b/src/storage/storage_backend.c
> @@ -1587,6 +1587,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;
let's stick with "%s/root.hds" in all places.
> + 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
> @@ -1595,29 +1614,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;\
> +}
There is no need to introduce macros here. Function is enought.
> +
> 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)) {
> @@ -1636,6 +1661,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
> @@ -1732,6 +1769,7 @@ virStorageBackendVolOpen(const char *path, struct stat *sb,
> return -1;
> }
>
> + VIR_FREE(ploop_path);
we don't have cleanup label in this function and a lot of returns above
this freeing thus we have memory leaks. I think instead of changing
all returns in this function we could leave it as it is and pass good
path here from outside. To check is this a ploop or not we
can just call virFileExist for hds file (i mean not using stat
call as i'm not sure is it heavier than access or not and current
code seems to optimize stat calls).
> return fd;
> }
>
> @@ -1759,8 +1797,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;
>
> @@ -1775,7 +1815,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;
> }
> @@ -1783,18 +1823,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))) {
do we need path to actual hds here? and all the changes above from target->path to target_path
> ret = -1;
> goto cleanup;
> @@ -1814,6 +1859,7 @@ virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target,
> virStorageSourceFree(meta);
> VIR_FORCE_CLOSE(fd);
> VIR_FREE(buf);
> + VIR_FREE(path);
> return ret;
> }
>
> @@ -2076,6 +2122,8 @@ virStorageBackendVolUploadLocal(virConnectPtr conn ATTRIBUTE_UNUSED,
> ret = virFDStreamOpenBlockDevice(stream, target_path,
> offset, len, O_WRONLY);
> if (ret < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("error "
> + "occured during the upload of ploop volume"));
must be in correspondent patch
> goto cleanup;
> }
>
> diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h
> index 65e91dc..de48012 100644
> --- a/src/storage/storage_backend.h
> +++ b/src/storage/storage_backend.h
> @@ -203,7 +203,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 abb6e47..f978b6e 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;
> + }
looks like if we return here we don't update capacity properly.
> 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..5184310 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,9 @@ 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, "WithoutFreSpaceExt", NULL, LV_LITTLE_ENDIAN,
> + -1, {0}, PLOOP_IMAGE_SIZE_OFFSET, 0,
> + PLOOP_SIZE_MULTIPLIER, 0, NULL, NULL },
>
> /* All formats with a backing store probe below here */
> [VIR_STORAGE_FILE_COW] = {
>
More information about the libvir-list
mailing list