[libvirt] Add virStorageVolResize()

From: "Zeeshan Ali (Khattak)" <zeeshanak@gnome.org> Add a new function to allow changing of capacity of storage volumes. --- include/libvirt/libvirt.h.in | 5 ++ src/driver.h | 5 ++ src/libvirt.c | 50 +++++++++++++++++++++++ src/libvirt_public.syms | 1 + src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 9 ++++- src/remote_protocol-structs | 6 +++ src/storage/storage_backend.h | 6 +++ src/storage/storage_backend_fs.c | 59 ++++++++++++++++++++++++++++ src/storage/storage_driver.c | 80 ++++++++++++++++++++++++++++++++++++++ src/util/storage_file.c | 16 ++++++++ src/util/storage_file.h | 2 + tools/virsh.c | 53 +++++++++++++++++++++++++ 13 files changed, 292 insertions(+), 1 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index e99cd00..b169592 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2386,6 +2386,11 @@ char * virStorageVolGetXMLDesc (virStorageVolPtr pool, char * virStorageVolGetPath (virStorageVolPtr vol); +int virStorageVolResize (virStorageVolPtr vol, + unsigned long long capacity, + unsigned int flags); + + /** * virKeycodeSet: * diff --git a/src/driver.h b/src/driver.h index df2aa60..c850926 100644 --- a/src/driver.h +++ b/src/driver.h @@ -1261,6 +1261,10 @@ typedef int unsigned long long offset, unsigned long long length, unsigned int flags); +typedef int + (*virDrvStorageVolResize) (virStorageVolPtr vol, + unsigned long long capacity, + unsigned int flags); typedef int (*virDrvStoragePoolIsActive)(virStoragePoolPtr pool); @@ -1323,6 +1327,7 @@ struct _virStorageDriver { virDrvStorageVolGetInfo volGetInfo; virDrvStorageVolGetXMLDesc volGetXMLDesc; virDrvStorageVolGetPath volGetPath; + virDrvStorageVolResize volResize; virDrvStoragePoolIsActive poolIsActive; virDrvStoragePoolIsPersistent poolIsPersistent; }; diff --git a/src/libvirt.c b/src/libvirt.c index e9d638b..d1962a2 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -12927,6 +12927,56 @@ error: return NULL; } +/** + * virStorageVolResize: + * @vol: pointer to storage volume + * @capacity: new capacity + * @flags: extra flags; not used yet, so callers should always pass 0 + * + * Changes the capacity of the storage volume @vol to @capacity. The new + * capacity must not exceed the sum of current capacity of the volume and + * remainining free space of its parent pool. Also the new capacity must + * be greater than or equal to current allocation of the volume. + * + * Returns 0 on success, or -1 on error. + */ +int +virStorageVolResize(virStorageVolPtr vol, + unsigned long long capacity, + unsigned int flags) +{ + virConnectPtr conn; + VIR_DEBUG("vol=%p capacity=%llu flags=%x", vol, capacity, flags); + + virResetLastError(); + + if (!VIR_IS_STORAGE_VOL(vol)) { + virLibStorageVolError(VIR_ERR_INVALID_STORAGE_VOL, __FUNCTION__); + virDispatchError(NULL); + return -1; + } + + conn = vol->conn; + + if (conn->flags & VIR_CONNECT_RO) { + virLibConnError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); + goto error; + } + + if (conn->storageDriver && conn->storageDriver->volResize) { + int ret; + ret = conn->storageDriver->volResize(vol, capacity, flags); + if (ret < 0) + goto error; + return ret; + } + + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + virDispatchError(vol->conn); + return -1; +} /** * virNodeNumOfDevices: diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 1340b0c..54a4f2c 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -520,6 +520,7 @@ LIBVIRT_0.9.10 { global: virDomainShutdownFlags; virStorageVolWipePattern; + virStorageVolResize; } LIBVIRT_0.9.9; # .... define new API here using predicted next version number .... diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index f79f53e..2bb4cbf 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -4837,6 +4837,7 @@ static virStorageDriver storage_driver = { .volGetInfo = remoteStorageVolGetInfo, /* 0.4.1 */ .volGetXMLDesc = remoteStorageVolGetXMLDesc, /* 0.4.1 */ .volGetPath = remoteStorageVolGetPath, /* 0.4.1 */ + .volResize = remoteStorageVolResize, /* 0.9.10 */ .poolIsActive = remoteStoragePoolIsActive, /* 0.7.3 */ .poolIsPersistent = remoteStoragePoolIsPersistent, /* 0.7.3 */ }; diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 0f354bb..29f98fc 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -1676,6 +1676,12 @@ struct remote_storage_vol_get_path_ret { remote_nonnull_string name; }; +struct remote_storage_vol_resize_args { + remote_nonnull_storage_vol vol; + unsigned hyper capacity; + unsigned int flags; +}; + /* Node driver calls: */ struct remote_node_num_of_devices_args { @@ -2667,7 +2673,8 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_SET_INTERFACE_PARAMETERS = 256, /* autogen autogen */ REMOTE_PROC_DOMAIN_GET_INTERFACE_PARAMETERS = 257, /* skipgen skipgen */ REMOTE_PROC_DOMAIN_SHUTDOWN_FLAGS = 258, /* autogen autogen */ - REMOTE_PROC_STORAGE_VOL_WIPE_PATTERN = 259 /* autogen autogen */ + REMOTE_PROC_STORAGE_VOL_WIPE_PATTERN = 259, /* autogen autogen */ + REMOTE_PROC_STORAGE_VOL_RESIZE = 300 /* autogen autogen */ /* * Notice how the entries are grouped in sets of 10 ? diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index de85862..9a60fc2 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -1260,6 +1260,11 @@ struct remote_storage_vol_get_path_args { struct remote_storage_vol_get_path_ret { remote_nonnull_string name; }; +struct remote_storage_vol_resize_args { + remote_nonnull_storage_vol vol; + uint64_t capacity; + u_int flags; +}; struct remote_node_num_of_devices_args { remote_string cap; u_int flags; @@ -2101,4 +2106,5 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_GET_INTERFACE_PARAMETERS = 257, REMOTE_PROC_DOMAIN_SHUTDOWN_FLAGS = 258, REMOTE_PROC_STORAGE_VOL_WIPE_PATTERN = 259, + REMOTE_PROC_STORAGE_VOL_RESIZE = 300, }; diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 75ed676..a37bf7c 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -44,6 +44,11 @@ typedef int (*virStorageBackendDeleteVol)(virConnectPtr conn, virStoragePoolObjP typedef int (*virStorageBackendBuildVolFrom)(virConnectPtr conn, virStoragePoolObjPtr pool, virStorageVolDefPtr origvol, virStorageVolDefPtr newvol, unsigned int flags); +typedef int (*virStorageBackendVolumeResize)(virConnectPtr conn, + virStoragePoolObjPtr pool, + virStorageVolDefPtr vol, + unsigned long long capacity, + unsigned int flags); /* File creation/cloning functions used for cloning between backends */ int virStorageBackendCreateRaw(virConnectPtr conn, @@ -78,6 +83,7 @@ struct _virStorageBackend { virStorageBackendCreateVol createVol; virStorageBackendRefreshVol refreshVol; virStorageBackendDeleteVol deleteVol; + virStorageBackendVolumeResize resizeVol; }; virStorageBackendPtr virStorageBackendForType(int type); diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index d8dc29c..31f7c0a 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -1187,6 +1187,62 @@ virStorageBackendFileSystemVolRefresh(virConnectPtr conn, return 0; } +static int +virStorageBackendFilesystemResizeQemuImg(const char *path, + unsigned long long capacity) +{ + int ret = -1; + char *img_tool; + virCommandPtr cmd = NULL; + + /* KVM is usually ahead of qemu on features, so try that first */ + img_tool = virFindFileInPath("kvm-img"); + if (!img_tool) + img_tool = virFindFileInPath("qemu-img"); + + if (!img_tool) { + virStorageReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("unable to find kvm-img or qemu-img")); + return -1; + } + + cmd = virCommandNew(img_tool); + virCommandAddArgList(cmd, "resize", path, NULL); + virCommandAddArgFormat(cmd, "%llu", capacity); + + ret = virCommandRun(cmd, NULL); + + VIR_FREE(img_tool); + virCommandFree(cmd); + + return ret; +} + +/** + * Resize a volume + */ +static int +virStorageBackendFileSystemVolResize(virConnectPtr conn ATTRIBUTE_UNUSED, + virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, + virStorageVolDefPtr vol, + unsigned long long capacity, + unsigned int flags) +{ + virCheckFlags(0, -1); + + if (vol->target.format == VIR_STORAGE_FILE_RAW) { + return virStorageFileResize(vol->target.path, capacity); + } else if (vol->target.format == VIR_STORAGE_FILE_DIR) { + virStorageReportError(VIR_ERR_NO_SUPPORT, + "%s", _("Changing size of directory based " + "volumes is not supported")); + return -1; + } else { + return virStorageBackendFilesystemResizeQemuImg(vol->target.path, + capacity); + } +} + virStorageBackend virStorageBackendDirectory = { .type = VIR_STORAGE_POOL_DIR, @@ -1199,6 +1255,7 @@ virStorageBackend virStorageBackendDirectory = { .createVol = virStorageBackendFileSystemVolCreate, .refreshVol = virStorageBackendFileSystemVolRefresh, .deleteVol = virStorageBackendFileSystemVolDelete, + .resizeVol = virStorageBackendFileSystemVolResize, }; #if WITH_STORAGE_FS @@ -1216,6 +1273,7 @@ virStorageBackend virStorageBackendFileSystem = { .createVol = virStorageBackendFileSystemVolCreate, .refreshVol = virStorageBackendFileSystemVolRefresh, .deleteVol = virStorageBackendFileSystemVolDelete, + .resizeVol = virStorageBackendFileSystemVolResize, }; virStorageBackend virStorageBackendNetFileSystem = { .type = VIR_STORAGE_POOL_NETFS, @@ -1232,5 +1290,6 @@ virStorageBackend virStorageBackendNetFileSystem = { .createVol = virStorageBackendFileSystemVolCreate, .refreshVol = virStorageBackendFileSystemVolRefresh, .deleteVol = virStorageBackendFileSystemVolDelete, + .resizeVol = virStorageBackendFileSystemVolResize, }; #endif /* WITH_STORAGE_FS */ diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index a332ada..76730b4 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1695,7 +1695,86 @@ out: return ret; } +static int +storageVolumeResize(virStorageVolPtr obj, + unsigned long long capacity, + unsigned int flags) +{ + virStorageDriverStatePtr driver = obj->conn->storagePrivateData; + virStorageBackendPtr backend; + virStoragePoolObjPtr pool = NULL; + virStorageVolDefPtr vol = NULL; + int ret = -1; + + virCheckFlags(0, -1); + + storageDriverLock(driver); + pool = virStoragePoolObjFindByName(&driver->pools, obj->pool); + storageDriverUnlock(driver); + + if (!pool) { + virStorageReportError(VIR_ERR_NO_STORAGE_POOL, + _("no storage pool with matching uuid")); + goto out; + } + + if (!virStoragePoolObjIsActive(pool)) { + virStorageReportError(VIR_ERR_OPERATION_INVALID, + _("storage pool is not active")); + goto out; + } + + if ((backend = virStorageBackendForType(pool->def->type)) == NULL) + goto out; + + vol = virStorageVolDefFindByName(pool, obj->name); + + if (vol == NULL) { + virStorageReportError(VIR_ERR_NO_STORAGE_VOL, + _("no storage vol with matching name '%s'"), + obj->name); + goto out; + } + + if (vol->building) { + virStorageReportError(VIR_ERR_OPERATION_INVALID, + _("volume '%s' is still being allocated."), + vol->name); + goto out; + } + + if (capacity < vol->allocation) { + virStorageReportError(VIR_ERR_INVALID_ARG, + _("can't shrink capacity below " + "existing allocation")); + goto out; + } + + if (capacity > vol->allocation + pool->def->available) { + virStorageReportError(VIR_ERR_INVALID_ARG, + _("Not enough space left on storage pool")); + goto out; + } + + if (!backend->resizeVol) { + virStorageReportError(VIR_ERR_NO_SUPPORT, + _("storage pool does not support changing of " + "volume capacity")); + goto out; + } + + if (backend->resizeVol(obj->conn, pool, vol, capacity, flags) < 0) + goto out; + + vol->capacity = capacity; + ret = 0; + +out: + if (pool) + virStoragePoolObjUnlock(pool); + return ret; +} /* If the volume we're wiping is already a sparse file, we simply * truncate and extend it to its original size, filling it with @@ -2243,6 +2322,7 @@ static virStorageDriver storageDriver = { .volGetInfo = storageVolumeGetInfo, /* 0.4.0 */ .volGetXMLDesc = storageVolumeGetXMLDesc, /* 0.4.0 */ .volGetPath = storageVolumeGetPath, /* 0.4.0 */ + .volResize = storageVolumeResize, /* 0.9.10 */ .poolIsActive = storagePoolIsActive, /* 0.7.3 */ .poolIsPersistent = storagePoolIsPersistent, /* 0.7.3 */ diff --git a/src/util/storage_file.c b/src/util/storage_file.c index ba9cfc5..8260adb 100644 --- a/src/util/storage_file.c +++ b/src/util/storage_file.c @@ -931,6 +931,22 @@ virStorageFileFreeMetadata(virStorageFileMetadata *meta) VIR_FREE(meta); } +/** + * virStorageFileResize: + * + * Change the capacity of the raw storage file at 'path'. + */ +int +virStorageFileResize(const char *path, unsigned long long capacity) +{ + if (truncate(path, capacity) < 0) { + virReportSystemError(errno, _("Failed to truncate file '%s'"), path); + return -1; + } + + return 0; +} + #ifdef __linux__ # ifndef NFS_SUPER_MAGIC diff --git a/src/util/storage_file.h b/src/util/storage_file.h index b8920d0..96afb12 100644 --- a/src/util/storage_file.h +++ b/src/util/storage_file.h @@ -72,6 +72,8 @@ int virStorageFileGetMetadataFromFD(const char *path, void virStorageFileFreeMetadata(virStorageFileMetadata *meta); +int virStorageFileResize(const char *path, unsigned long long capacity); + enum { VIR_STORAGE_FILE_SHFS_NFS = (1 << 0), VIR_STORAGE_FILE_SHFS_GFS2 = (1 << 1), diff --git a/tools/virsh.c b/tools/virsh.c index 74655c2..20d4bd0 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -11279,6 +11279,58 @@ cmdVolInfo(vshControl *ctl, const vshCmd *cmd) return ret; } +/* + * "vol-resize" command + */ +static const vshCmdInfo info_vol_resize[] = { + {"help", N_("resize a vol")}, + {"desc", N_("Resizes a storage vol.")}, + {NULL, NULL} +}; + +static const vshCmdOptDef opts_vol_resize[] = { + {"vol", VSH_OT_DATA, VSH_OFLAG_REQ, N_("vol name, key or path")}, + {"capacity", VSH_OT_DATA, VSH_OFLAG_REQ, N_("new capacity for the vol with optional k,M,G,T suffix")}, + {"pool", VSH_OT_STRING, 0, N_("pool name or uuid")}, + {NULL, 0, 0, NULL} +}; + +static bool +cmdVolResize(vshControl *ctl, const vshCmd *cmd) +{ + virStorageVolPtr vol; + const char *capacityStr = NULL; + unsigned long long capacity = 0; + bool ret = true; + + if (!vshConnectionUsability(ctl, ctl->conn)) + return false; + + if (!(vol = vshCommandOptVol(ctl, cmd, "vol", "pool", NULL))) + return false; + + if (vshCommandOptString(cmd, "capacity", &capacityStr) <= 0) + goto cleanup; + if (cmdVolSize(capacityStr, &capacity) < 0) { + vshError(ctl, _("Malformed size %s"), capacityStr); + goto cleanup; + } + + if (virStorageVolResize(vol, capacity, 0) == 0) { + vshPrint(ctl, "Size of volume '%s' successfully changed to %s\n", + virStorageVolGetName(vol), capacityStr); + ret = true; + } else { + vshError(ctl, "Failed to change size of volume '%s' to %s\n", + virStorageVolGetName(vol), capacityStr); + ret = false; + } + +cleanup: + virStorageVolFree(vol); + return ret; +} + /* * "vol-dumpxml" command @@ -16141,6 +16193,7 @@ static const vshCmdDef storageVolCmds[] = { {"vol-pool", cmdVolPool, opts_vol_pool, info_vol_pool, 0}, {"vol-upload", cmdVolUpload, opts_vol_upload, info_vol_upload, 0}, {"vol-wipe", cmdVolWipe, opts_vol_wipe, info_vol_wipe, 0}, + {"vol-resize", cmdVolResize, opts_vol_resize, info_vol_resize, 0}, {NULL, NULL, NULL, NULL, 0} }; -- 1.7.7.5

On 01/27/2012 04:40 PM, Zeeshan Ali (Khattak) wrote:
From: "Zeeshan Ali (Khattak)" <zeeshanak@gnome.org>
Add a new function to allow changing of capacity of storage volumes. --- include/libvirt/libvirt.h.in | 5 ++ src/driver.h | 5 ++ src/libvirt.c | 50 +++++++++++++++++++++++ src/libvirt_public.syms | 1 + src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 9 ++++- src/remote_protocol-structs | 6 +++ src/storage/storage_backend.h | 6 +++ src/storage/storage_backend_fs.c | 59 ++++++++++++++++++++++++++++ src/storage/storage_driver.c | 80 ++++++++++++++++++++++++++++++++++++++ src/util/storage_file.c | 16 ++++++++ src/util/storage_file.h | 2 + tools/virsh.c | 53 +++++++++++++++++++++++++ 13 files changed, 292 insertions(+), 1 deletions(-)
Part 2 of my review - the RPC implementation.
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index f79f53e..2bb4cbf 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -4837,6 +4837,7 @@ static virStorageDriver storage_driver = { .volGetInfo = remoteStorageVolGetInfo, /* 0.4.1 */ .volGetXMLDesc = remoteStorageVolGetXMLDesc, /* 0.4.1 */ .volGetPath = remoteStorageVolGetPath, /* 0.4.1 */ + .volResize = remoteStorageVolResize, /* 0.9.10 */ .poolIsActive = remoteStoragePoolIsActive, /* 0.7.3 */ .poolIsPersistent = remoteStoragePoolIsPersistent, /* 0.7.3 */ }; diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 0f354bb..29f98fc 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -1676,6 +1676,12 @@ struct remote_storage_vol_get_path_ret { remote_nonnull_string name; };
+struct remote_storage_vol_resize_args { + remote_nonnull_storage_vol vol; + unsigned hyper capacity;
Based on my proposed API changes, this must be signed.
+ unsigned int flags; +}; + /* Node driver calls: */
struct remote_node_num_of_devices_args { @@ -2667,7 +2673,8 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_SET_INTERFACE_PARAMETERS = 256, /* autogen autogen */ REMOTE_PROC_DOMAIN_GET_INTERFACE_PARAMETERS = 257, /* skipgen skipgen */ REMOTE_PROC_DOMAIN_SHUTDOWN_FLAGS = 258, /* autogen autogen */ - REMOTE_PROC_STORAGE_VOL_WIPE_PATTERN = 259 /* autogen autogen */ + REMOTE_PROC_STORAGE_VOL_WIPE_PATTERN = 259, /* autogen autogen */ + REMOTE_PROC_STORAGE_VOL_RESIZE = 300 /* autogen autogen */
Skipping is not allowed :) 260 is just fine.
/* * Notice how the entries are grouped in sets of 10 ? diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index de85862..9a60fc2 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -1260,6 +1260,11 @@ struct remote_storage_vol_get_path_args { struct remote_storage_vol_get_path_ret { remote_nonnull_string name; }; +struct remote_storage_vol_resize_args { + remote_nonnull_storage_vol vol; + uint64_t capacity; + u_int flags;
And regenerating this picks up a tweak.
+}; struct remote_node_num_of_devices_args { remote_string cap; u_int flags; @@ -2101,4 +2106,5 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_GET_INTERFACE_PARAMETERS = 257, REMOTE_PROC_DOMAIN_SHUTDOWN_FLAGS = 258, REMOTE_PROC_STORAGE_VOL_WIPE_PATTERN = 259, + REMOTE_PROC_STORAGE_VOL_RESIZE = 300, };
The RPC stuff is pretty easy when it gets autogenerated :) diff --git i/src/remote/remote_protocol.x w/src/remote/remote_protocol.x index 29f98fc..7d104b2 100644 --- i/src/remote/remote_protocol.x +++ w/src/remote/remote_protocol.x @@ -1678,7 +1678,7 @@ struct remote_storage_vol_get_path_ret { struct remote_storage_vol_resize_args { remote_nonnull_storage_vol vol; - unsigned hyper capacity; + hyper capacity; unsigned int flags; }; @@ -2674,7 +2674,7 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_GET_INTERFACE_PARAMETERS = 257, /* skipgen skipgen */ REMOTE_PROC_DOMAIN_SHUTDOWN_FLAGS = 258, /* autogen autogen */ REMOTE_PROC_STORAGE_VOL_WIPE_PATTERN = 259, /* autogen autogen */ - REMOTE_PROC_STORAGE_VOL_RESIZE = 300 /* autogen autogen */ + REMOTE_PROC_STORAGE_VOL_RESIZE = 260 /* autogen autogen */ /* * Notice how the entries are grouped in sets of 10 ? diff --git i/src/remote_protocol-structs w/src/remote_protocol-structs index de85862..70a69f6 100644 --- i/src/remote_protocol-structs +++ w/src/remote_protocol-structs @@ -1260,6 +1260,11 @@ struct remote_storage_vol_get_path_args { struct remote_storage_vol_get_path_ret { remote_nonnull_string name; }; +struct remote_storage_vol_resize_args { + remote_nonnull_storage_vol vol; + int64_t capacity; + u_int flags; +}; struct remote_node_num_of_devices_args { remote_string cap; u_int flags; @@ -2101,4 +2106,5 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_GET_INTERFACE_PARAMETERS = 257, REMOTE_PROC_DOMAIN_SHUTDOWN_FLAGS = 258, REMOTE_PROC_STORAGE_VOL_WIPE_PATTERN = 259, + REMOTE_PROC_STORAGE_VOL_RESIZE = 260, }; -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On Sat, Jan 28, 2012 at 2:27 AM, Eric Blake <eblake@redhat.com> wrote:
On 01/27/2012 04:40 PM, Zeeshan Ali (Khattak) wrote:
From: "Zeeshan Ali (Khattak)" <zeeshanak@gnome.org>
Add a new function to allow changing of capacity of storage volumes. Part 2 of my review - the RPC implementation.
Looks good.
@@ -2667,7 +2673,8 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_SET_INTERFACE_PARAMETERS = 256, /* autogen autogen */ REMOTE_PROC_DOMAIN_GET_INTERFACE_PARAMETERS = 257, /* skipgen skipgen */ REMOTE_PROC_DOMAIN_SHUTDOWN_FLAGS = 258, /* autogen autogen */ - REMOTE_PROC_STORAGE_VOL_WIPE_PATTERN = 259 /* autogen autogen */ + REMOTE_PROC_STORAGE_VOL_WIPE_PATTERN = 259, /* autogen autogen */ + REMOTE_PROC_STORAGE_VOL_RESIZE = 300 /* autogen autogen */
Skipping is not allowed :) 260 is just fine.
What? 259 + 1 != 300? :) I guess its time I learn to count beyond 259. :) -- Regards, Zeeshan Ali (Khattak) FSF member#5124

