[libvirt] [PATCH 1/2] Add API for printing tables.
Simon Kobyda
skobyda at redhat.com
Mon Aug 13 09:46:17 UTC 2018
On Fri, 2018-08-10 at 15:40 +0200, Michal Privoznik wrote:
> 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 at 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 at 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]
That way I will implement identical loop also there [1], to count
number of variable arguments to make that check (if there is better way
to count number of variable arguments, I'm open to suggestions :) ).
Sure I can drop it there, but what are pros of that? I'm curious :).
>
> > +{
> > + 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.
I had it the way you are suggesting before, but I inverted it because I
think the current state of code is more readable, compared to its
inverted state.
The length of code would be very similar, but perhaps it would be more
optimized? (we would have only 2 'for' loops instead of 4, however more
if-else statements) If that's the reason why are you suggesting to
invert it.
>
> > +
> > +}
> > +
> > +/**
> > + * 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?
You are right its useless in vshTablePrintToString function.
I'm also thinking about dropping ctl from this API completely, and pass
NULL to vshPrintExtra, like vshPrintExtra(NULL, "something to print").
Would that have any further consequences?
My knowledge of ctl structure and its use in printing is shallow.
I know only that ctl function in priting has affect on --quiet printing
(e.g. not priting table header), but that functionality could be
implemented other way. Does it have effect on anything else?
It would make API more universal, and quiet printing would not be
depended on ctl.
Simon.
>
> Michal
More information about the libvir-list
mailing list