[libvirt] [PATCH 0/3] Get the voldef target physical value
This effort started primarily to address some ideas/thoughts brought up in https://bugzilla.redhat.com/show_bug.cgi?id=1332019 although as things were updated, it seems that using storage volume definitions wouldn't be necessary since the same data can be obtained via the virDomainGetBlockInfo API. Still since code had been written - I figured I'd posted it and see what kind of feedback it got. The first patch just adds the <physical> element to the output XML and documents it thusly. The 2nd/3rd patch take a different approach adding virStorageVolInfoFlags which could take a single flag indicating that the caller would prefer to return the "physical" value instead of the "allocation" value. Yes, kind of a hack, but since we cannot extend _virStorageVolInfo to add a physical it's a mechanism to allow fetching the data using existing structures. Sure a virStorageVolStats API could be created as well, but I figured I'd see how this went first before thinking about that. John Ferlan (3): conf: Display <physical> in output of voldef storage: Introduce virStorageVolInfoFlags virsh: Allow display of the physical volume size daemon/remote.c | 38 +++++++++++++++++++++++++++++ docs/formatstorage.html.in | 5 ++++ include/libvirt/libvirt-storage.h | 11 +++++++++ src/conf/storage_conf.c | 6 +++++ src/driver-storage.h | 6 +++++ src/libvirt-storage.c | 51 +++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 5 ++++ src/remote/remote_driver.c | 37 ++++++++++++++++++++++++++++ src/remote/remote_protocol.x | 20 ++++++++++++++- src/remote_protocol-structs | 10 ++++++++ src/storage/storage_driver.c | 24 +++++++++++++++--- tools/virsh-volume.c | 37 +++++++++++++++++++++++++--- tools/virsh.pod | 8 ++++-- 13 files changed, 247 insertions(+), 11 deletions(-) -- 2.7.4
Although the virStorageBackendUpdateVolTargetInfo will update the target.physical value, there is no way to provide that information via the virStorageGetVolInfo API since it only returns the capacity and allocation of a volume. So as described in commit id '0282ca45', it should be possible to generate an output only <physical> value for that purpose. This patch generates the <physical> value in the volume XML output for the sole purpose of being able to view/see the value to allow someone to parse the XML in order to obtain the value. Update the documentation to describe the output only nature. Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatstorage.html.in | 5 +++++ src/conf/storage_conf.c | 6 ++++++ 2 files changed, 11 insertions(+) diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index 47c8b0c..a7273ed 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -550,6 +550,11 @@ specified with the same semantics as for <code>allocation</code> This is compulsory when creating a volume. <span class="since">Since 0.4.1</span></dd> + <dt><code>physical</code></dt> + <dd>This output only element provides the host physical size of + the target storage volume. The default output <code>unit</code> + will be in bytes. + <span class="since">Since 3.0.0</span></dd> <dt><code>source</code></dt> <dd>Provides information about the underlying storage allocation of the volume. This may not be available for some pool types. diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 7e7bb72..71ea0c9 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1669,6 +1669,12 @@ virStorageVolDefFormat(virStoragePoolDefPtr pool, def->target.capacity); virBufferAsprintf(&buf, "<allocation unit='bytes'>%llu</allocation>\n", def->target.allocation); + /* NB: Display only - since virStorageVolInfo is limited to just + * 'capacity' and 'allocation' on output. Since we don't read this + * in, be sure it was filled in before printing */ + if (def->target.physical) + virBufferAsprintf(&buf, "<physical unit='bytes'>%llu</physical>\n", + def->target.physical); if (virStorageVolTargetDefFormat(options, &buf, &def->target, "target") < 0) -- 2.7.4
https://bugzilla.redhat.com/show_bug.cgi?id=1332019 This function will essentially be a wrapper to virStorageVolInfo in order to provide a mechanism to have the "physical" size of the volume returned instead of the "allocation" size. This will provide similar capabilities to the virDomainBlockInfo which can return both allocation and physical of a domain storage volume. NB: Since we're reusing the _virStorageVolInfo and not creating a new _virStorageVolInfoFlags structure, we'll need to generate the rpc APIs remoteStorageVolGetInfoFlags and remoteDispatchStorageVolGetInfoFlags (although both were originally created from gendispatch.pl and then just copied into daemon/remote.c and src/remote/remote_driver.c). The new API will allow the usage of a VIR_STORAGE_VOL_GET_PHYSICAL flag and will make the decision to return the physical or allocation value into the allocation field. In order to get that physical value, virStorageBackendUpdateVolTargetInfoFD adds logic to fill in physical value matching logic in qemuStorageLimitsRefresh used by virDomainBlockInfo when the domain is inactive. Signed-off-by: John Ferlan <jferlan@redhat.com> --- daemon/remote.c | 38 +++++++++++++++++++++++++++++ include/libvirt/libvirt-storage.h | 11 +++++++++ src/driver-storage.h | 6 +++++ src/libvirt-storage.c | 51 +++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 5 ++++ src/remote/remote_driver.c | 37 ++++++++++++++++++++++++++++ src/remote/remote_protocol.x | 20 ++++++++++++++- src/remote_protocol-structs | 10 ++++++++ src/storage/storage_driver.c | 24 +++++++++++++++--- 9 files changed, 197 insertions(+), 5 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 46773da..23c9de4 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -6594,6 +6594,44 @@ remoteDispatchDomainInterfaceAddresses(virNetServerPtr server ATTRIBUTE_UNUSED, } +static int +remoteDispatchStorageVolGetInfoFlags(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client, + virNetMessagePtr msg ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr, + remote_storage_vol_get_info_flags_args *args, + remote_storage_vol_get_info_flags_ret *ret) +{ + int rv = -1; + virStorageVolPtr vol = NULL; + virStorageVolInfo tmp; + struct daemonClientPrivate *priv = + virNetServerClientGetPrivateData(client); + + if (!priv->conn) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open")); + goto cleanup; + } + + if (!(vol = get_nonnull_storage_vol(priv->conn, args->vol))) + goto cleanup; + + if (virStorageVolGetInfoFlags(vol, &tmp, args->flags) < 0) + goto cleanup; + + ret->type = tmp.type; + ret->capacity = tmp.capacity; + ret->allocation = tmp.allocation; + rv = 0; + + cleanup: + if (rv < 0) + virNetMessageSaveError(rerr); + virObjectUnref(vol); + return rv; +} + + /*----- Helpers. -----*/ /* get_nonnull_domain and get_nonnull_network turn an on-wire diff --git a/include/libvirt/libvirt-storage.h b/include/libvirt/libvirt-storage.h index 0974f6e..8a861e4 100644 --- a/include/libvirt/libvirt-storage.h +++ b/include/libvirt/libvirt-storage.h @@ -167,6 +167,14 @@ typedef enum { # endif } virStorageVolWipeAlgorithm; +typedef enum { + VIR_STORAGE_VOL_USE_ALLOCATION = 0, + + /* Return the physical size in allocation */ + VIR_STORAGE_VOL_GET_PHYSICAL = 1 << 0, + +} virStorageVolInfoFlags; + typedef struct _virStorageVolInfo virStorageVolInfo; struct _virStorageVolInfo { @@ -359,6 +367,9 @@ int virStorageVolFree (virStorageVolPtr vol); int virStorageVolGetInfo (virStorageVolPtr vol, virStorageVolInfoPtr info); +int virStorageVolGetInfoFlags (virStorageVolPtr vol, + virStorageVolInfoPtr info, + unsigned int flags); char * virStorageVolGetXMLDesc (virStorageVolPtr pool, unsigned int flags); diff --git a/src/driver-storage.h b/src/driver-storage.h index afcb12b..48e588a 100644 --- a/src/driver-storage.h +++ b/src/driver-storage.h @@ -158,6 +158,11 @@ typedef int (*virDrvStorageVolGetInfo)(virStorageVolPtr vol, virStorageVolInfoPtr info); +typedef int +(*virDrvStorageVolGetInfoFlags)(virStorageVolPtr vol, + virStorageVolInfoPtr info, + unsigned int flags); + typedef char * (*virDrvStorageVolGetXMLDesc)(virStorageVolPtr pool, unsigned int flags); @@ -257,6 +262,7 @@ struct _virStorageDriver { virDrvStorageVolWipe storageVolWipe; virDrvStorageVolWipePattern storageVolWipePattern; virDrvStorageVolGetInfo storageVolGetInfo; + virDrvStorageVolGetInfoFlags storageVolGetInfoFlags; virDrvStorageVolGetXMLDesc storageVolGetXMLDesc; virDrvStorageVolGetPath storageVolGetPath; virDrvStorageVolResize storageVolResize; diff --git a/src/libvirt-storage.c b/src/libvirt-storage.c index 48996ba..05eec8a 100644 --- a/src/libvirt-storage.c +++ b/src/libvirt-storage.c @@ -1914,6 +1914,57 @@ virStorageVolGetInfo(virStorageVolPtr vol, /** + * virStorageVolGetInfoFlags: + * @vol: pointer to storage volume + * @info: pointer at which to store info + * @flags: bitwise-OR of virStorageVolInfoFlags + * + * Fetches volatile information about the storage + * volume such as its current allocation. + * + * If the @flags argument is VIR_STORAGE_VOL_GET_PHYSICAL, then the physical + * bytes used for the volume will be returned in the @info allocation field. + * This is useful for sparse files and certain volume file types where the + * physical on disk usage can be different than the calculated allocation value + * as is the case with qcow2 files. + * + * Returns 0 on success, or -1 on failure + */ +int +virStorageVolGetInfoFlags(virStorageVolPtr vol, + virStorageVolInfoPtr info, + unsigned int flags) +{ + virConnectPtr conn; + VIR_DEBUG("vol=%p, info=%p, flags=%x", vol, info, flags); + + virResetLastError(); + + if (info) + memset(info, 0, sizeof(*info)); + + virCheckStorageVolReturn(vol, -1); + virCheckNonNullArgGoto(info, error); + + conn = vol->conn; + + if (conn->storageDriver->storageVolGetInfoFlags) { + int ret; + ret = conn->storageDriver->storageVolGetInfoFlags(vol, info, flags); + if (ret < 0) + goto error; + return ret; + } + + virReportUnsupportedError(); + + error: + virDispatchError(vol->conn); + return -1; +} + + +/** * virStorageVolGetXMLDesc: * @vol: pointer to storage volume * @flags: extra flags; not used yet, so callers should always pass 0 diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index e01604c..12ef085 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -746,4 +746,9 @@ LIBVIRT_2.2.0 { virConnectNodeDeviceEventDeregisterAny; } LIBVIRT_2.0.0; +LIBVIRT_3.0.0 { + global: + virStorageVolGetInfoFlags; +} LIBVIRT_2.2.0; + # .... define new API here using predicted next version number .... diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 8880520..46da06f 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -7842,6 +7842,42 @@ remoteDomainRename(virDomainPtr dom, const char *new_name, unsigned int flags) } +static int +remoteStorageVolGetInfoFlags(virStorageVolPtr vol, + virStorageVolInfoPtr result, + unsigned int flags) +{ + int rv = -1; + struct private_data *priv = vol->conn->privateData; + remote_storage_vol_get_info_flags_args args; + remote_storage_vol_get_info_flags_ret ret; + + remoteDriverLock(priv); + + make_nonnull_storage_vol(&args.vol, vol); + args.flags = flags; + + memset(&ret, 0, sizeof(ret)); + + if (call(vol->conn, priv, 0, REMOTE_PROC_STORAGE_VOL_GET_INFO_FLAGS, + (xdrproc_t)xdr_remote_storage_vol_get_info_flags_args, + (char *)&args, + (xdrproc_t)xdr_remote_storage_vol_get_info_flags_ret, + (char *)&ret) == -1) { + goto done; + } + + result->type = ret.type; + result->capacity = ret.capacity; + result->allocation = ret.allocation; + rv = 0; + + done: + remoteDriverUnlock(priv); + return rv; +} + + /* get_nonnull_domain and get_nonnull_network turn an on-wire * (name, uuid) pair into virDomainPtr or virNetworkPtr object. * These can return NULL if underlying memory allocations fail, @@ -8290,6 +8326,7 @@ static virStorageDriver storage_driver = { .storageVolWipe = remoteStorageVolWipe, /* 0.8.0 */ .storageVolWipePattern = remoteStorageVolWipePattern, /* 0.9.10 */ .storageVolGetInfo = remoteStorageVolGetInfo, /* 0.4.1 */ + .storageVolGetInfoFlags = remoteStorageVolGetInfoFlags, /* 3.0.0 */ .storageVolGetXMLDesc = remoteStorageVolGetXMLDesc, /* 0.4.1 */ .storageVolGetPath = remoteStorageVolGetPath, /* 0.4.1 */ .storageVolResize = remoteStorageVolResize, /* 0.9.10 */ diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index e8382dc..b846ef2 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -1941,6 +1941,17 @@ struct remote_storage_vol_get_info_ret { /* insert@1 */ unsigned hyper allocation; }; +struct remote_storage_vol_get_info_flags_args { + remote_nonnull_storage_vol vol; + unsigned int flags; +}; + +struct remote_storage_vol_get_info_flags_ret { /* insert@1 */ + char type; + unsigned hyper capacity; + unsigned hyper allocation; +}; + struct remote_storage_vol_get_path_args { remote_nonnull_storage_vol vol; }; @@ -5934,5 +5945,12 @@ enum remote_procedure { * @generate: both * @acl: none */ - REMOTE_PROC_NODE_DEVICE_EVENT_UPDATE = 377 + REMOTE_PROC_NODE_DEVICE_EVENT_UPDATE = 377, + + /** + * @generate: none + * @priority: high + * @acl: storage_vol:read + */ + REMOTE_PROC_STORAGE_VOL_GET_INFO_FLAGS = 378 }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index b71accc..41bc3bd 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -1472,6 +1472,15 @@ struct remote_storage_vol_get_info_ret { uint64_t capacity; uint64_t allocation; }; +struct remote_storage_vol_get_info_flags_args { + remote_nonnull_storage_vol vol; + u_int flags; +}; +struct remote_storage_vol_get_info_flags_ret { + char type; + uint64_t capacity; + uint64_t allocation; +}; struct remote_storage_vol_get_path_args { remote_nonnull_storage_vol vol; }; @@ -3169,4 +3178,5 @@ enum remote_procedure { REMOTE_PROC_CONNECT_NODE_DEVICE_EVENT_DEREGISTER_ANY = 375, REMOTE_PROC_NODE_DEVICE_EVENT_LIFECYCLE = 376, REMOTE_PROC_NODE_DEVICE_EVENT_UPDATE = 377, + REMOTE_PROC_STORAGE_VOL_GET_INFO_FLAGS = 378, }; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index a79acc6..5fc7066 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2620,18 +2620,21 @@ storageVolWipe(virStorageVolPtr obj, static int -storageVolGetInfo(virStorageVolPtr obj, - virStorageVolInfoPtr info) +storageVolGetInfoFlags(virStorageVolPtr obj, + virStorageVolInfoPtr info, + unsigned int flags) { virStoragePoolObjPtr pool; virStorageBackendPtr backend; virStorageVolDefPtr vol; int ret = -1; + virCheckFlags(VIR_STORAGE_VOL_GET_PHYSICAL, -1); + if (!(vol = virStorageVolDefFromVol(obj, &pool, &backend))) return -1; - if (virStorageVolGetInfoEnsureACL(obj->conn, pool->def, vol) < 0) + if (virStorageVolGetInfoFlagsEnsureACL(obj->conn, pool->def, vol) < 0) goto cleanup; if (backend->refreshVol && @@ -2641,7 +2644,10 @@ storageVolGetInfo(virStorageVolPtr obj, memset(info, 0, sizeof(*info)); info->type = vol->type; info->capacity = vol->target.capacity; - info->allocation = vol->target.allocation; + if (flags & VIR_STORAGE_VOL_GET_PHYSICAL) + info->allocation = vol->target.physical; + else + info->allocation = vol->target.allocation; ret = 0; cleanup: @@ -2649,6 +2655,15 @@ storageVolGetInfo(virStorageVolPtr obj, return ret; } + +static int +storageVolGetInfo(virStorageVolPtr obj, + virStorageVolInfoPtr info) +{ + return storageVolGetInfoFlags(obj, info, 0); +} + + static char * storageVolGetXMLDesc(virStorageVolPtr obj, unsigned int flags) @@ -2803,6 +2818,7 @@ static virStorageDriver storageDriver = { .storageVolWipe = storageVolWipe, /* 0.8.0 */ .storageVolWipePattern = storageVolWipePattern, /* 0.9.10 */ .storageVolGetInfo = storageVolGetInfo, /* 0.4.0 */ + .storageVolGetInfoFlags = storageVolGetInfoFlags, /* 3.0.0 */ .storageVolGetXMLDesc = storageVolGetXMLDesc, /* 0.4.0 */ .storageVolGetPath = storageVolGetPath, /* 0.4.0 */ .storageVolResize = storageVolResize, /* 0.9.10 */ -- 2.7.4
Add a new qualifier '--physical' to the 'vol-info' command in order to dispaly the physical size of the volume. The size can differ from the allocation value depending on the volume file time. In particular, qcow2 volumes will have a physical value larger than allocation. This also occurs for sparse files, although for those the capacity is the largest size; whereas, for qcow2 capacity is the logical size. Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-volume.c | 37 +++++++++++++++++++++++++++++++++---- tools/virsh.pod | 8 ++++++-- 2 files changed, 39 insertions(+), 6 deletions(-) diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c index 96b4a7b..f702627 100644 --- a/tools/virsh-volume.c +++ b/tools/virsh-volume.c @@ -996,6 +996,10 @@ static const vshCmdOptDef opts_vol_info[] = { .type = VSH_OT_BOOL, .help = N_("sizes are represented in bytes rather than pretty units") }, + {.name = "physical", + .type = VSH_OT_BOOL, + .help = N_("return the physical size of the volume in allocation field") + }, {.name = NULL} }; @@ -1005,14 +1009,32 @@ cmdVolInfo(vshControl *ctl, const vshCmd *cmd) virStorageVolInfo info; virStorageVolPtr vol; bool bytes = vshCommandOptBool(cmd, "bytes"); + bool physical = vshCommandOptBool(cmd, "physical"); bool ret = true; + int rc; + unsigned int flags = 0; if (!(vol = virshCommandOptVol(ctl, cmd, "vol", "pool", NULL))) return false; vshPrint(ctl, "%-15s %s\n", _("Name:"), virStorageVolGetName(vol)); - if (virStorageVolGetInfo(vol, &info) == 0) { + if (physical) + flags |= VIR_STORAGE_VOL_GET_PHYSICAL; + + if (flags) { + if ((rc = virStorageVolGetInfoFlags(vol, &info, flags)) < 0) { + flags = 0; + if (last_error->code == VIR_ERR_NO_SUPPORT) { + vshResetLibvirtError(); + rc = virStorageVolGetInfo(vol, &info); + } + } + } else { + rc = virStorageVolGetInfo(vol, &info); + } + + if (rc == 0) { double val; const char *unit; @@ -1028,11 +1050,18 @@ cmdVolInfo(vshControl *ctl, const vshCmd *cmd) } if (bytes) { - vshPrint(ctl, "%-15s %llu %s\n", _("Allocation:"), - info.allocation, _("bytes")); + if (physical) + vshPrint(ctl, "%-15s %llu %s\n", _("Physical:"), + info.allocation, _("bytes")); + else + vshPrint(ctl, "%-15s %llu %s\n", _("Allocation:"), + info.allocation, _("bytes")); } else { val = vshPrettyCapacity(info.allocation, &unit); - vshPrint(ctl, "%-15s %2.2lf %s\n", _("Allocation:"), val, unit); + if (physical) + vshPrint(ctl, "%-15s %2.2lf %s\n", _("Physical:"), val, unit); + else + vshPrint(ctl, "%-15s %2.2lf %s\n", _("Allocation:"), val, unit); } } else { ret = false; diff --git a/tools/virsh.pod b/tools/virsh.pod index 74c05c9..f606f4f 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -3862,13 +3862,17 @@ is in. I<vol-name-or-key-or-path> is the name or key or path of the volume to output the XML of. =item B<vol-info> [I<--pool> I<pool-or-uuid>] I<vol-name-or-key-or-path> -[I<--bytes>] +[I<--bytes>] [I<--physical>] Returns basic information about the given storage volume. 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 return information for. If I<--bytes> is specified the sizes are not -converted to human friendly units. +converted to human friendly units. If I<--physical> is specified, then the host +physical size is returned and displayed instead of the allocation value. The +physical value for some file types, such as qcow2 may have a different (larger) +physical value than is shown for allocation. Additionally sparse files will +have different physical and allocation values. =item B<vol-list> [I<--pool> I<pool-or-uuid>] [I<--details>] -- 2.7.4
On 13.12.2016 22:07, John Ferlan wrote:
Add a new qualifier '--physical' to the 'vol-info' command in order to dispaly the physical size of the volume. The size can differ from the allocation value depending on the volume file time. In particular, qcow2 volumes will have a physical value larger than allocation. This also occurs for sparse files, although for those the capacity is the largest size; whereas, for qcow2 capacity is the logical size.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-volume.c | 37 +++++++++++++++++++++++++++++++++---- tools/virsh.pod | 8 ++++++-- 2 files changed, 39 insertions(+), 6 deletions(-)
diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c index 96b4a7b..f702627 100644 --- a/tools/virsh-volume.c +++ b/tools/virsh-volume.c @@ -996,6 +996,10 @@ static const vshCmdOptDef opts_vol_info[] = { .type = VSH_OT_BOOL, .help = N_("sizes are represented in bytes rather than pretty units") }, + {.name = "physical", + .type = VSH_OT_BOOL, + .help = N_("return the physical size of the volume in allocation field") + }, {.name = NULL} };
@@ -1005,14 +1009,32 @@ cmdVolInfo(vshControl *ctl, const vshCmd *cmd) virStorageVolInfo info; virStorageVolPtr vol; bool bytes = vshCommandOptBool(cmd, "bytes"); + bool physical = vshCommandOptBool(cmd, "physical"); bool ret = true; + int rc; + unsigned int flags = 0;
if (!(vol = virshCommandOptVol(ctl, cmd, "vol", "pool", NULL))) return false;
vshPrint(ctl, "%-15s %s\n", _("Name:"), virStorageVolGetName(vol));
- if (virStorageVolGetInfo(vol, &info) == 0) { + if (physical) + flags |= VIR_STORAGE_VOL_GET_PHYSICAL; + + if (flags) { + if ((rc = virStorageVolGetInfoFlags(vol, &info, flags)) < 0) { + flags = 0; + if (last_error->code == VIR_ERR_NO_SUPPORT) { + vshResetLibvirtError(); + rc = virStorageVolGetInfo(vol, &info);
I don't think this is right. User had requested --physical. We should either get them what they demand or error out. This is not the case like in some other APIs where we can fetch a piece of info from different APIs and we iterate over them.
+ } + } + } else { + rc = virStorageVolGetInfo(vol, &info); + } + + if (rc == 0) { double val; const char *unit;
@@ -1028,11 +1050,18 @@ cmdVolInfo(vshControl *ctl, const vshCmd *cmd) }
if (bytes) { - vshPrint(ctl, "%-15s %llu %s\n", _("Allocation:"), - info.allocation, _("bytes")); + if (physical) + vshPrint(ctl, "%-15s %llu %s\n", _("Physical:"), + info.allocation, _("bytes")); + else + vshPrint(ctl, "%-15s %llu %s\n", _("Allocation:"), + info.allocation, _("bytes")); } else { val = vshPrettyCapacity(info.allocation, &unit); - vshPrint(ctl, "%-15s %2.2lf %s\n", _("Allocation:"), val, unit); + if (physical) + vshPrint(ctl, "%-15s %2.2lf %s\n", _("Physical:"), val, unit); + else + vshPrint(ctl, "%-15s %2.2lf %s\n", _("Allocation:"), val, unit); } } else { ret = false; diff --git a/tools/virsh.pod b/tools/virsh.pod index 74c05c9..f606f4f 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -3862,13 +3862,17 @@ is in. I<vol-name-or-key-or-path> is the name or key or path of the volume to output the XML of.
=item B<vol-info> [I<--pool> I<pool-or-uuid>] I<vol-name-or-key-or-path> -[I<--bytes>] +[I<--bytes>] [I<--physical>]
Returns basic information about the given storage volume. 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 return information for. If I<--bytes> is specified the sizes are not -converted to human friendly units. +converted to human friendly units. If I<--physical> is specified, then the host +physical size is returned and displayed instead of the allocation value. The +physical value for some file types, such as qcow2 may have a different (larger) +physical value than is shown for allocation. Additionally sparse files will +have different physical and allocation values.
If we want this to be true, we must error out if virStorageVolGetInfoFlags() fails (e.g. because its missing as a result of talking to older daemon). Otherwise, vol-info --physical ... might return allocation values which contradicts the documentation. ACK if you fix it. Michal
On 12/20/2016 07:02 AM, Michal Privoznik wrote:
On 13.12.2016 22:07, John Ferlan wrote:
Add a new qualifier '--physical' to the 'vol-info' command in order to dispaly the physical size of the volume. The size can differ from the allocation value depending on the volume file time. In particular, qcow2 volumes will have a physical value larger than allocation. This also occurs for sparse files, although for those the capacity is the largest size; whereas, for qcow2 capacity is the logical size.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-volume.c | 37 +++++++++++++++++++++++++++++++++---- tools/virsh.pod | 8 ++++++-- 2 files changed, 39 insertions(+), 6 deletions(-)
diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c index 96b4a7b..f702627 100644 --- a/tools/virsh-volume.c +++ b/tools/virsh-volume.c @@ -996,6 +996,10 @@ static const vshCmdOptDef opts_vol_info[] = { .type = VSH_OT_BOOL, .help = N_("sizes are represented in bytes rather than pretty units") }, + {.name = "physical", + .type = VSH_OT_BOOL, + .help = N_("return the physical size of the volume in allocation field") + }, {.name = NULL} };
@@ -1005,14 +1009,32 @@ cmdVolInfo(vshControl *ctl, const vshCmd *cmd) virStorageVolInfo info; virStorageVolPtr vol; bool bytes = vshCommandOptBool(cmd, "bytes"); + bool physical = vshCommandOptBool(cmd, "physical"); bool ret = true; + int rc; + unsigned int flags = 0;
if (!(vol = virshCommandOptVol(ctl, cmd, "vol", "pool", NULL))) return false;
vshPrint(ctl, "%-15s %s\n", _("Name:"), virStorageVolGetName(vol));
- if (virStorageVolGetInfo(vol, &info) == 0) { + if (physical) + flags |= VIR_STORAGE_VOL_GET_PHYSICAL; + + if (flags) { + if ((rc = virStorageVolGetInfoFlags(vol, &info, flags)) < 0) { + flags = 0; + if (last_error->code == VIR_ERR_NO_SUPPORT) { + vshResetLibvirtError(); + rc = virStorageVolGetInfo(vol, &info);
I don't think this is right. User had requested --physical. We should either get them what they demand or error out. This is not the case like in some other APIs where we can fetch a piece of info from different APIs and we iterate over them.
Fair enough... hedging my bets I suppose. I think I wrote the code before I decided to go with the if (flags) option and just 4 space moved things essentially for the opposite reason you've mentioned. Prior to the (flags) check, if 'flags == 0' we attempt to call the *InfoFlags and it doesn't exist, then fallback to the former call, but in that case failure should only happen if flags was set to physical. I'll adjust to simply be: if (flags) rc = virStorageVolGetInfoFlags(vol, &info, flags); else rc = virStorageVolGetInfo(vol, &info); Tks - John
+ } + } + } else { + rc = virStorageVolGetInfo(vol, &info); + } + + if (rc == 0) { double val; const char *unit;
@@ -1028,11 +1050,18 @@ cmdVolInfo(vshControl *ctl, const vshCmd *cmd) }
if (bytes) { - vshPrint(ctl, "%-15s %llu %s\n", _("Allocation:"), - info.allocation, _("bytes")); + if (physical) + vshPrint(ctl, "%-15s %llu %s\n", _("Physical:"), + info.allocation, _("bytes")); + else + vshPrint(ctl, "%-15s %llu %s\n", _("Allocation:"), + info.allocation, _("bytes")); } else { val = vshPrettyCapacity(info.allocation, &unit); - vshPrint(ctl, "%-15s %2.2lf %s\n", _("Allocation:"), val, unit); + if (physical) + vshPrint(ctl, "%-15s %2.2lf %s\n", _("Physical:"), val, unit); + else + vshPrint(ctl, "%-15s %2.2lf %s\n", _("Allocation:"), val, unit); } } else { ret = false; diff --git a/tools/virsh.pod b/tools/virsh.pod index 74c05c9..f606f4f 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -3862,13 +3862,17 @@ is in. I<vol-name-or-key-or-path> is the name or key or path of the volume to output the XML of.
=item B<vol-info> [I<--pool> I<pool-or-uuid>] I<vol-name-or-key-or-path> -[I<--bytes>] +[I<--bytes>] [I<--physical>]
Returns basic information about the given storage volume. 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 return information for. If I<--bytes> is specified the sizes are not -converted to human friendly units. +converted to human friendly units. If I<--physical> is specified, then the host +physical size is returned and displayed instead of the allocation value. The +physical value for some file types, such as qcow2 may have a different (larger) +physical value than is shown for allocation. Additionally sparse files will +have different physical and allocation values.
If we want this to be true, we must error out if virStorageVolGetInfoFlags() fails (e.g. because its missing as a result of talking to older daemon). Otherwise, vol-info --physical ... might return allocation values which contradicts the documentation.
ACK if you fix it.
Michal
On 13.12.2016 22:07, John Ferlan wrote:
This effort started primarily to address some ideas/thoughts brought up in https://bugzilla.redhat.com/show_bug.cgi?id=1332019 although as things were updated, it seems that using storage volume definitions wouldn't be necessary since the same data can be obtained via the virDomainGetBlockInfo API.
Still since code had been written - I figured I'd posted it and see what kind of feedback it got.
The first patch just adds the <physical> element to the output XML and documents it thusly.
The 2nd/3rd patch take a different approach adding virStorageVolInfoFlags which could take a single flag indicating that the caller would prefer to return the "physical" value instead of the "allocation" value. Yes, kind of a hack, but since we cannot extend _virStorageVolInfo to add a physical it's a mechanism to allow fetching the data using existing structures. Sure a virStorageVolStats API could be created as well, but I figured I'd see how this went first before thinking about that.
John Ferlan (3): conf: Display <physical> in output of voldef storage: Introduce virStorageVolInfoFlags virsh: Allow display of the physical volume size
daemon/remote.c | 38 +++++++++++++++++++++++++++++ docs/formatstorage.html.in | 5 ++++ include/libvirt/libvirt-storage.h | 11 +++++++++ src/conf/storage_conf.c | 6 +++++ src/driver-storage.h | 6 +++++ src/libvirt-storage.c | 51 +++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 5 ++++ src/remote/remote_driver.c | 37 ++++++++++++++++++++++++++++ src/remote/remote_protocol.x | 20 ++++++++++++++- src/remote_protocol-structs | 10 ++++++++ src/storage/storage_driver.c | 24 +++++++++++++++--- tools/virsh-volume.c | 37 +++++++++++++++++++++++++--- tools/virsh.pod | 8 ++++-- 13 files changed, 247 insertions(+), 11 deletions(-)
Looks good. ACK. BUT see my comment to 3/3 before pushing - it needs a bit of fixing. Michal
participants (2)
-
John Ferlan -
Michal Privoznik