Re: [PATCH v2 02/10] virsh: Refactor the table output of the list command

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

 



On Thu, Dec 10, 2015 at 03:13:36PM -0500, John Ferlan wrote:

On 12/01/2015 12:35 PM, Martin Kletzander wrote:
Clean up some duplicate code so it is easier to add new parameters.  The
tests need to be adjusted because after this patch there will be
additional spaces at the end of lines in the output of virsh list, but
this affects only the table output that is meant to be read by users.

Signed-off-by: Martin Kletzander <mkletzan@xxxxxxxxxx>
---
 tests/virshtest.c            | 14 +++++++-------
 tools/virsh-domain-monitor.c | 38 +++++++++++++++++++-------------------
 2 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/tests/virshtest.c b/tests/virshtest.c
index 3fdae92b7834..5983b5b190d6 100644
--- a/tests/virshtest.c
+++ b/tests/virshtest.c
@@ -98,9 +98,9 @@ static int testCompareListDefault(const void *data ATTRIBUTE_UNUSED)
 {
   const char *const argv[] = { VIRSH_DEFAULT, "list", NULL };
   const char *exp = "\
- Id    Name                           State\n\
-----------------------------------------------------\n\
- 1     test                           running\n\
+ Id    Name                           State     \n\
+-------------------------------------------------\n\
+ 1     test                           running   \n\
 \n";
   return testCompareOutputLit(exp, NULL, argv);
 }
@@ -109,10 +109,10 @@ static int testCompareListCustom(const void *data ATTRIBUTE_UNUSED)
 {
   const char *const argv[] = { VIRSH_CUSTOM, "list", NULL };
   const char *exp = "\
- Id    Name                           State\n\
-----------------------------------------------------\n\
- 1     fv0                            running\n\
- 2     fc4                            running\n\
+ Id    Name                           State     \n\
+-------------------------------------------------\n\
+ 1     fv0                            running   \n\
+ 2     fc4                            running   \n\
[1]If only to make the output line up w/r/t \n...

I can do that.  I don't like this output either, but I imagined
increasing the complexity of the output generating would gain negative
comments from others.  It's not needed for this series, just a
nice-to-have feature once this series is in (if ever).

 \n";
   return testCompareOutputLit(exp, NULL, argv);
 }
diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
index db5bf4b2a566..24b902905968 100644
--- a/tools/virsh-domain-monitor.c
+++ b/tools/virsh-domain-monitor.c
@@ -1914,16 +1914,18 @@ cmdList(vshControl *ctl, const vshCmd *cmd)

     /* print table header in legacy mode */
     if (optTable) {
+        vshPrintExtra(ctl, " %-5s %-30s %-10s", _("Id"), _("Name"), _("State"));
+
         if (optTitle)
-            vshPrintExtra(ctl, " %-5s %-30s %-10s %-20s\n%s\n",
-                          _("Id"), _("Name"), _("State"), _("Title"),
-                          "-----------------------------------------"
-                          "-----------------------------------------");
-        else
-            vshPrintExtra(ctl, " %-5s %-30s %s\n%s\n",
-                          _("Id"), _("Name"), _("State"),
-                          "-----------------------------------------"
-                          "-----------");
+            vshPrintExtra(ctl, " %-20s\n", _("Title"));
                                        ^^
I believe if Title were added there'd be an extra line once the
following dashes are printed (end w/ \n, start with \n)

I'll fix that as a part of another round of rework.

+
+        vshPrintExtra(ctl, "\n%s",
+                      "-------------------------------------------------");
+
+        if (optTitle)
+            vshPrintExtra(ctl, "%s", "---------------------");
It does seem in the expected virshtest.c output we're still off by 1.
That is there is 1 more "-" than extra space after the State column.

[1]
Instead of counting dashes...

   dashcount = 45;  /* Or some value - whatever it is */
...
   if (optTitle) {
...
       dashcount += 20;
  }
,,,
   for (i = 0; i < dashcount; i++)
       buf[i] = '-';
   vshPrintExtra(ctl, "%s\n", buf);
...

where of course buf is appropriately defined ;-)  A nit would be that
the old output was 3 dashes longer.

I'll generalize and unify it so that it looks nice most of the time.
For the time being I did not talk at all about what non-ASCII characters
do wit the output and I'm not going to deal with that, mainly because
it' s aproblem of (f)printf itself.

ACK with at least the fixup as a result of having Title on the line (and
it would be nice to add a test for it too)!

Whether you implement the '-' hack is up to you, but it may make future
adjustments more simple...

Not that I would see any adjustments coming, there wasn't mucha
happening in this part of the code for some time, but I agree that if it
can be done better I should try more ;)

John

+
+        vshPrintExtra(ctl, "\n");
     }

     for (i = 0; i < list->ndomains; i++) {
@@ -1945,23 +1947,21 @@ cmdList(vshControl *ctl, const vshCmd *cmd)
             state = -2;

         if (optTable) {
+            vshPrint(ctl, " %-5s %-30s %-10s", id_buf,
+                     virDomainGetName(dom),
+                     state == -2 ? _("saved")
+                     : virshDomainStateToString(state));
+
             if (optTitle) {
                 if (!(title = virshGetDomainDescription(ctl, dom, true, 0)))
                     goto cleanup;

-                vshPrint(ctl, " %-5s %-30s %-10s %-20s\n", id_buf,
-                         virDomainGetName(dom),
-                         state == -2 ? _("saved")
-                         : virshDomainStateToString(state),
-                         title);
+                vshPrint(ctl, " %-20s", title);

                 VIR_FREE(title);
-            } else {
-                vshPrint(ctl, " %-5s %-30s %s\n", id_buf,
-                         virDomainGetName(dom),
-                         state == -2 ? _("saved")
-                         : virshDomainStateToString(state));
             }
+
+            vshPrint(ctl, "\n");
         } else if (optUUID) {
             if (virDomainGetUUIDString(dom, uuid) < 0) {
                 vshError(ctl, "%s", _("Failed to get domain's UUID"));

Attachment: signature.asc
Description: PGP signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]