From: "Zeeshan Ali (Khattak)" <zeeshanak@gnome.org> Add a new function to allow changing of capacity of storage volumes. Plan out several flags, even if not all of them will be implemented up front. Expose the new command via 'virsh vol-resize'. Signed-off-by: Eric Blake <eblake@redhat.com> --- include/libvirt/libvirt.h.in | 11 ++++++ python/generator.py | 1 + src/driver.h | 5 +++ src/libvirt.c | 76 ++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 1 + tools/virsh.c | 82 ++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 11 ++++++ 7 files changed, 187 insertions(+), 0 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index e99cd00..d26cbbd 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2386,6 +2386,17 @@ char * virStorageVolGetXMLDesc (virStorageVolPtr pool, char * virStorageVolGetPath (virStorageVolPtr vol); +typedef enum { + VIR_STORAGE_VOL_RESIZE_ALLOCATE = 1 << 0, /* force allocation of new size */ + VIR_STORAGE_VOL_RESIZE_DELTA = 1 << 1, /* size is relative to current */ + VIR_STORAGE_VOL_RESIZE_SHRINK = 1 << 2, /* allow decrease in capacity */ +} virStorageVolResizeFlags; + +int virStorageVolResize (virStorageVolPtr vol, + long long capacity, + unsigned int flags); + + /** * virKeycodeSet: * diff --git a/python/generator.py b/python/generator.py index de635dc..791b267 100755 --- a/python/generator.py +++ b/python/generator.py @@ -259,6 +259,7 @@ py_types = { 'double': ('d', None, "double", "double"), 'unsigned int': ('i', None, "int", "int"), 'unsigned long': ('l', None, "long", "long"), + 'long long': ('l', None, "longlong", "long long"), 'unsigned long long': ('l', None, "longlong", "long long"), 'unsigned char *': ('z', None, "charPtr", "char *"), 'char *': ('z', None, "charPtr", "char *"), diff --git a/src/driver.h b/src/driver.h index df2aa60..485b578 100644 --- a/src/driver.h +++ b/src/driver.h @@ -1261,6 +1261,10 @@ typedef int unsigned long long offset, unsigned long long length, unsigned int flags); +typedef int + (*virDrvStorageVolResize) (virStorageVolPtr vol, + long long capacity, + unsigned int flags); typedef int (*virDrvStoragePoolIsActive)(virStoragePoolPtr pool); @@ -1323,6 +1327,7 @@ struct _virStorageDriver { virDrvStorageVolGetInfo volGetInfo; virDrvStorageVolGetXMLDesc volGetXMLDesc; virDrvStorageVolGetPath volGetPath; + virDrvStorageVolResize volResize; virDrvStoragePoolIsActive poolIsActive; virDrvStoragePoolIsPersistent poolIsPersistent; }; diff --git a/src/libvirt.c b/src/libvirt.c index e9d638b..540d74a 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -12927,6 +12927,82 @@ error: return NULL; } +/** + * virStorageVolResize: + * @vol: pointer to storage volume + * @capacity: new capacity, in bytes + * @flags: bitwise-OR of virStorageVolResizeFlags + * + * Changes the capacity of the storage volume @vol to @capacity. The + * operation will fail if the new capacity requires allocation that would + * exceed the remaining free space in the parent pool. The contents of + * the new capacity will appear as all zero bytes. + * + * Normally, the operation will attempt to affect capacity with a minimum + * impact on allocation (that is, the default operation favors a sparse + * resize). If @flags contains VIR_STORAGE_VOL_RESIZE_ALLOCATE, then the + * operation will ensure that allocation is sufficient for the new + * capacity; this may make the operation take noticeably longer. + * + * Normally, the operation treats @capacity as the new size in bytes; + * but if @flags contains VIR_STORAGE_RESIZE_DELTA, then @capacity + * represents the size difference to add to the current size. It is + * up to the storage pool implementation whether unaligned requests are + * rounded up to the next valid boundary, or rejected. + * + * Normally, this operation should only be used to enlarge capacity; + * but if @flags contains VIR_STORAGE_RESIZE_SHRINK, it is possible to + * attempt a reduction in capacity even though it might cause data loss. + * + * Returns 0 on success, or -1 on error. + */ +int +virStorageVolResize(virStorageVolPtr vol, + long long capacity, + unsigned int flags) +{ + virConnectPtr conn; + VIR_DEBUG("vol=%p capacity=%lld flags=%x", vol, capacity, flags); + + virResetLastError(); + + if (!VIR_IS_STORAGE_VOL(vol)) { + virLibStorageVolError(VIR_ERR_INVALID_STORAGE_VOL, __FUNCTION__); + virDispatchError(NULL); + return -1; + } + + conn = vol->conn; + + if (conn->flags & VIR_CONNECT_RO) { + virLibConnError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); + goto error; + } + + /* Negative capacity is valid only with both delta and shrink; + * zero capacity is valid with either delta or shrink. */ + if ((capacity < 0 && !(flags & VIR_STORAGE_VOL_RESIZE_DELTA) && + !(flags & VIR_STORAGE_VOL_RESIZE_SHRINK)) || + (capacity == 0 && !((flags & VIR_STORAGE_VOL_RESIZE_DELTA) || + (flags & VIR_STORAGE_VOL_RESIZE_SHRINK)))) { + virLibStorageVolError(VIR_ERR_INVALID_ARG, __FUNCTION__); + goto error; + } + + if (conn->storageDriver && conn->storageDriver->volResize) { + int ret; + ret = conn->storageDriver->volResize(vol, capacity, flags); + if (ret < 0) + goto error; + return ret; + } + + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + virDispatchError(vol->conn); + return -1; +} /** * virNodeNumOfDevices: diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 1340b0c..8bdb24b 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -519,6 +519,7 @@ LIBVIRT_0.9.9 { LIBVIRT_0.9.10 { global: virDomainShutdownFlags; + virStorageVolResize; virStorageVolWipePattern; } LIBVIRT_0.9.9; diff --git a/tools/virsh.c b/tools/virsh.c index 9b37dc0..ffcd746 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -11279,6 +11279,87 @@ cmdVolInfo(vshControl *ctl, const vshCmd *cmd) return ret; } +/* + * "vol-resize" command + */ +static const vshCmdInfo info_vol_resize[] = { + {"help", N_("resize a vol")}, + {"desc", N_("Resizes a storage volume.")}, + {NULL, NULL} +}; + +static const vshCmdOptDef opts_vol_resize[] = { + {"vol", VSH_OT_DATA, VSH_OFLAG_REQ, N_("vol name, key or path")}, + {"capacity", VSH_OT_DATA, VSH_OFLAG_REQ, + N_("new capacity for the vol with optional k,M,G,T suffix")}, + {"pool", VSH_OT_STRING, 0, N_("pool name or uuid")}, + {"allocate", VSH_OT_BOOL, 0, + N_("allocate the new capacity, rather than leaving it sparse")}, + {"delta", VSH_OT_BOOL, 0, + N_("use capacity as a delta to current size, rather than the new size")}, + {"shrink", VSH_OT_BOOL, 0, N_("allow the resize to shrink the volume")}, + {NULL, 0, 0, NULL} +}; + +static bool +cmdVolResize(vshControl *ctl, const vshCmd *cmd) +{ + virStorageVolPtr vol; + const char *capacityStr = NULL; + unsigned long long capacity = 0; + unsigned int flags = 0; + bool ret = false; + bool delta = false; + + if (vshCommandOptBool(cmd, "allocate")) + flags |= VIR_STORAGE_VOL_RESIZE_ALLOCATE; + if (vshCommandOptBool(cmd, "delta")) { + delta = true; + flags |= VIR_STORAGE_VOL_RESIZE_DELTA; + } + if (vshCommandOptBool(cmd, "shrink")) + flags |= VIR_STORAGE_VOL_RESIZE_SHRINK; + + if (!vshConnectionUsability(ctl, ctl->conn)) + return false; + + if (!(vol = vshCommandOptVol(ctl, cmd, "vol", "pool", NULL))) + return false; + + if (vshCommandOptString(cmd, "capacity", &capacityStr) <= 0) + goto cleanup; + if (delta && *capacityStr == '-') { + if (cmdVolSize(capacityStr + 1, &capacity) < 0) { + vshError(ctl, _("Malformed size %s"), capacityStr); + goto cleanup; + } + capacity = -capacity; + } else { + if (cmdVolSize(capacityStr, &capacity) < 0) { + vshError(ctl, _("Malformed size %s"), capacityStr); + goto cleanup; + } + } + + if (virStorageVolResize(vol, capacity, flags) == 0) { + vshPrint(ctl, + delta ? _("Size of volume '%s' successfully changed by %s\n") + : _("Size of volume '%s' successfully changed to %s\n"), + virStorageVolGetName(vol), capacityStr); + ret = true; + } else { + vshError(ctl, + delta ? _("Failed to change size of volume '%s' by %s\n") + : _("Failed to change size of volume '%s' to %s\n"), + virStorageVolGetName(vol), capacityStr); + ret = false; + } + +cleanup: + virStorageVolFree(vol); + return ret; +} + /* * "vol-dumpxml" command @@ -16139,6 +16220,7 @@ static const vshCmdDef storageVolCmds[] = { {"vol-name", cmdVolName, opts_vol_name, info_vol_name, 0}, {"vol-path", cmdVolPath, opts_vol_path, info_vol_path, 0}, {"vol-pool", cmdVolPool, opts_vol_pool, info_vol_pool, 0}, + {"vol-resize", cmdVolResize, opts_vol_resize, info_vol_resize, 0}, {"vol-upload", cmdVolUpload, opts_vol_upload, info_vol_upload, 0}, {"vol-wipe", cmdVolWipe, opts_vol_wipe, info_vol_wipe, 0}, {NULL, NULL, NULL, NULL, 0} diff --git a/tools/virsh.pod b/tools/virsh.pod index 8599f66..6622caf 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2012,6 +2012,17 @@ I<--pool> I<pool-or-uuid> is the name or UUID of the storage pool the volume is in. I<vol-name-or-path> is the name or path of the volume to return the volume key for. +=item B<vol-resize> [I<--pool> I<pool-or-uuid>] I<vol-name-or-path> +I<pool-or-uuid> I<capacity> [I<--allocate>] [I<--delta>] [I<--shrink>] + +Resize the capacity of the given volume, in bytes. +I<--pool> I<pool-or-uuid> is the name or UUID of the storage pool the volume +is in. I<vol-name-or-key-or-path> is the name or key or path of the volume +to resize. The new capacity might be sparse unless I<--allocate> is +specified. Normally, I<capacity> is the new size, but if I<--delta> +is present, then it is added to the existing size. Attempts to shrink +the volume will fail unless I<--shrink> is present. + =back =head1 SECRET COMMMANDS -- 1.7.7.6

On Sat, Jan 28, 2012 at 2:28 AM, Eric Blake <eblake@redhat.com> wrote:
From: "Zeeshan Ali (Khattak)" <zeeshanak@gnome.org>
Add a new function to allow changing of capacity of storage volumes. Plan out several flags, even if not all of them will be implemented up front.
Expose the new command via 'virsh vol-resize'.
ACK otherwise but i don't see how VIR_STORAGE_VOL_RESIZE_ALLOCATE can be implemented and also the inability of qemu-img to shrink images I mentioned in my previous mail. -- Regards, Zeeshan Ali (Khattak) FSF member#5124

On 01/27/2012 06:14 PM, Zeeshan Ali (Khattak) wrote:
On Sat, Jan 28, 2012 at 2:28 AM, Eric Blake <eblake@redhat.com> wrote:
From: "Zeeshan Ali (Khattak)" <zeeshanak@gnome.org>
Add a new function to allow changing of capacity of storage volumes. Plan out several flags, even if not all of them will be implemented up front.
Expose the new command via 'virsh vol-resize'.
ACK otherwise but i don't see how VIR_STORAGE_VOL_RESIZE_ALLOCATE can be implemented and also the inability of qemu-img to shrink images I mentioned in my previous mail.
We implement what makes sense, and reject what doesn't; and it's okay if we have a partial implementation (such as raw files work in all aspects, LVM partitions treat VIR_STORAGE_VOL_RESIZE_ALLOCATE as a no-op since that is what happens automatically, and qcow2 supports enlarging but not shrinking). And perhaps when someone encounters a situation that they need to work, they can help contribute patches - the important thing to note is that we can backport patches that implement the features without breaking API, but we can't backport the new API without bumping the .so version, so the goal is to get an API that we can agree to in the source now, but then we have a bit more breathing room on getting the implementation working. At any rate, I've gone ahead and pushed these two patches, so you can rebase your implementation patches on top of this and see what flags you can implement and for which file types. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On Fri, Jan 27, 2012 at 05:28:15PM -0700, Eric Blake wrote:
From: "Zeeshan Ali (Khattak)" <zeeshanak@gnome.org>
Add a new function to allow changing of capacity of storage volumes. Plan out several flags, even if not all of them will be implemented up front.
Expose the new command via 'virsh vol-resize'.
Signed-off-by: Eric Blake <eblake@redhat.com> --- include/libvirt/libvirt.h.in | 11 ++++++ python/generator.py | 1 + src/driver.h | 5 +++ src/libvirt.c | 76 ++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 1 + tools/virsh.c | 82 ++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 11 ++++++ 7 files changed, 187 insertions(+), 0 deletions(-)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index e99cd00..d26cbbd 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2386,6 +2386,17 @@ char * virStorageVolGetXMLDesc (virStorageVolPtr pool,
char * virStorageVolGetPath (virStorageVolPtr vol);
+typedef enum { + VIR_STORAGE_VOL_RESIZE_ALLOCATE = 1 << 0, /* force allocation of new size */ + VIR_STORAGE_VOL_RESIZE_DELTA = 1 << 1, /* size is relative to current */ + VIR_STORAGE_VOL_RESIZE_SHRINK = 1 << 2, /* allow decrease in capacity */ +} virStorageVolResizeFlags; + +int virStorageVolResize (virStorageVolPtr vol, + long long capacity, + unsigned int flags);
Why has this changed from 'unsigned long long' to just 'long long'. In virStorageVolInfo we use 'unsigned long long', and you can't ever have a negative capacity, so I don't see why this should be signed. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 01/30/2012 04:08 AM, Daniel P. Berrange wrote:
On Fri, Jan 27, 2012 at 05:28:15PM -0700, Eric Blake wrote:
From: "Zeeshan Ali (Khattak)" <zeeshanak@gnome.org>
Add a new function to allow changing of capacity of storage volumes. Plan out several flags, even if not all of them will be implemented up front.
+typedef enum { + VIR_STORAGE_VOL_RESIZE_ALLOCATE = 1 << 0, /* force allocation of new size */ + VIR_STORAGE_VOL_RESIZE_DELTA = 1 << 1, /* size is relative to current */ + VIR_STORAGE_VOL_RESIZE_SHRINK = 1 << 2, /* allow decrease in capacity */ +} virStorageVolResizeFlags; + +int virStorageVolResize (virStorageVolPtr vol, + long long capacity, + unsigned int flags);
Why has this changed from 'unsigned long long' to just 'long long'.
Because of VIR_STORAGE_VOL_RESIZE_DELTA and VIR_STORAGE_VOL_RESIZE_SHRINK. That is, virStorageVolResize(vol, -10 * 1024 * 1024, DELTA|SHRINK) is a valid call to shave off 10 MiB of data.
In virStorageVolInfo we use 'unsigned long long', and you can't ever have a negative capacity, so I don't see why this should be signed.
Remember, off_t is signed, so you can never have a storage volume larger than 2**63 bytes anyways. Using signed capacity is no different than using signed off_t (and if you can show me someone with a storage volume with 2**64 bytes, I will be rather envious). -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On Mon, Jan 30, 2012 at 07:18:06AM -0700, Eric Blake wrote:
On 01/30/2012 04:08 AM, Daniel P. Berrange wrote:
On Fri, Jan 27, 2012 at 05:28:15PM -0700, Eric Blake wrote:
From: "Zeeshan Ali (Khattak)" <zeeshanak@gnome.org>
Add a new function to allow changing of capacity of storage volumes. Plan out several flags, even if not all of them will be implemented up front.
+typedef enum { + VIR_STORAGE_VOL_RESIZE_ALLOCATE = 1 << 0, /* force allocation of new size */ + VIR_STORAGE_VOL_RESIZE_DELTA = 1 << 1, /* size is relative to current */ + VIR_STORAGE_VOL_RESIZE_SHRINK = 1 << 2, /* allow decrease in capacity */ +} virStorageVolResizeFlags; + +int virStorageVolResize (virStorageVolPtr vol, + long long capacity, + unsigned int flags);
Why has this changed from 'unsigned long long' to just 'long long'.
Because of VIR_STORAGE_VOL_RESIZE_DELTA and VIR_STORAGE_VOL_RESIZE_SHRINK. That is,
virStorageVolResize(vol, -10 * 1024 * 1024, DELTA|SHRINK)
is a valid call to shave off 10 MiB of data.
Isn't that rather redundant. Either you need a negative size, or you need a SHRINK flag. If you have a shrink flag, then we don't need a signed int. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On Mon, Jan 30, 2012 at 02:25:19PM +0000, Daniel P. Berrange wrote:
On Mon, Jan 30, 2012 at 07:18:06AM -0700, Eric Blake wrote:
On 01/30/2012 04:08 AM, Daniel P. Berrange wrote:
On Fri, Jan 27, 2012 at 05:28:15PM -0700, Eric Blake wrote:
From: "Zeeshan Ali (Khattak)" <zeeshanak@gnome.org>
Add a new function to allow changing of capacity of storage volumes. Plan out several flags, even if not all of them will be implemented up front.
+typedef enum { + VIR_STORAGE_VOL_RESIZE_ALLOCATE = 1 << 0, /* force allocation of new size */ + VIR_STORAGE_VOL_RESIZE_DELTA = 1 << 1, /* size is relative to current */ + VIR_STORAGE_VOL_RESIZE_SHRINK = 1 << 2, /* allow decrease in capacity */ +} virStorageVolResizeFlags; + +int virStorageVolResize (virStorageVolPtr vol, + long long capacity, + unsigned int flags);
Why has this changed from 'unsigned long long' to just 'long long'.
Because of VIR_STORAGE_VOL_RESIZE_DELTA and VIR_STORAGE_VOL_RESIZE_SHRINK. That is,
virStorageVolResize(vol, -10 * 1024 * 1024, DELTA|SHRINK)
is a valid call to shave off 10 MiB of data.
Isn't that rather redundant. Either you need a negative size, or you need a SHRINK flag. If you have a shrink flag, then we don't need a signed int.
In fact since our existing virDomainBlockResize API is already using unsigned long long, I'd say we should do shrinkage solely based off the SHRINK flag, and *not* require a negative size as well Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 01/30/2012 07:28 AM, Daniel P. Berrange wrote:
Why has this changed from 'unsigned long long' to just 'long long'.
Because of VIR_STORAGE_VOL_RESIZE_DELTA and VIR_STORAGE_VOL_RESIZE_SHRINK. That is,
virStorageVolResize(vol, -10 * 1024 * 1024, DELTA|SHRINK)
is a valid call to shave off 10 MiB of data.
Isn't that rather redundant. Either you need a negative size, or you need a SHRINK flag. If you have a shrink flag, then we don't need a signed int.
In fact since our existing virDomainBlockResize API is already using unsigned long long, I'd say we should do shrinkage solely based off the SHRINK flag, and *not* require a negative size as well
Here's what I was envisioning: set my size to an absolute of 10M, regardless of whether it was previously 5M or 15M: virStorageVolResize(vol, 10*1024*1024, SHRINK) set my size to an absolute of 10M, but only if it does not shrink: virStorageVolResize(vol, 10*1024*1024, 0) set my size to a relative of 10M larger virStorageVolResize(vol, 10*1024*1024, DELTA) set my size to a relative of 10M smaller, provided it was at least 10M to begin with: virStorageVolResize(vol, -10*1024*1024, DELTA|SHRINK) You are proposing that the negative sign should be implied by the combination of DELTA|SHRINK; I guess I can live with that, since the other three use cases still work as-is, and DELTA|SHRINK is the only point where a negative value makes sense (and therefore where implying the negative is okay). Shall I go ahead and write the patch? -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On Mon, Jan 30, 2012 at 08:23:36AM -0700, Eric Blake wrote:
On 01/30/2012 07:28 AM, Daniel P. Berrange wrote:
Why has this changed from 'unsigned long long' to just 'long long'.
Because of VIR_STORAGE_VOL_RESIZE_DELTA and VIR_STORAGE_VOL_RESIZE_SHRINK. That is,
virStorageVolResize(vol, -10 * 1024 * 1024, DELTA|SHRINK)
is a valid call to shave off 10 MiB of data.
Isn't that rather redundant. Either you need a negative size, or you need a SHRINK flag. If you have a shrink flag, then we don't need a signed int.
In fact since our existing virDomainBlockResize API is already using unsigned long long, I'd say we should do shrinkage solely based off the SHRINK flag, and *not* require a negative size as well
Here's what I was envisioning:
set my size to an absolute of 10M, regardless of whether it was previously 5M or 15M:
virStorageVolResize(vol, 10*1024*1024, SHRINK)
set my size to an absolute of 10M, but only if it does not shrink:
virStorageVolResize(vol, 10*1024*1024, 0)
set my size to a relative of 10M larger
virStorageVolResize(vol, 10*1024*1024, DELTA)
set my size to a relative of 10M smaller, provided it was at least 10M to begin with:
virStorageVolResize(vol, -10*1024*1024, DELTA|SHRINK)
You are proposing that the negative sign should be implied by the combination of DELTA|SHRINK; I guess I can live with that, since the other three use cases still work as-is, and DELTA|SHRINK is the only point where a negative value makes sense (and therefore where implying the negative is okay).
Shall I go ahead and write the patch?
Yep, that would be good. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On Mon, Jan 30, 2012 at 5:27 PM, Daniel P. Berrange <berrange@redhat.com> wrote:
On Mon, Jan 30, 2012 at 08:23:36AM -0700, Eric Blake wrote:
On 01/30/2012 07:28 AM, Daniel P. Berrange wrote:
Why has this changed from 'unsigned long long' to just 'long long'.
Because of VIR_STORAGE_VOL_RESIZE_DELTA and VIR_STORAGE_VOL_RESIZE_SHRINK. That is,
virStorageVolResize(vol, -10 * 1024 * 1024, DELTA|SHRINK)
is a valid call to shave off 10 MiB of data.
Isn't that rather redundant. Either you need a negative size, or you need a SHRINK flag. If you have a shrink flag, then we don't need a signed int.
In fact since our existing virDomainBlockResize API is already using unsigned long long, I'd say we should do shrinkage solely based off the SHRINK flag, and *not* require a negative size as well
Here's what I was envisioning:
set my size to an absolute of 10M, regardless of whether it was previously 5M or 15M:
virStorageVolResize(vol, 10*1024*1024, SHRINK)
set my size to an absolute of 10M, but only if it does not shrink:
virStorageVolResize(vol, 10*1024*1024, 0)
set my size to a relative of 10M larger
virStorageVolResize(vol, 10*1024*1024, DELTA)
set my size to a relative of 10M smaller, provided it was at least 10M to begin with:
virStorageVolResize(vol, -10*1024*1024, DELTA|SHRINK)
You are proposing that the negative sign should be implied by the combination of DELTA|SHRINK; I guess I can live with that, since the other three use cases still work as-is, and DELTA|SHRINK is the only point where a negative value makes sense (and therefore where implying the negative is okay).
Shall I go ahead and write the patch?
Yep, that would be good.
FWIW +1 from my side too. -- Regards, Zeeshan Ali (Khattak) FSF member#5124

