[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] RFE: virsh list improvement (--description and --details)



On 05/05/2017 01:17 AM, Przemysław Sztoch wrote:
> 
>> On 4 May 2017, at 17:03, Michal Privoznik <mprivozn redhat com> wrote:
>>> 2. Please add new columns to virsh list (for —details):
>>> a) CPUs
>>> b) MaxMem
>>> c) Flags: p-persistent, t-transient, a-autostart, m-managed save, s-with snapshot(s)
>>> and total sum for column vCPUs and Mem.
>>>
>>> In my opinion it is very popular use case. It will help you control vCPU and memory overcommiting in very simple and fast way.
>>> Put flags on list will prevents many additional commands from being executed. You will get all important information in one place.
>>
>> Frankly, this one looks at the edge of the scope of virsh list. I mean,
>> I view virsh as a simple CLI utility that you can build your own scripts
>> on the top of.
>> Also, you want 5 flags. Cool. But tomorrow somebody else wants another 5
>> (title, description, has given device, etc.). The possibilities are
>> endless. I suggest writing your own management application on the top of
>> libvirt.
> 
> In my opinion you are wrong. It is very natural place for more information about your domains.
> Of course, everybody can build his own scripts, but bash script will be too much slow and too much complicated for typical tech stuff.

Why they would be slow?

> 
> My patch is very simple:
> virsh list —details
> 
> Please, audit attached diff, because I have not any skill about C:
> 
> index 0ca53e4..c98160d 100644
> --- a/tools/virsh-domain-monitor.c
> +++ b/tools/virsh-domain-monitor.c
> @@ -1769,6 +1769,14 @@ static const vshCmdOptDef opts_list[] = {
>       .type = VSH_OT_BOOL,
>       .help = N_("show domain title")
>      },
> +    {.name = "description",
> +     .type = VSH_OT_BOOL,
> +     .help = N_("show domain description")
> +    },
> +    {.name = "details",
> +     .type = VSH_OT_BOOL,
> +     .help = N_("show domain details")
> +    },
>      {.name = NULL}
>  };
>  
> @@ -1780,6 +1788,8 @@ cmdList(vshControl *ctl, const vshCmd *cmd)
>  {
>      bool managed = vshCommandOptBool(cmd, "managed-save");
>      bool optTitle = vshCommandOptBool(cmd, "title");
> +    bool optDescription = vshCommandOptBool(cmd, "description");
> +    bool optDetails = vshCommandOptBool(cmd, "details");
>      bool optTable = vshCommandOptBool(cmd, "table");
>      bool optUUID = vshCommandOptBool(cmd, "uuid");
>      bool optName = vshCommandOptBool(cmd, "name");
> @@ -1822,6 +1832,7 @@ cmdList(vshControl *ctl, const vshCmd *cmd)
>  
>      VSH_EXCLUSIVE_OPTIONS("table", "name");
>      VSH_EXCLUSIVE_OPTIONS("table", "uuid");
> +    VSH_EXCLUSIVE_OPTIONS("title", "description");
>  
>      if (!optUUID && !optName)
>          optTable = true;
> @@ -1836,6 +1847,18 @@ cmdList(vshControl *ctl, const vshCmd *cmd)
>                            _("Id"), _("Name"), _("State"), _("Title"),
>                            "-----------------------------------------"
>                            "-----------------------------------------");
> +        else if (optDescription)
> +            vshPrintExtra(ctl, " %-5s %-30s %-10s %-20s\n%s\n",
> +                          _("Id"), _("Name"), _("State"), _("Description"),
> +                          "-----------------------------------------"
> +                          "-----------------------------------------");
> +        else if (optDetails)
> +            vshPrintExtra(ctl, " %-5s %-30s %-10s %5s %6s %9s %-5s %-20s\n%s\n",
> +                          _("Id"), _("Name"), _("State"), _("vCPU"), 
> +                          _("Memory"), _("Snapshots"), _("Flags"), _("Time"),
> +                          "-----------------------------------------"
> +                          "-----------------------------------------"
> +                          "----------------------");

What if I'd run 'list --description --details'? Only description header
will be printed out.

>          else
>              vshPrintExtra(ctl, " %-5s %-30s %s\n%s\n",
>                            _("Id"), _("Name"), _("State"),
> @@ -1862,8 +1885,8 @@ cmdList(vshControl *ctl, const vshCmd *cmd)
>                  virDomainHasManagedSaveImage(dom, 0) > 0)
>                  state = -2;
>  
> -            if (optTitle) {
> -                if (!(title = virshGetDomainDescription(ctl, dom, true, 0)))
> +            if (optTitle || optDescription) {
> +                if (!(title = virshGetDomainDescription(ctl, dom, optTitle, 0)))
>                      goto cleanup;
>  
>                  vshPrint(ctl, " %-5s %-30s %-10s %-20s\n", id_buf,
> @@ -1873,6 +1896,62 @@ cmdList(vshControl *ctl, const vshCmd *cmd)
>                           title);
>  
>                  VIR_FREE(title);
> +            } else if (optDetails) {
> +        	int vcpu = -1;
> +        	int memory = -1;
> +		int persistent;
> +		int autostart;
> +		int nsnap;
> +		int mansave;
> +		long long seconds = 0;
> +		unsigned int nseconds = 0;
> +		
> +		virDomainInfo info;
> +
> +		if (virDomainGetInfo(dom, &info) == 0) {
> +		    vcpu = info.nrVirtCpu;
> +		    memory = info.memory / 1024;
> +		}

I think that instead of silently ignoring errors, we should report the
error message and fail. This applies here and in the rest of your patch.

> +
> +        	persistent = virDomainIsPersistent(dom);
> +        	if (virDomainGetAutostart(dom, &autostart) < 0) {
> +        	    autostart = -1;
> +        	}
> +        	nsnap = virDomainSnapshotNum(dom, 0);
> +        	mansave = virDomainHasManagedSaveImage(dom, 0);
> +		
> +        	char timestr[100];
> +		if (state == VIR_DOMAIN_RUNNING) {		
> +    		    if (virDomainGetTime(dom, &seconds, &nseconds, false) < 0) {

false? The last argument is 'unsigned int flags'. s/false/0/

> +        		strcpy(timestr, "??");
> +        	    } else {
> +        		time_t cur_time = seconds;
> +        		struct tm time_info;
> +
> +        		if (!gmtime_r(&cur_time, &time_info)) {
> +            		    vshError(ctl, _("Unable to format time"));
> +            		    goto cleanup;
> +        		}
> +        		strftime(timestr, sizeof(timestr), "%Y-%m-%d %H:%M:%S", &time_info);
> +            	    }
> +        	} else {
> +        	    strcpy(timestr, "");
> +            	}
> +
> +                vshPrint(ctl, " %-5s %-30s %-10s %5d %6d %9d %s%s%s%s%s %s\n", id_buf,
> +                         virDomainGetName(dom),
> +                         state == -2 ? _("saved")
> +                         : virshDomainStateToString(state),
> +                         vcpu, memory,
> +                         nsnap,
> +                         persistent == 1 ? "p" : persistent == 0 ? "t" : "?",
> +                         autostart == 1 ? "a" : autostart == 0 ? " " : "?",
> +                         mansave == 1 ? "m" : mansave == 0 ? " " : "?",
> +                         nsnap > 0 ? "s" : nsnap == 0 ? " " : "?",
> +                         " ",
> +                         timestr
> +                         );
> +
>              } else {
>                  vshPrint(ctl, " %-5s %-30s %s\n", id_buf,
>                           virDomainGetName(dom),
> 

Michal


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]