On 08/10/2018 11:11 AM, Simon Kobyda wrote:
It solves problems with alignment of columns. Width of each column
is calculated by its biggest cell. Should solve unicode bug.
In future, it may be implemented in virsh, virt-admin...
This API has 5 public functions:
- vshTableNew - adds new table and defines its header
- vshTableRowAppend - appends new row (for same number of columns as in
header)
- vshTablePrintToStdout
- vshTablePrintToString
- vshTableFree
https://bugzilla.redhat.com/show_bug.cgi?id=1574624
https://bugzilla.redhat.com/show_bug.cgi?id=1584630
Signed-off-by: Simon Kobyda <skobyda(a)redhat.com>
---
tools/Makefile.am | 4 +-
tools/vsh-table.c | 367 ++++++++++++++++++++++++++++++++++++++++++++++
tools/vsh-table.h | 42 ++++++
3 files changed, 412 insertions(+), 1 deletion(-)
create mode 100644 tools/vsh-table.c
create mode 100644 tools/vsh-table.h
Missing cover letter. When sending more than one patch it's necessary.
Also the patch should have "vsh: " prefix so that it's obvious what part
of the ever growing source it's touching.
diff --git a/tools/Makefile.am b/tools/Makefile.am
index 26c887649e..ed23575aa7 100644
--- a/tools/Makefile.am
+++ b/tools/Makefile.am
@@ -144,7 +144,9 @@ libvirt_shell_la_LIBADD = \
$(READLINE_LIBS) \
../gnulib/lib/libgnu.la \
$(NULL)
-libvirt_shell_la_SOURCES = vsh.c vsh.h
+libvirt_shell_la_SOURCES = \
+ vsh.c vsh.h \
+ vsh-table.c vsh-table.h
virt_host_validate_SOURCES = \
virt-host-validate.c \
diff --git a/tools/vsh-table.c b/tools/vsh-table.c
new file mode 100644
index 0000000000..11dc807ba9
--- /dev/null
+++ b/tools/vsh-table.c
@@ -0,0 +1,367 @@
+/*
+ * vsh-table.c: table printing helper
+ *
+ * Copyright (C) 2018 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library. If not, see
+ * <
http://www.gnu.org/licenses/>.
+ *
+ * Authors:
+ * Simon Kobyda <skobyda(a)redhat.com>
+ *
+ */
+
+#include <config.h>
+#include "vsh-table.h"
+#include "virsh-util.h"
+
+#include <string.h>
+#include <stdarg.h>
+#include <stddef.h>
+
+#include "viralloc.h"
+#include "virbuffer.h"
+#include "virstring.h"
Nit pick, we tend to include system headers first and only after that
the local ones.
+
+typedef void (*vshPrintCB)(vshControl *ctl, const char *fmt, ...);
+
+struct _vshTableRow {
+ char **cells;
+ size_t ncells;
+};
+
+struct _vshTable {
+ vshTableRowPtr *rows;
+ size_t nrows;
+};
+
+static void
+vshTableRowFree(vshTableRowPtr row)
+{
+ size_t i;
+
+ if (!row)
+ return;
+
+ for (i = 0; i < row->ncells; i++)
+ VIR_FREE(row->cells[i]);
+
+ VIR_FREE(row->cells);
+ VIR_FREE(row);
+}
+
+void
+vshTableFree(vshTablePtr table)
+{
+ size_t i;
+
+ if (!table)
+ return;
+
+ for (i = 0; i < table->nrows; i++)
+ vshTableRowFree(table->rows[i]);
+ VIR_FREE(table->rows);
+ VIR_FREE(table);
+}
+
+/**
+ * vshTableRowNew:
+ * @size: expected number of cells in row.
+ * @arg: the first argument.
+ * @ap: list of variadic arguments
+ *
+ * Creates a new row in the table. Each argument passed
+ * represents a cell in the row. If the number of cells is not
+ * equal to @size, then NULL is returned. However, if @size is
+ * equal to 0, the aforementioned check is suppressed.
+ *
+ * Returns: pointer to vshTableRowPtr row or NULL.
+ */
+static vshTableRowPtr
+vshTableRowNew(size_t size, const char *arg, va_list ap)
I know we discussed this in person, but now that I see this code and
think about it more I think this @size argument should be dropped from
this function and the corresponding check should be moved to [1]
+{
+ vshTableRowPtr row = NULL;
+ char *tmp = NULL;
+
+ if (!arg) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Table row cannot be empty"));
+ goto error;
+ }
+
+ if (VIR_ALLOC(row) < 0)
+ goto error;
+
+ while (arg) {
+ if (VIR_STRDUP(tmp, arg) < 0)
+ goto error;
+
+ if (VIR_APPEND_ELEMENT(row->cells, row->ncells, tmp) < 0)
+ goto error;
+
+ arg = va_arg(ap, const char *);
+ }
+
+ if (size && row->ncells != size) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Incorrect number of cells in a table row"));
+ goto error;
+ }
+
+ return row;
+
+ error:
+ vshTableRowFree(row);
+ return NULL;
+}
+
+/**
+ * vshTableNew:
+ * @arg: List of column names (NULL terminated)
+ *
+ * Creates a new table.
+ *
+ * Returns: pointer to table or NULL.
+ */
+vshTablePtr
+vshTableNew(const char *arg, ...)
+{
+ vshTablePtr table;
+ vshTableRowPtr header = NULL;
+ va_list ap;
+
+ if (VIR_ALLOC(table) < 0)
+ goto error;
+
+ va_start(ap, arg);
+ header = vshTableRowNew(0, arg, ap);
+ va_end(ap);
+
+ if (!header)
+ goto error;
+
+ if (VIR_APPEND_ELEMENT(table->rows, table->nrows, header) < 0)
+ goto error;
+
+ return table;
+ error:
+ vshTableRowFree(header);
+ vshTableFree(table);
+ return NULL;
+}
+
+/**
+ * vshTableRowAppend:
+ * @table: table to append to
+ * @arg: cells of the row (NULL terminated)
+ *
+ * Appends 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.
+ *
+ * Returns: 0 if succeeded, -1 if failed.
+ */
+int
+vshTableRowAppend(vshTablePtr table, const char *arg, ...)
+{
+ vshTableRowPtr row = NULL;
+ size_t ncolumns = table->rows[0]->ncells;
+ va_list ap;
+ int ret = -1;
+
+ va_start(ap, arg);
+ row = vshTableRowNew(ncolumns, arg, ap);
+ va_end(ap);
+
+ if (!row)
+ goto cleanup;
1: here.
+
+ if (VIR_APPEND_ELEMENT(table->rows, table->nrows, row) < 0)
+ goto cleanup;
+
+ ret = 0;
+ cleanup:
+ vshTableRowFree(row);
+ return ret;
+}
+
+/**
+ * vshTableGetColumnsWidths:
+ * @table: table
+ * @maxwidths: maximum count of characters for each columns
+ * @widths: count of characters for each cell in the table
+ *
+ * Fills passed @maxwidths and @widths arrays with maximum number
+ * of characters for columns and number of character per each
+ * table cell, respectively.
+ *
+ * Tries to handle unicode strings.
+ */
+static void
+vshTableGetColumnsWidths(vshTablePtr table,
+ size_t **maxwidths,
+ size_t ***widths)
Too much pointers here. You are not allocating @maxwidths nor @widths.
They both can be one level less.
+{
+ size_t i;
+ size_t j;
+
+ for (i = 0; i < table->nrows; i++) {
+ vshTableRowPtr row = table->rows[i];
+
+ for (j = 0; j < row->ncells; j++) {
+ size_t len;
+
+ len = mbstowcs(NULL, row->cells[j], 0);
+
+ if (len == (size_t) -1)
+ len = strlen(row->cells[j]);
+
+ (*widths)[i][j] = len;
+ if (len > (*maxwidths)[j])
+ (*maxwidths)[j] = len;
+ }
+ }
+}
+
+/**
+ * vshTableRowPrint:
+ * @ctl virtshell control structure
+ * @row: table to append to
+ * @maxwidths: maximum count of characters for each columns
+ * @widths: count of character for each cell in this row
+ * @printCB function for priting table
+ * @buf: buffer to store table (only if @toStdout == true)
+ * @toStdout: whetever print table to Stdout or return in buffer
+ */
+static void
+vshTableRowPrint(vshControl *ctl,
+ vshTableRowPtr row,
+ size_t *maxwidths,
+ size_t *widths,
+ vshPrintCB printCB,
+ virBufferPtr buf,
+ bool toStdout)
+{
+ size_t i;
+ size_t j;
+
+ if (toStdout) {
+ for (i = 0; i < row->ncells; i++) {
+ printCB(ctl, " %s", row->cells[i]);
+
+ for (j = 0; j < maxwidths[i] - widths[i] + 2; j++)
+ printCB(ctl, " ");
+ }
+ printCB(ctl, "\n");
+ } else {
+ for (i = 0; i < row->ncells; i++) {
+ virBufferAsprintf(buf, " %s", row->cells[i]);
+
+ for (j = 0; j < maxwidths[i] - widths[i] + 2; j++)
+ virBufferAddStr(buf, " ");
+ }
+ virBufferAddStr(buf, "\n");
+ }
How about inverting these ifs and fors? I mean:
for () {
if (toStdout)
printCB()
else
virBufferAsprintf.
}
This suggestion applies to vshTablePrint too.
+
+}
+
+/**
+ * vshTablePrint:
+ * @ctl virtshell control structure
+ * @table: table to print
+ * @toStdout: whetever to print to stdout (true) or return in string (false)
+ *
+ * Prints table. To get an alignment of columns right, function
+ * fills 2d array @widths with count of characters in each cell and
+ * array @maxwidths maximum count of character in each column.
+ * Function then prints tables header and content.
+ *
+ * Returns string containing table, or NULL if table was printed to
+ * stdout
+ */
+static char *
+vshTablePrint(vshControl *ctl, vshTablePtr table, bool toStdout)
+{
+ size_t i;
+ size_t j;
+ size_t *maxwidths;
+ size_t **widths;
+ virBuffer buf = VIR_BUFFER_INITIALIZER;
+ char *ret = NULL;
+
+ if (VIR_ALLOC_N(maxwidths, table->rows[0]->ncells))
+ goto cleanup;
+
+ if (VIR_ALLOC_N(widths, table->nrows))
+ goto cleanup;
+
+ /* retrieve widths of columns */
+ for (i = 0; i < table->nrows; i++) {
+ if (VIR_ALLOC_N(widths[i], table->rows[0]->ncells))
+ goto cleanup;
+ }
+
+ vshTableGetColumnsWidths(table, &maxwidths, &widths);
+
+ /* print header */
+ VIR_WARNINGS_NO_PRINTF
+ vshTableRowPrint(ctl, table->rows[0], maxwidths, widths[0],
+ vshPrintExtra, &buf, toStdout);
+ VIR_WARNINGS_RESET
+
+ /* print dividing line */
+ if (toStdout) {
+ for (i = 0; i < table->rows[0]->ncells; i++) {
+ for (j = 0; j < maxwidths[i] + 3; j++)
+ vshPrintExtra(ctl, "-");
+ }
+ vshPrintExtra(ctl, "\n");
+ } else {
+ for (i = 0; i < table->rows[0]->ncells; i++) {
+ for (j = 0; j < maxwidths[i] + 3; j++)
+ virBufferAddStr(&buf, "-");
+ }
+ virBufferAddStr(&buf, "\n");
+ }
+
+ /* print content */
+ for (i = 1; i < table->nrows; i++) {
+ VIR_WARNINGS_NO_PRINTF
+ vshTableRowPrint(ctl, table->rows[i], maxwidths, widths[0],
Ooops. s/0/i/ ;-)
+ vshPrintExtra, &buf, toStdout);
+ VIR_WARNINGS_RESET
+ }
+
+ if (!toStdout)
+ ret = virBufferContentAndReset(&buf);
+
+ cleanup:
+ VIR_FREE(maxwidths);
+ for (i = 0; i < table->nrows; i++)
+ VIR_FREE(widths[i]);
+ VIR_FREE(widths);
+ return ret;
+}
+
+
+void
+vshTablePrintToStdout(vshControl *ctl, vshTablePtr table)
+{
+ vshTablePrint(ctl, table, true);
+}
+
+char *
+vshTablePrintToString(vshControl *ctl, vshTablePtr table)
+{
+ return vshTablePrint(ctl, table, false);
+}
Why does this function require @ctl?
Michal