4:39 a.m.
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(a)redhat.com>
> ---
> 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"));
>