Re: [libvirt] [PATCH 3/3] virsh: Allow display of the physical volume size

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&gt; > --- > 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