[libvirt] [PATCH 1/2] Add API for printing tables.
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@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 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@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" + +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) +{ + 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; + + 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) +{ + 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"); + } + +} + +/** + * 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], + 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); +} diff --git a/tools/vsh-table.h b/tools/vsh-table.h new file mode 100644 index 0000000000..41600fe06f --- /dev/null +++ b/tools/vsh-table.h @@ -0,0 +1,42 @@ +/* + * vsh-table.h: 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@redhat.com> + * + */ + +#ifndef VSH_TABLE_H +# define VSH_TABLE_H + +# include "vsh.h" + +/* forward declarations */ +typedef struct _vshTable vshTable; +typedef struct _vshTableRow vshTableRow; +typedef vshTable *vshTablePtr; +typedef vshTableRow *vshTableRowPtr; + +void vshTableFree(vshTablePtr table); +vshTablePtr vshTableNew(const char *format, ...); +int vshTableRowAppend(vshTablePtr table, const char *arg, ...); +void vshTablePrintToStdout(vshControl *ctl, vshTablePtr table); +char *vshTablePrintToString(vshControl *ctl, vshTablePtr table); + +#endif /* VSH_TABLE_H */ -- 2.17.1
Instead of printing it straight in virsh, it creates table struct which is filled with header and rows(domains). It allows us to know more about table before printing to calculate alignment right. Signed-off-by: Simon Kobyda <skobyda@redhat.com> --- tools/virsh-domain-monitor.c | 39 ++++++++++++++++++------------------ 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index b9b4f9739b..f6842877f5 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -39,6 +39,7 @@ #include "virmacaddr.h" #include "virxml.h" #include "virstring.h" +#include "vsh-table.h" VIR_ENUM_DECL(virshDomainIOError) VIR_ENUM_IMPL(virshDomainIOError, @@ -1901,6 +1902,7 @@ cmdList(vshControl *ctl, const vshCmd *cmd) char id_buf[INT_BUFSIZE_BOUND(unsigned int)]; unsigned int id; unsigned int flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE; + vshTablePtr table = NULL; /* construct filter flags */ if (vshCommandOptBool(cmd, "inactive") || @@ -1940,15 +1942,12 @@ cmdList(vshControl *ctl, const vshCmd *cmd) /* print table header in legacy mode */ if (optTable) { if (optTitle) - vshPrintExtra(ctl, " %-5s %-30s %-10s %-20s\n%s\n", - _("Id"), _("Name"), _("State"), _("Title"), - "-----------------------------------------" - "-----------------------------------------"); + table = vshTableNew("Id", "Name", "State", "Title", NULL); else - vshPrintExtra(ctl, " %-5s %-30s %s\n%s\n", - _("Id"), _("Name"), _("State"), - "-----------------------------------------" - "-----------"); + table = vshTableNew("Id", "Name", "State", NULL); + + if (!table) + goto cleanup; } for (i = 0; i < list->ndomains; i++) { @@ -1973,20 +1972,18 @@ cmdList(vshControl *ctl, const vshCmd *cmd) if (optTitle) { if (!(title = virshGetDomainDescription(ctl, dom, true, 0))) goto cleanup; - - vshPrint(ctl, " %-5s %-30s %-10s %-20s\n", id_buf, - virDomainGetName(dom), - state == -2 ? _("saved") - : virshDomainStateToString(state), - title); - + vshTableRowAppend(table, id_buf, virDomainGetName(dom), + state == -2 ? _("saved") + : virshDomainStateToString(state), + title, NULL); VIR_FREE(title); } else { - vshPrint(ctl, " %-5s %-30s %s\n", id_buf, - virDomainGetName(dom), - state == -2 ? _("saved") - : virshDomainStateToString(state)); + vshTableRowAppend(table, id_buf, virDomainGetName(dom), + state == -2 ? _("saved") + : virshDomainStateToString(state), + NULL); } + } else if (optUUID && optName) { if (virDomainGetUUIDString(dom, uuid) < 0) { vshError(ctl, "%s", _("Failed to get domain's UUID")); @@ -2004,8 +2001,12 @@ cmdList(vshControl *ctl, const vshCmd *cmd) } } + if (optTable) + vshTablePrintToStdout(ctl, table); + ret = true; cleanup: + vshTableFree(table); virshDomainListFree(list); return ret; } -- 2.17.1
On 08/10/2018 11:11 AM, Simon Kobyda wrote:
Instead of printing it straight in virsh, it creates table struct which is filled with header and rows(domains). It allows us to know more about table before printing to calculate alignment right.
Signed-off-by: Simon Kobyda <skobyda@redhat.com> --- tools/virsh-domain-monitor.c | 39 ++++++++++++++++++------------------ 1 file changed, 20 insertions(+), 19 deletions(-)
diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index b9b4f9739b..f6842877f5 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -39,6 +39,7 @@ #include "virmacaddr.h" #include "virxml.h" #include "virstring.h" +#include "vsh-table.h"
VIR_ENUM_DECL(virshDomainIOError) VIR_ENUM_IMPL(virshDomainIOError, @@ -1901,6 +1902,7 @@ cmdList(vshControl *ctl, const vshCmd *cmd) char id_buf[INT_BUFSIZE_BOUND(unsigned int)]; unsigned int id; unsigned int flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE; + vshTablePtr table = NULL;
/* construct filter flags */ if (vshCommandOptBool(cmd, "inactive") || @@ -1940,15 +1942,12 @@ cmdList(vshControl *ctl, const vshCmd *cmd) /* print table header in legacy mode */ if (optTable) { if (optTitle) - vshPrintExtra(ctl, " %-5s %-30s %-10s %-20s\n%s\n", - _("Id"), _("Name"), _("State"), _("Title"), - "-----------------------------------------" - "-----------------------------------------"); + table = vshTableNew("Id", "Name", "State", "Title", NULL); else - vshPrintExtra(ctl, " %-5s %-30s %s\n%s\n", - _("Id"), _("Name"), _("State"), - "-----------------------------------------" - "-----------"); + table = vshTableNew("Id", "Name", "State", NULL); + + if (!table) + goto cleanup; }
for (i = 0; i < list->ndomains; i++) { @@ -1973,20 +1972,18 @@ cmdList(vshControl *ctl, const vshCmd *cmd) if (optTitle) { if (!(title = virshGetDomainDescription(ctl, dom, true, 0))) goto cleanup; - - vshPrint(ctl, " %-5s %-30s %-10s %-20s\n", id_buf, - virDomainGetName(dom), - state == -2 ? _("saved") - : virshDomainStateToString(state), - title); - + vshTableRowAppend(table, id_buf, virDomainGetName(dom), + state == -2 ? _("saved") + : virshDomainStateToString(state), + title, NULL);
You need to check for vshTableRowAppend() retval here.
VIR_FREE(title); } else { - vshPrint(ctl, " %-5s %-30s %s\n", id_buf, - virDomainGetName(dom), - state == -2 ? _("saved") - : virshDomainStateToString(state)); + vshTableRowAppend(table, id_buf, virDomainGetName(dom), + state == -2 ? _("saved") + : virshDomainStateToString(state), + NULL);
Another approach might be to initialize title to NULL, and then set it iff optTitle and then call: vshTableRowAppend(table, id_buf, virDomainGetName(dom), state == -2 ? _("saved") : virshDomainStateToString(state), title, NULL); Anyway, the rest looks good. Michal
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@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@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
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@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@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
On 08/13/2018 11:46 AM, Simon Kobyda wrote:
On Fri, 2018-08-10 at 15:40 +0200, Michal Privoznik wrote:
On 08/10/2018 11:11 AM, Simon Kobyda wrote:
+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 :).
I'm not sure why you would need the second loop? The way your code works right now is that vshTableRowNew must have the sentinel (= args have to end with NULL). What I am suggesting is for vshTableRowNew() to just consume all the args (like it is doing now) until the sentinel. And only after that check at [1] whether row->ncells is expected value.
+{ + 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; +} +
+ +/** + * 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.
Well, the reason is that if we ever need to change the way we're iterating (e.g. call some function at the end of each loop) we can do it in one change: for () { if (toStdout) printCB() else virBufferAsprintf() + newFunction(); } Michal
+ +} + +/** + * 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").
I'm failing to see where would vshPrintExtra() be called with NULL. The point is that if you're printing to string virBuffer*() APIs are called.
Would that have any further consequences?
Yes, vshPrintExtra(NULL, ...) will print to stdout regardless of user requesting quiet output. For instance, `virsh -q list --all` doesn't print the table header, because it's printed with vshPrintExtra() which checks for '-q' and therefore doesn't print anything. However, that's not true if passed ctl is NULL.
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.
I'm not quite sure how you want to achieve that. So currently when -q is passed, no table header is printed. If you don't pass that info to vshTablePrint*() then how is it supposed to know what to print? Michal
On Mon, 2018-08-13 at 12:01 +0200, Michal Prívozník wrote:
On 08/13/2018 11:46 AM, Simon Kobyda wrote:
On Fri, 2018-08-10 at 15:40 +0200, Michal Privoznik wrote:
On 08/10/2018 11:11 AM, Simon Kobyda wrote:
+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 :).
I'm not sure why you would need the second loop? The way your code works right now is that vshTableRowNew must have the sentinel (= args have to end with NULL). What I am suggesting is for vshTableRowNew() to just consume all the args (like it is doing now) until the sentinel. And only after that check at [1] whether row->ncells is expected value.
I see, I misunderstood. That's also an option.
+{ + 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; +} +
+ +/** + * 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.
Well, the reason is that if we ever need to change the way we're iterating (e.g. call some function at the end of each loop) we can do it in one change:
for () { if (toStdout) printCB() else virBufferAsprintf()
+ newFunction(); }
Michal
+ +} + +/** + * 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").
I'm failing to see where would vshPrintExtra() be called with NULL. The point is that if you're printing to string virBuffer*() APIs are called.
I'm talking about getting rid of ctl also in printing to terminal.
Would that have any further consequences?
Yes, vshPrintExtra(NULL, ...) will print to stdout regardless of user requesting quiet output. For instance, `virsh -q list --all` doesn't print the table header, because it's printed with vshPrintExtra() which checks for '-q' and therefore doesn't print anything. However, that's not true if passed ctl is NULL.
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.
I'm not quite sure how you want to achieve that. So currently when -q is passed, no table header is printed. If you don't pass that info to vshTablePrint*() then how is it supposed to know what to print?
I could have it as an argument for vshPrintToTerminal and vshPrintToString (perhaps some bool whetever to print header or not, or some flags). And according to value of that bool, priting of header would or wouldn't commence. (Whole block of code which takes care of priting header would be in some if-statement or something). My point is that it would make whole table API priting more universal, so it could be called from place where there is no vshControl variable, and user still could "turn off" header even without vshControl variable. Therefore I'm suggesting to get rid of vshControl ctl variable, and for priting without header, let's use some bool or something. Simon.
participants (3)
-
Michal Privoznik -
Michal Prívozník
-
Simon Kobyda