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@xxxxxxxxxx> > --- > 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@xxxxxxxxxx> > + * > + */ > + > +#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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
