[libvirt] [PATCH v3 0/3] vsh: Introduce new API for printing tables
Created new API for priting tables, mainly to solve alignment problems. Implemented these test to virsh list. In the future, API may be everywhere in virsh and virt-admin. Also wrote basic tests for the new API, and corrected tests in virshtest which are influenced by implementation of the API in virsh list. Changes in v3: - changed encoding of 3/3 patch, otherwise it cannot be applied Changes in v2: - added tests - fixed alignment for unicode character which span more spaces - moved ncolumns check to vshTableRowAppend - changed arguments for functions vshTablePrint, vshTablePrintToStdout, vshTablePrintToString Simon Kobyda (3): vsh: Add API for printing tables. virsh: Implement new table API for virsh list vsh: Added tests tests/Makefile.am | 8 + tests/virshtest.c | 14 +- tests/vshtabletest.c | 247 +++++++++++++++++++++ tools/Makefile.am | 4 +- tools/virsh-domain-monitor.c | 43 ++-- tools/vsh-table.c | 413 +++++++++++++++++++++++++++++++++++ tools/vsh-table.h | 42 ++++ 7 files changed, 744 insertions(+), 27 deletions(-) create mode 100644 tests/vshtabletest.c create mode 100644 tools/vsh-table.c create mode 100644 tools/vsh-table.h -- 2.17.1
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 | 413 ++++++++++++++++++++++++++++++++++++++++++++++ tools/vsh-table.h | 42 +++++ 3 files changed, 458 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 1452d984a0..f069167acc 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..8842e4e4fd --- /dev/null +++ b/tools/vsh-table.c @@ -0,0 +1,413 @@ +/* + * 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 <string.h> +#include <stdarg.h> +#include <stddef.h> +#include <wchar.h> +#include <locale.h> + +#include "viralloc.h" +#include "virbuffer.h" +#include "virstring.h" +#include "virsh-util.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: + * @arg: the first argument. + * @ap: list of variadic arguments + * + * Create a new row in the table. Each argument passed + * represents a cell in the row. + * Return: pointer to vshTableRowPtr row or NULL. + */ +static vshTableRowPtr +vshTableRowNew(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 *); + } + + return row; + + error: + vshTableRowFree(row); + return NULL; +} + +/** + * vshTableNew: + * @arg: List of column names (NULL terminated) + * + * Create 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(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) + * + * Append 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(arg, ap); + va_end(ap); + + if (!row) + goto cleanup; + + if (ncolumns != row->ncells) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Incorrect number of cells in a table 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 + * + * Fill passed @maxwidths and @widths arrays with maximum number + * of characters for columns and number of character per each + * table cell, respectively. + * + * Handle unicode strings (user must have multibyte locale) + */ +static int +vshTableGetColumnsWidths(vshTablePtr table, + size_t *maxwidths, + size_t **widths, + bool header) +{ + int ret = -1; + size_t i = 1; + size_t j; + size_t len; + int tmp; + wchar_t *wstr = NULL; + size_t wstrlen; + + if (header) + i = 0; + else + i = 1; + for (; i < table->nrows; i++) { + vshTableRowPtr row = table->rows[i]; + + for (j = 0; j < row->ncells; j++) { + /* strlen should return maximum possible length needed */ + wstrlen = strlen(row->cells[j]); + VIR_FREE(wstr); + if (VIR_ALLOC_N(wstr, wstrlen) < 0) + goto cleanup; + /* mbstowcs fails if machine is using singlebyte locale + * and user tries to convert unicode(multibyte) + * */ + if (mbstowcs(wstr, row->cells[j], wstrlen) == + (size_t) -1) { + len = wstrlen; + } else { + tmp = wcswidth(wstr, wstrlen); + if (tmp < 0) + goto cleanup; + len = (size_t)((unsigned)tmp); + } + widths[i][j] = len; + if (len > maxwidths[j]) + maxwidths[j] = len; + } + } + + ret = 0; + cleanup: + VIR_FREE(wstr); + return ret; +} + +/** + * 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; + + for (i = 0; i < row->ncells; i++) { + if (toStdout) + printCB(ctl, " %s", row->cells[i]); + else + virBufferAsprintf(buf, " %s", row->cells[i]); + + for (j = 0; j < maxwidths[i] - widths[i] + 2; j++) { + if (toStdout) + printCB(ctl, " "); + else + virBufferAddStr(buf, " "); + } + } + if (toStdout) + printCB(ctl, "\n"); + else + virBufferAddStr(buf, "\n"); +} + +/** + * vshTablePrint: + * @ctl virtshell control structure + * @table: table to print + * @header: whetever to print to header (true) or not (false) + * this argument is relevant only if @ctl == NULL + * @toStdout: whetever to print to stdout (true) or return in string + * + * Print 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. + * + * Return string containing table, or NULL if table was printed to + * stdout + */ +static char * +vshTablePrint(vshControl *ctl, vshTablePtr table, bool header, 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; + } + + if (vshTableGetColumnsWidths(table, maxwidths, widths, header) < 0) + goto cleanup; + + if (header) { + /* print header */ + VIR_WARNINGS_NO_PRINTF + vshTableRowPrint(ctl, table->rows[0], maxwidths, widths[0], + vshPrintExtra, &buf, toStdout); + VIR_WARNINGS_RESET + + /* print dividing line */ + for (i = 0; i < table->rows[0]->ncells; i++) { + for (j = 0; j < maxwidths[i] + 3; j++) { + if (toStdout) + vshPrintExtra(ctl, "-"); + else + virBufferAddStr(&buf, "-"); + } + } + if (toStdout) + vshPrintExtra(ctl, "\n"); + else + virBufferAddStr(&buf, "\n"); + } + /* print content */ + for (i = 1; i < table->nrows; i++) { + VIR_WARNINGS_NO_PRINTF + vshTableRowPrint(ctl, table->rows[i], maxwidths, widths[i], + vshPrint, &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; +} + + +/** + * vshTablePrintToStdout: + * @table: table to print + * @ctl virtshell control structure + * + * Print table to stdout. + * + */ +void +vshTablePrintToStdout(vshTablePtr table, vshControl *ctl) +{ + bool header; + if (ctl) + header = !ctl->quiet; + else + header = true; + vshTablePrint(ctl, table, header, true); +} + +/** + * vshTablePrintToString: + * @table: table to print + * @header: whetever to print to header (true) or not (false) + * + * Return string containing table, or NULL if table was printed to + * stdout. User will have to free returned string. + */ +char * +vshTablePrintToString(vshTablePtr table, bool header) +{ + return vshTablePrint(NULL, table, header, false); +} diff --git a/tools/vsh-table.h b/tools/vsh-table.h new file mode 100644 index 0000000000..e4e9582b9f --- /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(vshTablePtr table, vshControl *ctl); +char *vshTablePrintToString(vshTablePtr table, bool header); + +#endif /* VSH_TABLE_H */ -- 2.17.1
On Thu, Aug 16, 2018 at 12:56:24PM +0200, 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 | 413 ++++++++++++++++++++++++++++++++++++++++++++++ tools/vsh-table.h | 42 +++++ 3 files changed, 458 insertions(+), 1 deletion(-) create mode 100644 tools/vsh-table.c create mode 100644 tools/vsh-table.h
+/** + * vshTableGetColumnsWidths: + * @table: table + * @maxwidths: maximum count of characters for each columns + * @widths: count of characters for each cell in the table + * + * Fill passed @maxwidths and @widths arrays with maximum number + * of characters for columns and number of character per each + * table cell, respectively. + * + * Handle unicode strings (user must have multibyte locale) + */ +static int +vshTableGetColumnsWidths(vshTablePtr table, + size_t *maxwidths, + size_t **widths, + bool header) +{ + int ret = -1; + size_t i = 1; + size_t j; + size_t len; + int tmp; + wchar_t *wstr = NULL; + size_t wstrlen; + + if (header) + i = 0; + else + i = 1; + for (; i < table->nrows; i++) { + vshTableRowPtr row = table->rows[i]; + + for (j = 0; j < row->ncells; j++) { + /* strlen should return maximum possible length needed */ + wstrlen = strlen(row->cells[j]); + VIR_FREE(wstr); + if (VIR_ALLOC_N(wstr, wstrlen) < 0) + goto cleanup; + /* mbstowcs fails if machine is using singlebyte locale + * and user tries to convert unicode(multibyte) + * */ + if (mbstowcs(wstr, row->cells[j], wstrlen) == + (size_t) -1) { + len = wstrlen; + } else { + tmp = wcswidth(wstr, wstrlen); + if (tmp < 0) + goto cleanup; + len = (size_t)((unsigned)tmp); + } + widths[i][j] = len; + if (len > maxwidths[j]) + maxwidths[j] = len;
After asking around I have found the right solution that we need to use for measuring string width. mbstowcs()/wcswidth() will get the answer wrong wrt zero-width characters, combining characters, non-printable characters, etc. We need to use the libunistring library: https://www.gnu.org/software/libunistring/manual/libunistring.html#uniwidth_... Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Thu, 2018-08-16 at 12:28 +0100, Daniel P. Berrangé wrote:
On Thu, Aug 16, 2018 at 12:56:24PM +0200, Simon Kobyda wrote:
After asking around I have found the right solution that we need to use for measuring string width. mbstowcs()/wcswidth() will get the answer wrong wrt zero-width characters, combining characters, non-printable characters, etc. We need to use the libunistring library:
https://www.gnu.org/software/libunistring/manual/libunistring.html#uniwidth_...
I've tried what you've suggested, but it seems that it doesn't work well with all unicode characters. I'm looking into the code of the library, and each function uN_strwidth calls function uN_width, and that function calls uc_width for calculation of width of characters. And if we look into the code of uc_width here: http://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blob;f=lib/uniwidth/width... it seems that this library is limited only to certain unicodes, e.g.: hangul characters, angle brackets, CJK characters... But it doesn't cover all multiple-width characters. Example: I try to throw any emoji (e.g. 🙉, 🦀, 🏙), it returns width of 1 column for each charact er, nevertheless these characters have width of 2 columns on terminal. BTW, it seems unistring library imports those funcions from gnulib. Simon Kobyda.
On 08/21/2018 11:18 AM, Simon Kobyda wrote:
On Thu, 2018-08-16 at 12:28 +0100, Daniel P. Berrangé wrote:
On Thu, Aug 16, 2018 at 12:56:24PM +0200, Simon Kobyda wrote:
After asking around I have found the right solution that we need to use for measuring string width. mbstowcs()/wcswidth() will get the answer wrong wrt zero-width characters, combining characters, non-printable characters, etc. We need to use the libunistring library:
https://www.gnu.org/software/libunistring/manual/libunistring.html#uniwidth_...
I've tried what you've suggested, but it seems that it doesn't work well with all unicode characters. I'm looking into the code of the library, and each function uN_strwidth calls function uN_width, and that function calls uc_width for calculation of width of characters. And if we look into the code of uc_width here:
http://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blob;f=lib/uniwidth/width... it seems that this library is limited only to certain unicodes, e.g.: hangul characters, angle brackets, CJK characters... But it doesn't cover all multiple-width characters. Example: I try to throw any emoji (e.g. 🙉, 🦀, 🏙), it returns width of 1 column for each charact er, nevertheless these characters have width of 2 columns on terminal.
BTW, it seems unistring library imports those funcions from gnulib.
I guess the only option then is to try smartcols [1]. If it is good for util-linux it's going to be good for us too. Although, I'd prefer to have our own wrappers over their API. https://github.com/karelzak/util-linux/tree/master/libsmartcols Michal
On Tue, Aug 21, 2018 at 12:27:34PM +0200, Michal Privoznik wrote:
On 08/21/2018 11:18 AM, Simon Kobyda wrote:
On Thu, 2018-08-16 at 12:28 +0100, Daniel P. Berrangé wrote:
On Thu, Aug 16, 2018 at 12:56:24PM +0200, Simon Kobyda wrote:
After asking around I have found the right solution that we need to use for measuring string width. mbstowcs()/wcswidth() will get the answer wrong wrt zero-width characters, combining characters, non-printable characters, etc. We need to use the libunistring library:
https://www.gnu.org/software/libunistring/manual/libunistring.html#uniwidth_...
I've tried what you've suggested, but it seems that it doesn't work well with all unicode characters. I'm looking into the code of the library, and each function uN_strwidth calls function uN_width, and that function calls uc_width for calculation of width of characters. And if we look into the code of uc_width here:
http://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blob;f=lib/uniwidth/width... it seems that this library is limited only to certain unicodes, e.g.: hangul characters, angle brackets, CJK characters... But it doesn't cover all multiple-width characters. Example: I try to throw any emoji (e.g. 🙉, 🦀, 🏙), it returns width of 1 column for each charact er, nevertheless these characters have width of 2 columns on terminal.
BTW, it seems unistring library imports those funcions from gnulib.
I guess the only option then is to try smartcols [1]. If it is good for util-linux it's going to be good for us too. Although, I'd prefer to have our own wrappers over their API.
https://github.com/karelzak/util-linux/tree/master/libsmartcols
The util-linux code does something that uses mbstowcs / wcwidth to convert the characters and count their width, sort of like the original version of this patch. They have further code that decides to convert certain unicode characters into "\xNN" escaped sequences, which avoids the problems I raised wrt non-printable strings. https://github.com/karelzak/util-linux/blob/master/lib/mbsalign.c So we could pull that helper API into our code, since its LGPL loicensed. I'm unclear if this correctly handles all the cases or not though as there's no unit tests for it in util-linux AFACT. Really the only way for us to be sure is to provide a unit test which stresses our the code with a variety of unicode input strings. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Tue, 2018-08-21 at 11:46 +0100, Daniel P. Berrangé wrote:
On Tue, Aug 21, 2018 at 12:27:34PM +0200, Michal Privoznik wrote:
On 08/21/2018 11:18 AM, Simon Kobyda wrote:
On Thu, 2018-08-16 at 12:28 +0100, Daniel P. Berrangé wrote:
On Thu, Aug 16, 2018 at 12:56:24PM +0200, Simon Kobyda wrote:
After asking around I have found the right solution that we need to use for measuring string width. mbstowcs()/wcswidth() will get the answer wrong wrt zero-width characters, combining characters, non- printable characters, etc. We need to use the libunistring library:
https://www.gnu.org/software/libunistring/manual/libunistring.html#uniwidth_...
I've tried what you've suggested, but it seems that it doesn't work well with all unicode characters. I'm looking into the code of the library, and each function uN_strwidth calls function uN_width, and that function calls uc_width for calculation of width of characters. And if we look into the code of uc_width here:
http://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blob;f=lib/uniwidth/width...
it seems that this library is limited only to certain unicodes, e.g.: hangul characters, angle brackets, CJK characters... But it doesn't cover all multiple-width characters. Example: I try to throw any emoji (e.g. 🙉, 🦀, 🏙), it returns width of 1 column for each charact er, nevertheless these characters have width of 2 columns on terminal.
BTW, it seems unistring library imports those funcions from gnulib.
I guess the only option then is to try smartcols [1]. If it is good for util-linux it's going to be good for us too. Although, I'd prefer to have our own wrappers over their API.
https://github.com/karelzak/util-linux/tree/master/libsmartcols
The util-linux code does something that uses mbstowcs / wcwidth to convert the characters and count their width, sort of like the original version of this patch. They have further code that decides to convert certain unicode characters into "\xNN" escaped sequences, which avoids the problems I raised wrt non-printable strings.
https://github.com/karelzak/util-linux/blob/master/lib/mbsalign.c
So we could pull that helper API into our code, since its LGPL loicensed. I'm unclear if this correctly handles all the cases or not though as there's no unit tests for it in util-linux AFACT.
Really the only way for us to be sure is to provide a unit test which stresses our the code with a variety of unicode input strings.
About unit tests. Right now i've got tests for non-pritnable, zero- width, combining characters and opposite (rigth to left) writing. Anybody got any idea what else could be problematic with mbstowcs()/wcswidth(), and therefore tested? Simon Kobyda.
On Wed, Aug 22, 2018 at 12:50:12PM +0200, Simon Kobyda wrote:
On Tue, 2018-08-21 at 11:46 +0100, Daniel P. Berrangé wrote:
On Tue, Aug 21, 2018 at 12:27:34PM +0200, Michal Privoznik wrote:
On 08/21/2018 11:18 AM, Simon Kobyda wrote:
On Thu, 2018-08-16 at 12:28 +0100, Daniel P. Berrangé wrote:
On Thu, Aug 16, 2018 at 12:56:24PM +0200, Simon Kobyda wrote:
After asking around I have found the right solution that we need to use for measuring string width. mbstowcs()/wcswidth() will get the answer wrong wrt zero-width characters, combining characters, non- printable characters, etc. We need to use the libunistring library:
https://www.gnu.org/software/libunistring/manual/libunistring.html#uniwidth_...
I've tried what you've suggested, but it seems that it doesn't work well with all unicode characters. I'm looking into the code of the library, and each function uN_strwidth calls function uN_width, and that function calls uc_width for calculation of width of characters. And if we look into the code of uc_width here:
http://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blob;f=lib/uniwidth/width...
it seems that this library is limited only to certain unicodes, e.g.: hangul characters, angle brackets, CJK characters... But it doesn't cover all multiple-width characters. Example: I try to throw any emoji (e.g. 🙉, 🦀, 🏙), it returns width of 1 column for each charact er, nevertheless these characters have width of 2 columns on terminal.
BTW, it seems unistring library imports those funcions from gnulib.
I guess the only option then is to try smartcols [1]. If it is good for util-linux it's going to be good for us too. Although, I'd prefer to have our own wrappers over their API.
https://github.com/karelzak/util-linux/tree/master/libsmartcols
The util-linux code does something that uses mbstowcs / wcwidth to convert the characters and count their width, sort of like the original version of this patch. They have further code that decides to convert certain unicode characters into "\xNN" escaped sequences, which avoids the problems I raised wrt non-printable strings.
https://github.com/karelzak/util-linux/blob/master/lib/mbsalign.c
So we could pull that helper API into our code, since its LGPL loicensed. I'm unclear if this correctly handles all the cases or not though as there's no unit tests for it in util-linux AFACT.
Really the only way for us to be sure is to provide a unit test which stresses our the code with a variety of unicode input strings.
About unit tests. Right now i've got tests for non-pritnable, zero- width, combining characters and opposite (rigth to left) writing. Anybody got any idea what else could be problematic with mbstowcs()/wcswidth(), and therefore tested?
I think that sounds reasonable enough for now - passing such tests would already be massively better than the code that exists today with strlen() Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Thu, Aug 16, 2018 at 12:56:24PM +0200, 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 | 413 ++++++++++++++++++++++++++++++++++++++++++++++ tools/vsh-table.h | 42 +++++ 3 files changed, 458 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 1452d984a0..f069167acc 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..8842e4e4fd --- /dev/null +++ b/tools/vsh-table.c @@ -0,0 +1,413 @@ +/* + * 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 <string.h> +#include <stdarg.h> +#include <stddef.h> +#include <wchar.h> +#include <locale.h> + +#include "viralloc.h" +#include "virbuffer.h" +#include "virstring.h" +#include "virsh-util.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: + * @arg: the first argument. + * @ap: list of variadic arguments + * + * Create a new row in the table. Each argument passed + * represents a cell in the row. + * Return: pointer to vshTableRowPtr row or NULL. + */ +static vshTableRowPtr +vshTableRowNew(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 *); + } + + return row; + + error: + vshTableRowFree(row); + return NULL; +} + +/** + * vshTableNew: + * @arg: List of column names (NULL terminated) + * + * Create 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(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) + * + * Append 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(arg, ap); + va_end(ap); + + if (!row) + goto cleanup; + + if (ncolumns != row->ncells) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Incorrect number of cells in a table 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 + * + * Fill passed @maxwidths and @widths arrays with maximum number + * of characters for columns and number of character per each + * table cell, respectively. + * + * Handle unicode strings (user must have multibyte locale) + */ +static int +vshTableGetColumnsWidths(vshTablePtr table, + size_t *maxwidths, + size_t **widths, + bool header) +{ + int ret = -1; + size_t i = 1; + size_t j; + size_t len; + int tmp; + wchar_t *wstr = NULL; + size_t wstrlen; + + if (header) + i = 0; + else + i = 1; + for (; i < table->nrows; i++) { + vshTableRowPtr row = table->rows[i]; + + for (j = 0; j < row->ncells; j++) {
+ /* strlen should return maximum possible length needed */ + wstrlen = strlen(row->cells[j]); + VIR_FREE(wstr); + if (VIR_ALLOC_N(wstr, wstrlen) < 0) + goto cleanup; + /* mbstowcs fails if machine is using singlebyte locale + * and user tries to convert unicode(multibyte) + * */ + if (mbstowcs(wstr, row->cells[j], wstrlen) == + (size_t) -1) { + len = wstrlen; + } else { + tmp = wcswidth(wstr, wstrlen); + if (tmp < 0) + goto cleanup; + len = (size_t)((unsigned)tmp); + }
Whatever solution you use for converting a multi-byte string to a maximum count of characters, please put it in a separate function. That would make this function more readable.
+ widths[i][j] = len; + if (len > maxwidths[j]) + maxwidths[j] = len; + } + } + + ret = 0; + cleanup: + VIR_FREE(wstr); + return ret; +} + +/** + * 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; + + for (i = 0; i < row->ncells; i++) { + if (toStdout) + printCB(ctl, " %s", row->cells[i]); + else + virBufferAsprintf(buf, " %s", row->cells[i]); + + for (j = 0; j < maxwidths[i] - widths[i] + 2; j++) { + if (toStdout) + printCB(ctl, " "); + else + virBufferAddStr(buf, " "); + } + } + if (toStdout) + printCB(ctl, "\n"); + else + virBufferAddStr(buf, "\n"); +} + +/** + * vshTablePrint: + * @ctl virtshell control structure + * @table: table to print + * @header: whetever to print to header (true) or not (false) + * this argument is relevant only if @ctl == NULL + * @toStdout: whetever to print to stdout (true) or return in string + * + * Print 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. + * + * Return string containing table, or NULL if table was printed to + * stdout + */ +static char * +vshTablePrint(vshControl *ctl, vshTablePtr table, bool header, bool toStdout)
Rather than having both 'header' and 'toStdout' arguments, I think having only 'header' and returning the result in a virBuffer would be nicer than having if (toStdout) before every single print...
+{ + 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; + } + + if (vshTableGetColumnsWidths(table, maxwidths, widths, header) < 0) + goto cleanup; + + if (header) { + /* print header */ + VIR_WARNINGS_NO_PRINTF + vshTableRowPrint(ctl, table->rows[0], maxwidths, widths[0], + vshPrintExtra, &buf, toStdout); + VIR_WARNINGS_RESET + + /* print dividing line */ + for (i = 0; i < table->rows[0]->ncells; i++) { + for (j = 0; j < maxwidths[i] + 3; j++) { + if (toStdout) + vshPrintExtra(ctl, "-"); + else + virBufferAddStr(&buf, "-"); + } + } + if (toStdout) + vshPrintExtra(ctl, "\n"); + else + virBufferAddStr(&buf, "\n"); + } + /* print content */ + for (i = 1; i < table->nrows; i++) { + VIR_WARNINGS_NO_PRINTF + vshTableRowPrint(ctl, table->rows[i], maxwidths, widths[i], + vshPrint, &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; +} + + +/** + * vshTablePrintToStdout: + * @table: table to print + * @ctl virtshell control structure + * + * Print table to stdout. + * + */ +void +vshTablePrintToStdout(vshTablePtr table, vshControl *ctl) +{ + bool header; + if (ctl) + header = !ctl->quiet; + else + header = true;
... here you alread set header based on ctl->quiet. Since vshPrint and vshPrintExtra differ only in whether the extra stuff gets printed, calling vshTablePrint to get the string, then vshPrint on that would achieve the same result. (at the expense of extra string copy, which should not matter in virsh). Jano
On Thu, Aug 16, 2018 at 02:05:45PM +0200, Ján Tomko wrote:
On Thu, Aug 16, 2018 at 12:56:24PM +0200, 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 | 413 ++++++++++++++++++++++++++++++++++++++++++++++ tools/vsh-table.h | 42 +++++ 3 files changed, 458 insertions(+), 1 deletion(-) create mode 100644 tools/vsh-table.c create mode 100644 tools/vsh-table.h
+/** + * vshTableGetColumnsWidths: + * @table: table + * @maxwidths: maximum count of characters for each columns + * @widths: count of characters for each cell in the table + * + * Fill passed @maxwidths and @widths arrays with maximum number + * of characters for columns and number of character per each + * table cell, respectively. + * + * Handle unicode strings (user must have multibyte locale) + */ +static int +vshTableGetColumnsWidths(vshTablePtr table, + size_t *maxwidths, + size_t **widths, + bool header) +{ + int ret = -1; + size_t i = 1; + size_t j; + size_t len; + int tmp; + wchar_t *wstr = NULL; + size_t wstrlen; + + if (header) + i = 0; + else + i = 1; + for (; i < table->nrows; i++) { + vshTableRowPtr row = table->rows[i]; + + for (j = 0; j < row->ncells; j++) {
+ /* strlen should return maximum possible length needed */ + wstrlen = strlen(row->cells[j]); + VIR_FREE(wstr); + if (VIR_ALLOC_N(wstr, wstrlen) < 0) + goto cleanup; + /* mbstowcs fails if machine is using singlebyte locale + * and user tries to convert unicode(multibyte) + * */ + if (mbstowcs(wstr, row->cells[j], wstrlen) == + (size_t) -1) { + len = wstrlen; + } else { + tmp = wcswidth(wstr, wstrlen); + if (tmp < 0) + goto cleanup; + len = (size_t)((unsigned)tmp); + }
Whatever solution you use for converting a multi-byte string to a maximum count of characters, please put it in a separate function. That would make this function more readable.
Note that unfortnately libunistring is "GPLv2+ or LGPLv3+", so is not compatible with libvirt.so It is fine to use it from virsh since that's free to be GPLv2+ only, but we mustn't link libunistring to libvirt.so. So any helper func would have to stay in the tools dir just for virsh/virt-admin Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
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> --- tests/virshtest.c | 14 ++++++------ tools/virsh-domain-monitor.c | 43 ++++++++++++++++++++---------------- 2 files changed, 31 insertions(+), 26 deletions(-) diff --git a/tests/virshtest.c b/tests/virshtest.c index 94548a82d1..10cd0d356b 100644 --- a/tests/virshtest.c +++ b/tests/virshtest.c @@ -98,9 +98,9 @@ static int testCompareListDefault(const void *data ATTRIBUTE_UNUSED) { const char *const argv[] = { VIRSH_DEFAULT, "list", NULL }; const char *exp = "\ - Id Name State\n\ -----------------------------------------------------\n\ - 1 test running\n\ + Id Name State \n\ +----------------------\n\ + 1 test running \n\ \n"; return testCompareOutputLit(exp, NULL, argv); } @@ -109,10 +109,10 @@ static int testCompareListCustom(const void *data ATTRIBUTE_UNUSED) { const char *const argv[] = { VIRSH_CUSTOM, "list", NULL }; const char *exp = "\ - Id Name State\n\ -----------------------------------------------------\n\ - 1 fv0 running\n\ - 2 fc4 running\n\ + Id Name State \n\ +----------------------\n\ + 1 fv0 running \n\ + 2 fc4 running \n\ \n"; return testCompareOutputLit(exp, NULL, argv); } diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index b9b4f9739b..adc5bb1a7a 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,22 @@ 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); - + if (vshTableRowAppend(table, id_buf, + virDomainGetName(dom), + state == -2 ? _("saved") + : virshDomainStateToString(state), + title, NULL) < 0) + goto cleanup; VIR_FREE(title); } else { - vshPrint(ctl, " %-5s %-30s %s\n", id_buf, - virDomainGetName(dom), - state == -2 ? _("saved") - : virshDomainStateToString(state)); + if (vshTableRowAppend(table, id_buf, + virDomainGetName(dom), + state == -2 ? _("saved") + : virshDomainStateToString(state), + NULL) < 0) + goto cleanup; } + } else if (optUUID && optName) { if (virDomainGetUUIDString(dom, uuid) < 0) { vshError(ctl, "%s", _("Failed to get domain's UUID")); @@ -2004,8 +2005,12 @@ cmdList(vshControl *ctl, const vshCmd *cmd) } } + if (optTable) + vshTablePrintToStdout(table, ctl); + ret = true; cleanup: + vshTableFree(table); virshDomainListFree(list); return ret; } -- 2.17.1
For now, there are 5 test cases - testVshTableNew: Creating table with empty header - testVshTableHeader: Printing table with/without header - testVshTableRowAppend: Appending row with various number of cells. Only row with same number of cells as in header is accepted. - testVshTableNewUnicode: Printing table with unicode characters. Checking correct alignment. - testNTables: Create and print various types of tables - one column, one row table, table without content, standard table... Signed-off-by: Simon Kobyda <skobyda@redhat.com> --- tests/Makefile.am | 8 ++ tests/vshtabletest.c | 247 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 255 insertions(+) create mode 100644 tests/vshtabletest.c diff --git a/tests/Makefile.am b/tests/Makefile.am index 21a6c823d9..136fe16f71 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -206,6 +206,7 @@ test_programs = virshtest sockettest \ virhostdevtest \ virnetdevtest \ virtypedparamtest \ + vshtabletest \ $(NULL) test_libraries = libshunload.la \ @@ -938,6 +939,13 @@ metadatatest_SOURCES = \ testutils.c testutils.h metadatatest_LDADD = $(LDADDS) $(LIBXML_LIBS) +vshtabletest_SOURCES = \ + vshtabletest.c \ + testutils.c testutils.h +vshtabletest_LDADD = \ + $(LDADDS) \ + ../tools/libvirt_shell.la + virshtest_SOURCES = \ virshtest.c \ testutils.c testutils.h diff --git a/tests/vshtabletest.c b/tests/vshtabletest.c new file mode 100644 index 0000000000..b41a205761 --- /dev/null +++ b/tests/vshtabletest.c @@ -0,0 +1,247 @@ +/* + * 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/>. + */ + +#include <config.h> + +#include <stdlib.h> +#include <stdio.h> +#include <locale.h> + +#include "internal.h" +#include "testutils.h" +#include "viralloc.h" +#include "../tools/vsh-table.h" + +static int +testVshTableNew(const void *opaque ATTRIBUTE_UNUSED) +{ + int ret = 0; + + if (vshTableNew(NULL)) { + fprintf(stderr, "expected failure when passing null to" + "vshtablenew\n"); + ret = -1; + } + + return ret; +} + +static int +testVshTableHeader(const void *opaque ATTRIBUTE_UNUSED) +{ + int ret = 0; + char *out; + const char *exp = "\ + 1 fedora28 running \n\ + 2 rhel7.5 running \n"; + const char *exp2 = "\ + Id Name State \n\ +--------------------------\n\ + 1 fedora28 running \n\ + 2 rhel7.5 running \n"; + + vshTablePtr table = vshTableNew("Id", "Name", "State", + NULL); //to ask about return + if (!table) + goto cleanup; + + vshTableRowAppend(table, "1", "fedora28", "running", NULL); + vshTableRowAppend(table, "2", "rhel7.5", "running", + NULL); + + out = vshTablePrintToString(table, false); + if (virTestCompareToString(exp, out) < 0) + ret = -1; + + VIR_FREE(out); + out = vshTablePrintToString(table, true); + if (virTestCompareToString(exp2, out) < 0) + ret = -1; + + cleanup: + VIR_FREE(out); + vshTableFree(table); + return ret; +} + +static int +testVshTableNewUnicode(const void *opaque ATTRIBUTE_UNUSED) +{ + + int ret = 0; + char *out; + + char *locale = setlocale(LC_CTYPE, NULL); + if (!setlocale(LC_CTYPE, "en_US.UTF-8")) + return EXIT_AM_SKIP; + + const char *exp = "\ + Id 名稱 государство \n\ +-----------------------------------------\n\ + 1 fedora28 running \n\ + 2 🙊🙉🙈rhel7.5🙆🙆🙅 running \n"; + vshTablePtr table; + + table = vshTableNew("Id", "名稱", "государство", NULL); + if (!table) + goto cleanup; + + vshTableRowAppend(table, "1", "fedora28", "running", NULL); + vshTableRowAppend(table, "2", "🙊🙉🙈rhel7.5🙆🙆🙅", "running", + NULL); + + out = vshTablePrintToString(table, true); + if (virTestCompareToString(exp, out) < 0) + ret = -1; + + cleanup: + setlocale(LC_CTYPE, locale); + VIR_FREE(out); + vshTableFree(table); + return ret; +} + +static int +testVshTableRowAppend(const void *opaque ATTRIBUTE_UNUSED) +{ + int ret = 0; + + vshTablePtr table = vshTableNew("Id", "Name", NULL); + if (!table) + goto cleanup; + + if (vshTableRowAppend(table, NULL) >= 0) { + fprintf(stderr, "Appending NULL shouldn't work\n"); + ret = -1; + } + + if (vshTableRowAppend(table, "2", NULL) >= 0) { + fprintf(stderr, "Appending less items than in header\n"); + ret = -1; + } + + if (vshTableRowAppend(table, "2", "rhel7.5", "running", + NULL) >= 0) { + fprintf(stderr, "Appending more items than in header\n"); + ret = -1; + } + + if (vshTableRowAppend(table, "2", "rhel7.5", NULL) < 0) { + fprintf(stderr, "Appending same number of items as in header" + " should not return NULL\n"); + ret = -1; + } + + cleanup: + vshTableFree(table); + return ret; +} + +static int +testNTables(const void *opaque ATTRIBUTE_UNUSED) +{ + int ret = 0; + vshTablePtr table1; + vshTablePtr table2; + vshTablePtr table3; + const char *exp1 = "\ + Id Name Status \n\ +--------------------------\n\ + 1 fedora28 running \n\ + 2 rhel7.5 running \n"; + const char *exp2 = "\ + Id Name Status \n\ +---------------------\n"; + const char *exp3 = "\ + Id \n\ +-----\n\ + 1 \n\ + 2 \n\ + 3 \n\ + 4 \n"; + char *out1; + char *out2; + char *out3; + + table1 = vshTableNew("Id", "Name", "Status", NULL); + if (!table1) + goto cleanup; + vshTableRowAppend(table1, "1", "fedora28", "running", NULL); + vshTableRowAppend(table1, "2", "rhel7.5", "running", NULL); + out1 = vshTablePrintToString(table1, true); + + table2 = vshTableNew("Id", "Name", "Status", NULL); + if (!table2) + goto cleanup; + out2 = vshTablePrintToString(table2, true); + + table3 = vshTableNew("Id", NULL); + if (!table3) + goto cleanup; + vshTableRowAppend(table3, "1", NULL); + vshTableRowAppend(table3, "2", NULL); + vshTableRowAppend(table3, "3", NULL); + vshTableRowAppend(table3, "4", NULL); + out3 = vshTablePrintToString(table3, true); + + if (virTestCompareToString(exp1, out1) < 0) + ret = -1; + if (virTestCompareToString(exp2, out2) < 0) + ret = -1; + if (virTestCompareToString(exp3, out3) < 0) + ret = -1; + + cleanup: + VIR_FREE(out1); + VIR_FREE(out2); + VIR_FREE(out3); + vshTableFree(table1); + vshTableFree(table2); + vshTableFree(table3); + return ret; +} + +static int +mymain(void) +{ + int ret = 0; + + if (virTestRun("testVshTableNew", testVshTableNew, + NULL) < 0) + ret = -1; + + if (virTestRun("testVshTableHeader", testVshTableHeader, + NULL) < 0) + ret = -1; + + if (virTestRun("testVshTableRowAppend", testVshTableRowAppend, + NULL) < 0) + ret = -1; + + if (virTestRun("testVshTableWithUnicode", testVshTableNewUnicode, + NULL) < 0) + ret = -1; + + if (virTestRun("testNTables", testNTables, + NULL) < 0) + ret = -1; + + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; +} + +VIR_TEST_MAIN(mymain) -- 2.17.1
On Thu, Aug 16, 2018 at 12:56:26PM +0200, Simon Kobyda wrote:
For now, there are 5 test cases - testVshTableNew: Creating table with empty header - testVshTableHeader: Printing table with/without header - testVshTableRowAppend: Appending row with various number of cells. Only row with same number of cells as in header is accepted. - testVshTableNewUnicode: Printing table with unicode characters. Checking correct alignment. - testNTables: Create and print various types of tables - one column, one row table, table without content, standard table...
Signed-off-by: Simon Kobyda <skobyda@redhat.com> --- tests/Makefile.am | 8 ++ tests/vshtabletest.c | 247 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 255 insertions(+) create mode 100644 tests/vshtabletest.c +static int +testVshTableNew(const void *opaque ATTRIBUTE_UNUSED) +{ + int ret = 0;
no need for a 'ret' variable if you don't have a cleanup section.
+ + if (vshTableNew(NULL)) { + fprintf(stderr, "expected failure when passing null to" + "vshtablenew\n");
Missing space between 'to' and 'vshtablenew'
+ ret = -1; + } + + return ret; +} + +static int +testVshTableHeader(const void *opaque ATTRIBUTE_UNUSED) +{ + int ret = 0; + char *out; + const char *exp = "\ + 1 fedora28 running \n\ + 2 rhel7.5 running \n"; + const char *exp2 = "\ + Id Name State \n\ +--------------------------\n\ + 1 fedora28 running \n\ + 2 rhel7.5 running \n";
Please use one string literal per line in new code. That way you can align them.
+ + vshTablePtr table = vshTableNew("Id", "Name", "State", + NULL); //to ask about return + if (!table) + goto cleanup; + + vshTableRowAppend(table, "1", "fedora28", "running", NULL); + vshTableRowAppend(table, "2", "rhel7.5", "running", + NULL); +
[...]
+static int +testNTables(const void *opaque ATTRIBUTE_UNUSED) +{ + int ret = 0; + vshTablePtr table1; + vshTablePtr table2; + vshTablePtr table3; + const char *exp1 = "\ + Id Name Status \n\ +--------------------------\n\ + 1 fedora28 running \n\ + 2 rhel7.5 running \n"; + const char *exp2 = "\ + Id Name Status \n\ +---------------------\n"; + const char *exp3 = "\ + Id \n\ +-----\n\ + 1 \n\ + 2 \n\ + 3 \n\ + 4 \n"; + char *out1; + char *out2; + char *out3; + + table1 = vshTableNew("Id", "Name", "Status", NULL); + if (!table1) + goto cleanup; + vshTableRowAppend(table1, "1", "fedora28", "running", NULL); + vshTableRowAppend(table1, "2", "rhel7.5", "running", NULL); + out1 = vshTablePrintToString(table1, true); + + table2 = vshTableNew("Id", "Name", "Status", NULL); + if (!table2) + goto cleanup;
out2 and out3 are unitialized if you jump to cleanup here. Jano
+ out2 = vshTablePrintToString(table2, true); + + table3 = vshTableNew("Id", NULL);
participants (4)
-
Daniel P. Berrangé -
Ján Tomko -
Michal Privoznik -
Simon Kobyda