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(a)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