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