On 2012年01月28日 08:28, Eric Blake wrote:
From: "Zeeshan Ali (Khattak)"<zeeshanak@gnome.org>
Add a new function to allow changing of capacity of storage volumes. Plan out several flags, even if not all of them will be implemented up front.
Expose the new command via 'virsh vol-resize'.
Signed-off-by: Eric Blake<eblake@redhat.com> --- include/libvirt/libvirt.h.in | 11 ++++++ python/generator.py | 1 + src/driver.h | 5 +++ src/libvirt.c | 76 ++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 1 + tools/virsh.c | 82 ++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 11 ++++++ 7 files changed, 187 insertions(+), 0 deletions(-)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index e99cd00..d26cbbd 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2386,6 +2386,17 @@ char * virStorageVolGetXMLDesc (virStorageVolPtr pool,
char * virStorageVolGetPath (virStorageVolPtr vol);
+typedef enum { + VIR_STORAGE_VOL_RESIZE_ALLOCATE = 1<< 0, /* force allocation of new size */ + VIR_STORAGE_VOL_RESIZE_DELTA = 1<< 1, /* size is relative to current */ + VIR_STORAGE_VOL_RESIZE_SHRINK = 1<< 2, /* allow decrease in capacity */ +} virStorageVolResizeFlags; + +int virStorageVolResize (virStorageVolPtr vol, + long long capacity, + unsigned int flags); + + /** * virKeycodeSet: * diff --git a/python/generator.py b/python/generator.py index de635dc..791b267 100755 --- a/python/generator.py +++ b/python/generator.py @@ -259,6 +259,7 @@ py_types = { 'double': ('d', None, "double", "double"), 'unsigned int': ('i', None, "int", "int"), 'unsigned long': ('l', None, "long", "long"), + 'long long': ('l', None, "longlong", "long long"), 'unsigned long long': ('l', None, "longlong", "long long"), 'unsigned char *': ('z', None, "charPtr", "char *"), 'char *': ('z', None, "charPtr", "char *"), diff --git a/src/driver.h b/src/driver.h index df2aa60..485b578 100644 --- a/src/driver.h +++ b/src/driver.h @@ -1261,6 +1261,10 @@ typedef int unsigned long long offset, unsigned long long length, unsigned int flags); +typedef int + (*virDrvStorageVolResize) (virStorageVolPtr vol, + long long capacity, + unsigned int flags);
typedef int (*virDrvStoragePoolIsActive)(virStoragePoolPtr pool); @@ -1323,6 +1327,7 @@ struct _virStorageDriver { virDrvStorageVolGetInfo volGetInfo; virDrvStorageVolGetXMLDesc volGetXMLDesc; virDrvStorageVolGetPath volGetPath; + virDrvStorageVolResize volResize; virDrvStoragePoolIsActive poolIsActive; virDrvStoragePoolIsPersistent poolIsPersistent; }; diff --git a/src/libvirt.c b/src/libvirt.c index e9d638b..540d74a 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -12927,6 +12927,82 @@ error: return NULL; }
+/** + * virStorageVolResize: + * @vol: pointer to storage volume + * @capacity: new capacity, in bytes + * @flags: bitwise-OR of virStorageVolResizeFlags + * + * Changes the capacity of the storage volume @vol to @capacity. The + * operation will fail if the new capacity requires allocation that would + * exceed the remaining free space in the parent pool. The contents of + * the new capacity will appear as all zero bytes. + * + * Normally, the operation will attempt to affect capacity with a minimum + * impact on allocation (that is, the default operation favors a sparse + * resize). If @flags contains VIR_STORAGE_VOL_RESIZE_ALLOCATE, then the + * operation will ensure that allocation is sufficient for the new + * capacity; this may make the operation take noticeably longer. + * + * Normally, the operation treats @capacity as the new size in bytes; + * but if @flags contains VIR_STORAGE_RESIZE_DELTA, then @capacity + * represents the size difference to add to the current size. It is + * up to the storage pool implementation whether unaligned requests are + * rounded up to the next valid boundary, or rejected. + * + * Normally, this operation should only be used to enlarge capacity; + * but if @flags contains VIR_STORAGE_RESIZE_SHRINK, it is possible to + * attempt a reduction in capacity even though it might cause data loss. + * + * Returns 0 on success, or -1 on error. + */ +int +virStorageVolResize(virStorageVolPtr vol, + long long capacity, + unsigned int flags) +{ + virConnectPtr conn; + VIR_DEBUG("vol=%p capacity=%lld flags=%x", vol, capacity, flags); + + virResetLastError(); + + if (!VIR_IS_STORAGE_VOL(vol)) { + virLibStorageVolError(VIR_ERR_INVALID_STORAGE_VOL, __FUNCTION__); + virDispatchError(NULL); + return -1; + } + + conn = vol->conn; + + if (conn->flags& VIR_CONNECT_RO) { + virLibConnError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); + goto error; + } + + /* Negative capacity is valid only with both delta and shrink; + * zero capacity is valid with either delta or shrink. */ + if ((capacity< 0&& !(flags& VIR_STORAGE_VOL_RESIZE_DELTA)&& + !(flags& VIR_STORAGE_VOL_RESIZE_SHRINK)) || + (capacity == 0&& !((flags& VIR_STORAGE_VOL_RESIZE_DELTA) || + (flags& VIR_STORAGE_VOL_RESIZE_SHRINK)))) { + virLibStorageVolError(VIR_ERR_INVALID_ARG, __FUNCTION__); + goto error; + } + + if (conn->storageDriver&& conn->storageDriver->volResize) { + int ret; + ret = conn->storageDriver->volResize(vol, capacity, flags); + if (ret< 0) + goto error; + return ret; + } + + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + virDispatchError(vol->conn); + return -1; +}
/** * virNodeNumOfDevices: diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 1340b0c..8bdb24b 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -519,6 +519,7 @@ LIBVIRT_0.9.9 { LIBVIRT_0.9.10 { global: virDomainShutdownFlags; + virStorageVolResize; virStorageVolWipePattern; } LIBVIRT_0.9.9;
diff --git a/tools/virsh.c b/tools/virsh.c index 9b37dc0..ffcd746 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -11279,6 +11279,87 @@ cmdVolInfo(vshControl *ctl, const vshCmd *cmd) return ret; }
+/* + * "vol-resize" command + */ +static const vshCmdInfo info_vol_resize[] = { + {"help", N_("resize a vol")}, + {"desc", N_("Resizes a storage volume.")}, + {NULL, NULL} +}; + +static const vshCmdOptDef opts_vol_resize[] = { + {"vol", VSH_OT_DATA, VSH_OFLAG_REQ, N_("vol name, key or path")}, + {"capacity", VSH_OT_DATA, VSH_OFLAG_REQ, + N_("new capacity for the vol with optional k,M,G,T suffix")}, + {"pool", VSH_OT_STRING, 0, N_("pool name or uuid")}, + {"allocate", VSH_OT_BOOL, 0, + N_("allocate the new capacity, rather than leaving it sparse")}, + {"delta", VSH_OT_BOOL, 0, + N_("use capacity as a delta to current size, rather than the new size")}, + {"shrink", VSH_OT_BOOL, 0, N_("allow the resize to shrink the volume")}, + {NULL, 0, 0, NULL} +}; + +static bool +cmdVolResize(vshControl *ctl, const vshCmd *cmd) +{ + virStorageVolPtr vol; + const char *capacityStr = NULL; + unsigned long long capacity = 0; + unsigned int flags = 0; + bool ret = false; + bool delta = false; + + if (vshCommandOptBool(cmd, "allocate")) + flags |= VIR_STORAGE_VOL_RESIZE_ALLOCATE; + if (vshCommandOptBool(cmd, "delta")) { + delta = true; + flags |= VIR_STORAGE_VOL_RESIZE_DELTA; + } + if (vshCommandOptBool(cmd, "shrink")) + flags |= VIR_STORAGE_VOL_RESIZE_SHRINK; + + if (!vshConnectionUsability(ctl, ctl->conn)) + return false;
Late response, but it seems not good idea to expose the options which are actually not implemented underlying yet. One would get error like: # virsh vol-resize /var/lib/libvirt/images/test.img 60 --shrink error: invalid argument: storageVolumeResize: unsupported flags (0x4) With the magic number "0x4", one'd never known what it is. It can be improved roundabout, but it should be avoided in the beginning. Regards, Osier

On 06/06/2012 06:12 AM, Osier Yang wrote:
On 2012年01月28日 08:28, Eric Blake wrote:
From: "Zeeshan Ali (Khattak)"<zeeshanak@gnome.org>
Add a new function to allow changing of capacity of storage volumes. Plan out several flags, even if not all of them will be implemented up front.
Late response, but it seems not good idea to expose the options which are actually not implemented underlying yet. One would get error like:
# virsh vol-resize /var/lib/libvirt/images/test.img 60 --shrink error: invalid argument: storageVolumeResize: unsupported flags (0x4)
With the magic number "0x4", one'd never known what it is. It can be improved roundabout, but it should be avoided in the beginning.
Improving the error message would indeed be nice, if you can find a way to do it across the board for all APIs that call virCheckFlags(). But your general objection to avoiding flag values that are not implemented across all drivers is impossible - as we add new APIs and new flags to existing APIs, we are stuck with the fact that not all hypervisors can implement all those flags simultaneously (if even at all). I see no problem with defining a libvirt API with flags that some drivers cannot implement yet, only with the quality of an error message when a flag is unsupported. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 2012年06月06日 20:58, Eric Blake wrote:
On 06/06/2012 06:12 AM, Osier Yang wrote:
On 2012年01月28日 08:28, Eric Blake wrote:
From: "Zeeshan Ali (Khattak)"<zeeshanak@gnome.org>
Add a new function to allow changing of capacity of storage volumes. Plan out several flags, even if not all of them will be implemented up front.
Late response, but it seems not good idea to expose the options which are actually not implemented underlying yet. One would get error like:
# virsh vol-resize /var/lib/libvirt/images/test.img 60 --shrink error: invalid argument: storageVolumeResize: unsupported flags (0x4)
With the magic number "0x4", one'd never known what it is. It can be improved roundabout, but it should be avoided in the beginning.
Improving the error message would indeed be nice, if you can find a way to do it across the board for all APIs that call virCheckFlags(). But your general objection to avoiding flag values that are not implemented across all drivers is impossible - as we add new APIs and new flags to existing APIs, we are stuck with the fact that not all hypervisors can implement all those flags simultaneously (if even at all). I see no problem with defining a libvirt API with flags that some drivers cannot implement yet,
Sure, it's fine for an API to have a flag not implemented underlying. what I'm objecting is not to expose the options in virsh commands if none of the drivers support them yet. But yeah, all the problems are not problem if we could find a way to improve the error message with some work on virCheckFlags. I don't have a good idea in mind yet, though. Converting the magic number (flag) to string doesn't make sense as the user won't known the mapping between them, and it smells like overkilling. Regards, Osier

From: "Zeeshan Ali (Khattak)" <zeeshanak@gnome.org> Autogeneration saves the day. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 9 ++++++++- src/remote_protocol-structs | 6 ++++++ 3 files changed, 15 insertions(+), 1 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index f79f53e..2bb4cbf 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -4837,6 +4837,7 @@ static virStorageDriver storage_driver = { .volGetInfo = remoteStorageVolGetInfo, /* 0.4.1 */ .volGetXMLDesc = remoteStorageVolGetXMLDesc, /* 0.4.1 */ .volGetPath = remoteStorageVolGetPath, /* 0.4.1 */ + .volResize = remoteStorageVolResize, /* 0.9.10 */ .poolIsActive = remoteStoragePoolIsActive, /* 0.7.3 */ .poolIsPersistent = remoteStoragePoolIsPersistent, /* 0.7.3 */ }; diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 0f354bb..7d104b2 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -1676,6 +1676,12 @@ struct remote_storage_vol_get_path_ret { remote_nonnull_string name; }; +struct remote_storage_vol_resize_args { + remote_nonnull_storage_vol vol; + hyper capacity; + unsigned int flags; +}; + /* Node driver calls: */ struct remote_node_num_of_devices_args { @@ -2667,7 +2673,8 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_SET_INTERFACE_PARAMETERS = 256, /* autogen autogen */ REMOTE_PROC_DOMAIN_GET_INTERFACE_PARAMETERS = 257, /* skipgen skipgen */ REMOTE_PROC_DOMAIN_SHUTDOWN_FLAGS = 258, /* autogen autogen */ - REMOTE_PROC_STORAGE_VOL_WIPE_PATTERN = 259 /* autogen autogen */ + REMOTE_PROC_STORAGE_VOL_WIPE_PATTERN = 259, /* autogen autogen */ + REMOTE_PROC_STORAGE_VOL_RESIZE = 260 /* autogen autogen */ /* * Notice how the entries are grouped in sets of 10 ? diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index de85862..70a69f6 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -1260,6 +1260,11 @@ struct remote_storage_vol_get_path_args { struct remote_storage_vol_get_path_ret { remote_nonnull_string name; }; +struct remote_storage_vol_resize_args { + remote_nonnull_storage_vol vol; + int64_t capacity; + u_int flags; +}; struct remote_node_num_of_devices_args { remote_string cap; u_int flags; @@ -2101,4 +2106,5 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_GET_INTERFACE_PARAMETERS = 257, REMOTE_PROC_DOMAIN_SHUTDOWN_FLAGS = 258, REMOTE_PROC_STORAGE_VOL_WIPE_PATTERN = 259, + REMOTE_PROC_STORAGE_VOL_RESIZE = 260, }; -- 1.7.7.6

On Sat, Jan 28, 2012 at 2:28 AM, Eric Blake <eblake@redhat.com> wrote:
From: "Zeeshan Ali (Khattak)" <zeeshanak@gnome.org>
Autogeneration saves the day.
ACK! -- Regards, Zeeshan Ali (Khattak) FSF member#5124

On 01/27/2012 04:40 PM, Zeeshan Ali (Khattak) wrote:
From: "Zeeshan Ali (Khattak)" <zeeshanak@gnome.org>
Add a new function to allow changing of capacity of storage volumes.
I know you still have to rebase, but here's some more review comments:
diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 75ed676..a37bf7c 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -44,6 +44,11 @@ typedef int (*virStorageBackendDeleteVol)(virConnectPtr conn, virStoragePoolObjP typedef int (*virStorageBackendBuildVolFrom)(virConnectPtr conn, virStoragePoolObjPtr pool, virStorageVolDefPtr origvol, virStorageVolDefPtr newvol, unsigned int flags); +typedef int (*virStorageBackendVolumeResize)(virConnectPtr conn, + virStoragePoolObjPtr pool, + virStorageVolDefPtr vol, + unsigned long long capacity,
You'll want to change this to long long.
+ unsigned int flags);
+static int +virStorageBackendFilesystemResizeQemuImg(const char *path, + unsigned long long capacity) +{ + int ret = -1; + char *img_tool; + virCommandPtr cmd = NULL; + + /* KVM is usually ahead of qemu on features, so try that first */ + img_tool = virFindFileInPath("kvm-img"); + if (!img_tool) + img_tool = virFindFileInPath("qemu-img");
I'm surprised we're not caching this in some helper function. Oh well, that's an independent cleanup.
+ + cmd = virCommandNew(img_tool); + virCommandAddArgList(cmd, "resize", path, NULL); + virCommandAddArgFormat(cmd, "%llu", capacity);
Guess what - qemu-img can do delta resizing, as well as shrinking. In fact, qemu-img can even do (sparse-only) resizing of raw files! But for raw files, I think we are better off doing it ourselves (as your patch already did), as it means fewer forks and more control over whether the result is sparse. qemu-img resize foo 10M # reset size to 10M, regardless of whether that is bigger or smaller - which means you have to check the capacity yourself before calling qemu-img with a smaller capacity qemu-img resize foo +10M # add 10M to capacity qemu-img resize foo -10M # remove 10M from capacity That all means that you have to be careful of the incoming flags, and control whether you output a sign in the command line argument.
+/** + * Resize a volume + */ +static int +virStorageBackendFileSystemVolResize(virConnectPtr conn ATTRIBUTE_UNUSED, + virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, + virStorageVolDefPtr vol, + unsigned long long capacity, + unsigned int flags) +{ + virCheckFlags(0, -1); + + if (vol->target.format == VIR_STORAGE_FILE_RAW) { + return virStorageFileResize(vol->target.path, capacity);
That signature loses the notion of whether you are doing a delta or an absolute change. Either you need to convert delta into absolute before forwarding lower down the chain, or you need to add a bool parameter stating whether a delta change is desired.
+ } else if (vol->target.format == VIR_STORAGE_FILE_DIR) { + virStorageReportError(VIR_ERR_NO_SUPPORT, + "%s", _("Changing size of directory based " + "volumes is not supported")); + return -1;
@@ -1199,6 +1255,7 @@ virStorageBackend virStorageBackendDirectory = { .createVol = virStorageBackendFileSystemVolCreate, .refreshVol = virStorageBackendFileSystemVolRefresh, .deleteVol = virStorageBackendFileSystemVolDelete, + .resizeVol = virStorageBackendFileSystemVolResize,
I'd leave this one out. That way, you don't have to handle VIR_STORAGE_FILE_DIR in virStorageBackendFileSystemVolResize in the first place.
+static int +storageVolumeResize(virStorageVolPtr obj, + unsigned long long capacity, + unsigned int flags) +{ + virStorageDriverStatePtr driver = obj->conn->storagePrivateData; + virStorageBackendPtr backend; + virStoragePoolObjPtr pool = NULL; + virStorageVolDefPtr vol = NULL; + int ret = -1; + + virCheckFlags(0, -1);
Make sure you permit the flags you plan on supporting in at least one backend (and it's okay if you don't support them all up front).
+++ b/src/util/storage_file.c @@ -931,6 +931,22 @@ virStorageFileFreeMetadata(virStorageFileMetadata *meta) VIR_FREE(meta); }
+/** + * virStorageFileResize: + * + * Change the capacity of the raw storage file at 'path'. + */ +int +virStorageFileResize(const char *path, unsigned long long capacity) +{ + if (truncate(path, capacity) < 0) { + virReportSystemError(errno, _("Failed to truncate file '%s'"), path); + return -1; + }
Here, if the allocate flag is specified, you'd want two implementations: If posix_fallocate exists, then open() the file, and use posix_fallocate to ensure that the delta from the old size to the new size is allocated. If it does not exist, then you need a while loop that writes a '\0' byte every 4096 bytes or so, in order to force the capacity increase to be non-sparse. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (4)
-
Daniel P. Berrange
-
Eric Blake
-
Osier Yang
-
Zeeshan Ali (Khattak)