[libvirt] [PATCH 1/2] Add API for printing tables.
Michal Privoznik
mprivozn at redhat.com
Fri Aug 10 13:40:02 UTC 2018
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]
> +{
> + 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
More information about the libvir-list
mailing list