[PATCH 0/7] vsh-table: clean up, add support for column modes and fix 'virsh nodememstats' alignment with the new code
The output of virsh nodememstats was misaligned after recent addition. Add new modes to vshTable and use it. Peter Krempa (7): vsh-table: Refactor cleanup in 'vshTableRowNew' vsh-table: Refactor 'vshTableRowAppend' vsh-table: Refactor 'vshTableNew' vshTableGetColumnsWidths: Include spacing in lenght calculation vsh-table: Add support for right-align and skipping the embedded whitespace tests: vshtable: Excercise all flag combinations in vshTableRowAppendFlags virsh: cmdNodeMemStats: Rework to vshTable tests/vshtabletest.c | 68 +++++++++++ tests/vshtabletestdata/testRowsWithFlags.out | 24 ++++ tools/virsh-host.c | 17 ++- tools/vsh-table.c | 113 ++++++++++++++----- tools/vsh-table.h | 14 +++ 5 files changed, 208 insertions(+), 28 deletions(-) create mode 100644 tests/vshtabletestdata/testRowsWithFlags.out -- 2.53.0
From: Peter Krempa <pkrempa@redhat.com> The 'error' label is not needed, we can directly return failure from the only error. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/vsh-table.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/tools/vsh-table.c b/tools/vsh-table.c index da7dc84ee8..20d25eb227 100644 --- a/tools/vsh-table.c +++ b/tools/vsh-table.c @@ -92,7 +92,7 @@ vshTableRowNew(const char *arg, va_list ap) if (!arg) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Table row cannot be empty")); - goto error; + return NULL; } row = g_new0(vshTableRow, 1); @@ -108,10 +108,6 @@ vshTableRowNew(const char *arg, va_list ap) } return row; - - error: - vshTableRowFree(row); - return NULL; } -- 2.53.0
On a Friday in 2026, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
The 'error' label is not needed, we can directly return failure from the only error.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/vsh-table.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
From: Peter Krempa <pkrempa@redhat.com> Register cleanup function for vshTableRow and use it to simplify cleanup in 'vshTableRowAppend' Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/vsh-table.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/tools/vsh-table.c b/tools/vsh-table.c index 20d25eb227..13e9887cba 100644 --- a/tools/vsh-table.c +++ b/tools/vsh-table.c @@ -57,6 +57,7 @@ vshTableRowFree(vshTableRow *row) g_free(row->cells); g_free(row); } +G_DEFINE_AUTOPTR_CLEANUP_FUNC(vshTableRow, vshTableRowFree); void @@ -158,30 +159,26 @@ vshTableNew(const char *arg, ...) int vshTableRowAppend(vshTable *table, const char *arg, ...) { - vshTableRow *row = NULL; + g_autoptr(vshTableRow) row = NULL; size_t ncolumns = table->rows[0]->ncells; va_list ap; - int ret = -1; va_start(ap, arg); row = vshTableRowNew(arg, ap); va_end(ap); if (!row) - goto cleanup; + return -1; if (ncolumns != row->ncells) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Incorrect number of cells in a table row")); - goto cleanup; + return -1; } VIR_APPEND_ELEMENT(table->rows, table->nrows, row); - ret = 0; - cleanup: - vshTableRowFree(row); - return ret; + return 0; } -- 2.53.0
On a Friday in 2026, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
Register cleanup function for vshTableRow and use it to simplify cleanup in 'vshTableRowAppend'
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/vsh-table.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
From: Peter Krempa <pkrempa@redhat.com> Use automatic memory freeing to remove 'error' label. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/vsh-table.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/tools/vsh-table.c b/tools/vsh-table.c index 13e9887cba..0c96407f03 100644 --- a/tools/vsh-table.c +++ b/tools/vsh-table.c @@ -123,8 +123,8 @@ vshTableRowNew(const char *arg, va_list ap) vshTable * vshTableNew(const char *arg, ...) { - vshTable *table = NULL; - vshTableRow *header = NULL; + g_autoptr(vshTable) table = NULL; + g_autoptr(vshTableRow) header = NULL; va_list ap; table = g_new0(vshTable, 1); @@ -134,15 +134,11 @@ vshTableNew(const char *arg, ...) va_end(ap); if (!header) - goto error; + return NULL; VIR_APPEND_ELEMENT(table->rows, table->nrows, header); - return table; - error: - vshTableRowFree(header); - vshTableFree(table); - return NULL; + return g_steal_pointer(&table); } -- 2.53.0
On a Friday in 2026, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
Use automatic memory freeing to remove 'error' label.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/vsh-table.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
From: Peter Krempa <pkrempa@redhat.com> Modify the array holding lenghts of individual colums in the table to include the spacing. This will be used later when we'll allow to modify the spacing. To do this we'll include the 3 extra spaces as lenghths as well as fix the two loops using the value to use it directly. Since the spacing is not included in the string the code in 'vshTableRowPrint' is modified to explicitly add the spacing instead of adding a constant to the calculated length. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/vsh-table.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/tools/vsh-table.c b/tools/vsh-table.c index 0c96407f03..0e537dd106 100644 --- a/tools/vsh-table.c +++ b/tools/vsh-table.c @@ -257,7 +257,8 @@ vshTableSafeEncode(const char *s, size_t *width) * * Fill passed @maxwidths and @widths arrays with maximum number * of characters for columns and number of character per each - * table cell, respectively. + * table cell, respectively. Both lengths include the extra whitespace for + * separation of colums. * Handle unicode strings (user must have multibyte locale) * * Return 0 in case of success, -1 otherwise. @@ -284,6 +285,9 @@ vshTableGetColumnsWidths(vshTable *table, if (!tmp) return -1; + /* include the built-in whitespace in the calculated length */ + size += 3; + VIR_FREE(row->cells[j]); row->cells[j] = tmp; widths[i][j] = size; @@ -317,8 +321,9 @@ vshTableRowPrint(vshTableRow *row, virBufferAsprintf(buf, " %s", row->cells[i]); if (i < (row->ncells - 1)) { - for (j = 0; j < maxwidths[i] - widths[i] + 2; j++) + for (j = 0; j < maxwidths[i] - widths[i]; j++) virBufferAddChar(buf, ' '); + virBufferAddLit(buf, " "); } } virBufferAddChar(buf, '\n'); @@ -365,7 +370,7 @@ vshTablePrint(vshTable *table, bool header) /* print dividing line */ for (i = 0; i < table->rows[0]->ncells; i++) { - for (j = 0; j < maxwidths[i] + 3; j++) + for (j = 0; j < maxwidths[i]; j++) virBufferAddChar(&buf, '-'); } virBufferAddChar(&buf, '\n'); -- 2.53.0
On a Friday in 2026, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
In the summary, s/lenght/length/
Modify the array holding lenghts of individual colums in the table to
lengths columns
include the spacing. This will be used later when we'll allow to modify the spacing.
To do this we'll include the 3 extra spaces as lenghths as well as fix
lengths
the two loops using the value to use it directly.
Since the spacing is not included in the string the code in 'vshTableRowPrint' is modified to explicitly add the spacing instead of adding a constant to the calculated length.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/vsh-table.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/tools/vsh-table.c b/tools/vsh-table.c index 0c96407f03..0e537dd106 100644 --- a/tools/vsh-table.c +++ b/tools/vsh-table.c @@ -257,7 +257,8 @@ vshTableSafeEncode(const char *s, size_t *width) * * Fill passed @maxwidths and @widths arrays with maximum number * of characters for columns and number of character per each - * table cell, respectively. + * table cell, respectively. Both lengths include the extra whitespace for + * separation of colums.
columns
* Handle unicode strings (user must have multibyte locale) * * Return 0 in case of success, -1 otherwise.
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
From: Peter Krempa <pkrempa@redhat.com> In certain cases the code might want to skip the forced spacing. Introduce a concept of flags for each column and new function 'vshTableRowAppendFlags' which will do similar job as 'vshTableRowAppend' but add tuples of flags and the string itself. This patch implements the following flags: VSH_TABLE_CELL_SKIP_LEADING - skips the single leading whitespace VSH_TABLE_CELL_SKIP_TRAILING - skips the trailing 2 whitespaces VSH_TABLE_CELL_ALIGN_RIGHT - moves the alignment to the right of the column Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/vsh-table.c | 75 ++++++++++++++++++++++++++++++++++++++++++++--- tools/vsh-table.h | 14 +++++++++ 2 files changed, 85 insertions(+), 4 deletions(-) diff --git a/tools/vsh-table.c b/tools/vsh-table.c index 0e537dd106..0502546bab 100644 --- a/tools/vsh-table.c +++ b/tools/vsh-table.c @@ -33,6 +33,7 @@ typedef struct _vshTableRow vshTableRow; struct _vshTableRow { char **cells; + unsigned int *flags; size_t ncells; }; @@ -55,6 +56,7 @@ vshTableRowFree(vshTableRow *row) g_free(row->cells[i]); g_free(row->cells); + g_free(row->flags); g_free(row); } G_DEFINE_AUTOPTR_CLEANUP_FUNC(vshTableRow, vshTableRowFree); @@ -89,6 +91,7 @@ static vshTableRow * vshTableRowNew(const char *arg, va_list ap) { vshTableRow *row = NULL; + size_t nflags = 0; if (!arg) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -100,10 +103,12 @@ vshTableRowNew(const char *arg, va_list ap) while (arg) { g_autofree char *tmp = NULL; + unsigned int fn = VSH_TABLE_CELL_DEFAULT; tmp = g_strdup(arg); VIR_APPEND_ELEMENT(row->cells, row->ncells, tmp); + VIR_APPEND_ELEMENT(row->flags, nflags, fn); arg = va_arg(ap, const char *); } @@ -178,6 +183,52 @@ vshTableRowAppend(vshTable *table, const char *arg, ...) } +/** + * vshTableRowAppendFlags: + * @table: table to append to + * @flags: start of the cell tuples of the row row (NULL terminated) + * + * Append new row into the @table. The number of cells in the row has + * to be equal to the number of cells in the table header. Each column consists + * of a tuple of an 'unsigned int' corresponding to bitwise-or of + * vshTableCellFlags and an const char * representing the value to add. + * + * Returns: 0 if succeeded, -1 if failed. + */ +int +vshTableRowAppendFlags(vshTable *table, + unsigned int f1, + const char *c1, + ...) +{ + g_autoptr(vshTableRow) row = g_new0(vshTableRow, 1); + size_t ncolumns = table->rows[0]->ncells; + va_list ap; + unsigned int fn = f1; + g_autofree char *cn = g_strdup(c1); + size_t nflags = 0; + + va_start(ap, c1); + while (cn) { + VIR_APPEND_ELEMENT(row->flags, nflags, fn); + VIR_APPEND_ELEMENT(row->cells, row->ncells, cn); + + fn = va_arg(ap, unsigned int); + cn = g_strdup(va_arg(ap, const char *)); + } + va_end(ap); + + if (ncolumns != row->ncells) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Incorrect number of cells in a table row")); + return -1; + } + + VIR_APPEND_ELEMENT(table->rows, table->nrows, row); + + return 0; +} + /** * Function pulled from util-linux * @@ -286,7 +337,11 @@ vshTableGetColumnsWidths(vshTable *table, return -1; /* include the built-in whitespace in the calculated length */ - size += 3; + if (!(row->flags[j] & VSH_TABLE_CELL_SKIP_LEADING)) + size += 1; + + if (!(row->flags[j] & VSH_TABLE_CELL_SKIP_TRAILING)) + size += 2; VIR_FREE(row->cells[j]); row->cells[j] = tmp; @@ -318,13 +373,25 @@ vshTableRowPrint(vshTableRow *row, size_t j; for (i = 0; i < row->ncells; i++) { - virBufferAsprintf(buf, " %s", row->cells[i]); + if (!(row->flags[i] & VSH_TABLE_CELL_SKIP_LEADING)) + virBufferAddLit(buf, " "); - if (i < (row->ncells - 1)) { + if (row->flags[i] & VSH_TABLE_CELL_ALIGN_RIGHT) { for (j = 0; j < maxwidths[i] - widths[i]; j++) virBufferAddChar(buf, ' '); - virBufferAddLit(buf, " "); + virBufferAsprintf(buf, "%s", row->cells[i]); + } else { + virBufferAsprintf(buf, "%s", row->cells[i]); + + if (i < (row->ncells - 1)) { + for (j = 0; j < maxwidths[i] - widths[i]; j++) + virBufferAddChar(buf, ' '); + } } + + if (!(row->flags[i] & VSH_TABLE_CELL_SKIP_TRAILING) && + i < (row->ncells - 1)) + virBufferAddLit(buf, " "); } virBufferAddChar(buf, '\n'); } diff --git a/tools/vsh-table.h b/tools/vsh-table.h index 18d5139aa6..cffc9c3dbc 100644 --- a/tools/vsh-table.h +++ b/tools/vsh-table.h @@ -24,6 +24,13 @@ typedef struct _vshTable vshTable; +typedef enum { + VSH_TABLE_CELL_DEFAULT = 0, + VSH_TABLE_CELL_SKIP_LEADING = 1 << 0, + VSH_TABLE_CELL_SKIP_TRAILING = 1 << 1, + VSH_TABLE_CELL_ALIGN_RIGHT = 1 << 2, +} vshTableCellFlags; + void vshTableFree(vshTable *table); G_DEFINE_AUTOPTR_CLEANUP_FUNC(vshTable, vshTableFree); @@ -36,6 +43,13 @@ int vshTableRowAppend(vshTable *table, const char *arg, ...) G_GNUC_NULL_TERMINATED; +int +vshTableRowAppendFlags(vshTable *table, + unsigned int f1, + const char *c1, + ...) + G_GNUC_NULL_TERMINATED; + void vshTablePrintToStdout(vshTable *table, vshControl *ctl); -- 2.53.0
From: Peter Krempa <pkrempa@redhat.com> The new test case iterates over all combinations and tries the formatting of the table with strings of 3 distinct lengths (1 char, 8 chars and maximum widht that fits in the column (8+3 characters)). Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/vshtabletest.c | 68 ++++++++++++++++++++ tests/vshtabletestdata/testRowsWithFlags.out | 24 +++++++ 2 files changed, 92 insertions(+) create mode 100644 tests/vshtabletestdata/testRowsWithFlags.out diff --git a/tests/vshtabletest.c b/tests/vshtabletest.c index bb75a41418..e4c670d27f 100644 --- a/tests/vshtabletest.c +++ b/tests/vshtabletest.c @@ -316,6 +316,71 @@ testNTables(const void *opaque G_GNUC_UNUSED) return 0; } + +static int +testRowsWithFlags(const void *opaque G_GNUC_UNUSED) +{ + g_autoptr(vshTable) t = vshTableNew("comment", "|", "content", "|", NULL); + g_autofree char *act = NULL; + unsigned int fl; + unsigned int maxfl = VSH_TABLE_CELL_SKIP_LEADING | VSH_TABLE_CELL_SKIP_TRAILING | VSH_TABLE_CELL_ALIGN_RIGHT; + + for (fl = 0; fl <= maxfl; fl++) { + size_t maxlen = 8; + g_auto(virBuffer) b = VIR_BUFFER_INITIALIZER; + g_auto(virBuffer) n = VIR_BUFFER_INITIALIZER; + size_t i; + + virBufferAddLit(&n, "base"); + + if (fl & VSH_TABLE_CELL_SKIP_LEADING) { + maxlen += 1; + virBufferAddLit(&n, "+skip_leading"); + } + + if (fl & VSH_TABLE_CELL_SKIP_TRAILING) { + maxlen += 2; + virBufferAddLit(&n, "+skip_trailing"); + } + + if (fl & VSH_TABLE_CELL_ALIGN_RIGHT) { + virBufferAddLit(&n, "+align_right"); + } + + for (i = 0; i < maxlen; i++) + virBufferAddChar(&b, 'b'); + + vshTableRowAppendFlags(t, + VSH_TABLE_CELL_DEFAULT, virBufferCurrentContent(&n), + VSH_TABLE_CELL_SKIP_LEADING | VSH_TABLE_CELL_SKIP_TRAILING, "|", + fl, "short", + VSH_TABLE_CELL_SKIP_LEADING | VSH_TABLE_CELL_SKIP_TRAILING, "|", + 0, NULL); + + vshTableRowAppendFlags(t, + VSH_TABLE_CELL_DEFAULT, virBufferCurrentContent(&n), + VSH_TABLE_CELL_SKIP_LEADING | VSH_TABLE_CELL_SKIP_TRAILING, "|", + fl, "8__chars", + VSH_TABLE_CELL_SKIP_LEADING | VSH_TABLE_CELL_SKIP_TRAILING, "|", + 0, NULL); + + vshTableRowAppendFlags(t, + VSH_TABLE_CELL_DEFAULT, virBufferCurrentContent(&n), + VSH_TABLE_CELL_SKIP_LEADING | VSH_TABLE_CELL_SKIP_TRAILING, "|", + fl, virBufferCurrentContent(&b), + VSH_TABLE_CELL_SKIP_LEADING | VSH_TABLE_CELL_SKIP_TRAILING, "|", + 0, NULL); + } + + act = vshTablePrintToString(t, false); + + if (virTestCompareToFile(act, abs_srcdir "/vshtabletestdata/testRowsWithFlags.out") < 0) + return -1; + + return 0; +} + + static int mymain(void) { @@ -357,6 +422,9 @@ mymain(void) if (virTestRun("testNTables", testNTables, NULL) < 0) ret = -1; + if (virTestRun("testRowsWithFlags", testRowsWithFlags, NULL) < 0) + ret = -1; + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } diff --git a/tests/vshtabletestdata/testRowsWithFlags.out b/tests/vshtabletestdata/testRowsWithFlags.out new file mode 100644 index 0000000000..5de74586eb --- /dev/null +++ b/tests/vshtabletestdata/testRowsWithFlags.out @@ -0,0 +1,24 @@ + base | short | + base | 8__chars | + base | bbbbbbbb | + base+skip_leading |short | + base+skip_leading |8__chars | + base+skip_leading |bbbbbbbbb | + base+skip_trailing | short | + base+skip_trailing | 8__chars | + base+skip_trailing | bbbbbbbbbb| + base+skip_leading+skip_trailing |short | + base+skip_leading+skip_trailing |8__chars | + base+skip_leading+skip_trailing |bbbbbbbbbbb| + base+align_right | short | + base+align_right | 8__chars | + base+align_right | bbbbbbbb | + base+skip_leading+align_right | short | + base+skip_leading+align_right | 8__chars | + base+skip_leading+align_right |bbbbbbbbb | + base+skip_trailing+align_right | short| + base+skip_trailing+align_right | 8__chars| + base+skip_trailing+align_right | bbbbbbbbbb| + base+skip_leading+skip_trailing+align_right | short| + base+skip_leading+skip_trailing+align_right | 8__chars| + base+skip_leading+skip_trailing+align_right |bbbbbbbbbbb| -- 2.53.0
From: Peter Krempa <pkrempa@redhat.com> After recent addition of 'available' field the hardcoded alignments no longer match: $ virsh nodememstats total : 63393452 KiB free : 4046756 KiB available: 35747628 KiB buffers: 2291748 KiB cached : 24086464 KiB To address the issue switch to use dynamicaly aligned columns via vshTable infrastructure: $ virsh nodememstats total : 63393452 KiB free : 3888776 KiB available: 35640268 KiB buffers : 2291768 KiB cached : 24089916 KiB Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-host.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/tools/virsh-host.c b/tools/virsh-host.c index dd98917fa8..ef91e22fed 100644 --- a/tools/virsh-host.c +++ b/tools/virsh-host.c @@ -33,6 +33,7 @@ #include "virfile.h" #include "virenum.h" #include "virsh-util.h" +#include "vsh-table.h" /* * "capabilities" command @@ -889,6 +890,8 @@ cmdNodeMemStats(vshControl *ctl, const vshCmd *cmd) int cellNum = VIR_NODE_MEMORY_STATS_ALL_CELLS; g_autofree virNodeMemoryStatsPtr params = NULL; virshControl *priv = ctl->privData; + g_autoptr(vshTable) table = vshTableNew("field", ":", "value", NULL); + g_autofree char *tblstr = NULL; if (vshCommandOptInt(ctl, cmd, "cell", &cellNum) < 0) return false; @@ -912,8 +915,18 @@ cmdNodeMemStats(vshControl *ctl, const vshCmd *cmd) return false; } - for (i = 0; i < nparams; i++) - vshPrint(ctl, "%-7s: %20llu KiB\n", params[i].field, params[i].value); + for (i = 0; i < nparams; i++) { + g_autofree char *val = g_strdup_printf("%llu KiB", params[i].value); + + vshTableRowAppendFlags(table, + VSH_TABLE_CELL_SKIP_LEADING | VSH_TABLE_CELL_SKIP_TRAILING, params[i].field, + VSH_TABLE_CELL_SKIP_LEADING, ":", + VSH_TABLE_CELL_ALIGN_RIGHT, val, + 0, NULL); + } + + tblstr = vshTablePrintToString(table, false); + vshPrint(ctl, "%s", tblstr); return true; } -- 2.53.0
participants (2)
-
Ján Tomko -
Peter Krempa