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