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

[John Ferlan]

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 at 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
>