[libvirt] [PATCH v3 3/4] cmdDomblkinfoPrint: support printing "-" for invalid virDomainBlockInfo

[John Ferlan]

On 06/19/2018 06:01 AM, Chen Hanxiao wrote:
> From: Chen Hanxiao <chenhanxiao at gmail.com>
> 
> For inactive domain, we'll set virDomainBlockInfo to 0
> if specific error code got.
> Print "-" to show the value should be ignored in this scenario.
> 
> Signed-off-by: Chen Hanxiao <chenhanxiao at gmail.com>
> ---
>  tools/virsh-domain-monitor.c | 73 ++++++++++++++++++++++++------------
>  1 file changed, 49 insertions(+), 24 deletions(-)
> 

I understand why it's separate for ease of reading, but this
should have been combined with the previous patch.

> diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
> index 43e39f79c1..3acf5450b3 100644
> --- a/tools/virsh-domain-monitor.c
> +++ b/tools/virsh-domain-monitor.c
> @@ -410,6 +410,15 @@ cmdDomblkinfoPrint(vshControl *ctl,
>                     bool human, bool title)
>  {
>      char *cap = NULL, *alloc = NULL, *phy = NULL;
> +    bool invalid = false;
> +
> +    struct blockInfoText {
> +        char *capacity;
> +        char *allocation;
> +        char *physical;
> +    };
> +
> +    struct blockInfoText *blkInfoText = NULL;

All this is over complicated an unnecessary - especially since
you already have cap, alloc, and phy which now went unused!
'll fix before pushing.

>  
>      if (title) {
>          vshPrintExtra(ctl, "%-10s %-15s %-15s %-15s\n", _("Target"),
> @@ -419,15 +428,23 @@ cmdDomblkinfoPrint(vshControl *ctl,
>          return;
>      }
>  
> -    if (!human) {
> -        if (device) {
> -            vshPrint(ctl, "%-10s %-15llu %-15llu %-15llu\n", device,
> -                     info->capacity, info->allocation, info->physical);
> -        } else {
> -            vshPrint(ctl, "%-15s %llu\n", _("Capacity:"), info->capacity);
> -            vshPrint(ctl, "%-15s %llu\n", _("Allocation:"), info->allocation);
> -            vshPrint(ctl, "%-15s %llu\n", _("Physical:"), info->physical);
> -        }
> +    invalid = info->capacity == 0 &&
> +              info->allocation == 0 &&
> +              info->physical == 0;
> +    blkInfoText = vshCalloc(ctl, 1, sizeof(*blkInfoText));
> +
> +    if (invalid) {

@invalid is unnecessary since it's used just once - I'll adjust.

> +        blkInfoText->capacity = vshStrdup(ctl, "-");
> +        blkInfoText->allocation = vshStrdup(ctl, "-");
> +        blkInfoText->physical = vshStrdup(ctl, "-");
> +    } else if (!human) {
> +        if (virAsprintf(&blkInfoText->capacity, "%llu",
> +                        info->capacity) < 0 ||
> +            virAsprintf(&blkInfoText->allocation, "%llu",
> +                        info->allocation) < 0 ||
> +            virAsprintf(&blkInfoText->physical, "%llu",
> +                        info->physical) < 0)
> +            goto cleanup;
>      } else {
>          double val_cap, val_alloc, val_phy;
>          const char *unit_cap, *unit_alloc, *unit_phy;
> @@ -435,28 +452,36 @@ cmdDomblkinfoPrint(vshControl *ctl,
>          val_cap = vshPrettyCapacity(info->capacity, &unit_cap);
>          val_alloc = vshPrettyCapacity(info->allocation, &unit_alloc);
>          val_phy = vshPrettyCapacity(info->physical, &unit_phy);
> -        if (device) {
> -            if (virAsprintf(&cap, "%.3lf %s", val_cap, unit_cap) < 0 ||
> -                virAsprintf(&alloc, "%.3lf %s", val_alloc, unit_alloc) < 0 ||
> -                virAsprintf(&phy, "%.3lf %s", val_phy, unit_phy) < 0)
> -                goto cleanup;
>  
> -            vshPrint(ctl, "%-10s %-15s %-15s %-15s\n",
> -                     device, cap, alloc, phy);
> -        } else {
> -            vshPrint(ctl, "%-15s %-.3lf %s\n", _("Capacity:"),
> -                     val_cap, unit_cap);
> -            vshPrint(ctl, "%-15s %-.3lf %s\n", _("Allocation:"),
> -                     val_alloc, unit_alloc);
> -            vshPrint(ctl, "%-15s %-.3lf %s\n", _("Physical:"),
> -                     val_phy, unit_phy);
> -        }
> +        if (virAsprintf(&blkInfoText->capacity, "%.3lf %s",
> +                        val_cap, unit_cap) < 0 ||
> +            virAsprintf(&blkInfoText->allocation, "%.3lf %s",
> +                        val_alloc, unit_alloc) < 0 ||
> +            virAsprintf(&blkInfoText->physical, "%.3lf %s",
> +                        val_phy, unit_phy) < 0)
> +            goto cleanup;
> +    }
> +
> +    if (device) {
> +        vshPrint(ctl, "%-10s %-15s %-15s %-15s\n", device,
> +                 blkInfoText->capacity, blkInfoText->allocation,
> +                 blkInfoText->physical);
> +    } else {
> +        vshPrint(ctl, "%-15s %s\n", _("Capacity:"), blkInfoText->capacity);
> +        vshPrint(ctl, "%-15s %s\n", _("Allocation:"), blkInfoText->allocation);
> +        vshPrint(ctl, "%-15s %s\n", _("Physical:"), blkInfoText->physical);
>      }
>  
>   cleanup:
>      VIR_FREE(cap);
>      VIR_FREE(alloc);
>      VIR_FREE(phy);
> +    if (blkInfoText) {
> +        VIR_FREE(blkInfoText->capacity);
> +        VIR_FREE(blkInfoText->allocation);
> +        VIR_FREE(blkInfoText->physical);
> +    }
> +    VIR_FREE(blkInfoText);
>  }
>  
>  static bool
> 

NB: I'm going to merge this into the previous, adding
the following to the commit message:

"For inactive domains using networked storage, a "-" will
be printed instead of the value since it's not possible
to ascertain the value without the storage connection."

The code will now look like:

static void
cmdDomblkinfoPrint(vshControl *ctl,
                   const virDomainBlockInfo *info,
                   const char *device,
                   bool human, bool title)
{
    char *cap = NULL;
    char *alloc = NULL;
    char *phy = NULL;

    if (title) {
        vshPrintExtra(ctl, "%-10s %-15s %-15s %-15s\n", _("Target"),
                      _("Capacity"), _("Allocation"), _("Physical"));
        vshPrintExtra(ctl, "-----------------------------"
                      "------------------------\n");
        return;
    }

    if (info->capacity == 0 && info->allocation == 0 && info->physical == 0) {
        cap = vshStrdup(ctl, "-");
        alloc = vshStrdup(ctl, "-");
        phy = vshStrdup(ctl, "-");
    } else if (!human) {
        if (virAsprintf(&cap, "%llu", info->capacity) < 0 ||
            virAsprintf(&alloc, "%llu", info->allocation) < 0 ||
            virAsprintf(&phy, "%llu", info->physical) < 0)
            goto cleanup;
    } else {
        double val_cap, val_alloc, val_phy;
        const char *unit_cap, *unit_alloc, *unit_phy;

        val_cap = vshPrettyCapacity(info->capacity, &unit_cap);
        val_alloc = vshPrettyCapacity(info->allocation, &unit_alloc);
        val_phy = vshPrettyCapacity(info->physical, &unit_phy);

        if (virAsprintf(&cap, "%.3lf %s", val_cap, unit_cap) < 0 ||
            virAsprintf(&alloc, "%.3lf %s", val_alloc, unit_alloc) < 0 ||
            virAsprintf(&phy, "%.3lf %s", val_phy, unit_phy) < 0)
            goto cleanup;
    }

    if (device) {
        vshPrint(ctl, "%-10s %-15s %-15s %-15s\n", device, cap, alloc, phy);
     } else {
        vshPrint(ctl, "%-15s %s\n", _("Capacity:"), cap);
        vshPrint(ctl, "%-15s %s\n", _("Allocation:"), alloc);
        vshPrint(ctl, "%-15s %s\n", _("Physical:"), phy);
     }
 
 cleanup:
    VIR_FREE(cap);
    VIR_FREE(alloc);
    VIR_FREE(phy);
}