On 05/05/2017 01:17 AM, Przemysław Sztoch wrote:
>
>> On 4 May 2017, at 17:03, Michal Privoznik <mprivozn@xxxxxxxxxx> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
