[libvirt] [PATCH v5 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 v5: - cleanup and merged code for calculating zero-width, nonprintable and combined character. - replaced virBufferAddStr with virBufferAddChar in some places - in tests moved code for setting correct locale - fixed few leaks and unitialized values Changes in v4: - fixed width calculation for zero-width, nonprintable and combined character. (pulled some code from linux-util) - added tests for cases mentioned above - changed usage of vshControl variables. From now on PrintToStdout calls PrintToString and then prints returned string to stdout 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 | 377 +++++++++++++++++++++++++++++ tools/Makefile.am | 4 +- tools/virsh-domain-monitor.c | 43 ++-- tools/vsh-table.c | 449 +++++++++++++++++++++++++++++++++++ tools/vsh-table.h | 42 ++++ 7 files changed, 910 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 | 449 ++++++++++++++++++++++++++++++++++++++++++++++ tools/vsh-table.h | 42 +++++ 3 files changed, 494 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..ca4e9265c5 --- /dev/null +++ b/tools/vsh-table.c @@ -0,0 +1,449 @@ +/* + * 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 <wctype.h> +#include "c-ctype.h" + +#include "viralloc.h" +#include "virbuffer.h" +#include "virstring.h" +#include "virsh-util.h" + +#define HEX_ENCODE_LENGTH 4 /* represents length of '\xNN' */ + +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; + + if (!arg) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Table row cannot be empty")); + goto error; + } + + if (VIR_ALLOC(row) < 0) + goto error; + + while (arg) { + char *tmp = NULL; + + if (VIR_STRDUP(tmp, arg) < 0) + goto error; + + if (VIR_APPEND_ELEMENT(row->cells, row->ncells, tmp) < 0) { + VIR_FREE(tmp); + 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 = NULL; + 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; +} + +/** + * Function pulled from util-linux + * + * Function's name in util-linux: mbs_safe_encode_to_buffer + * + * Returns allocated string where all control and non-printable chars are + * replaced with \x?? hex sequence, or NULL. + */ +static char * +vshTableSafeEncode(const char *s, size_t *width) +{ + const char *p = s; + size_t sz = s ? strlen(s) : 0; + char *buf; + char *ret; + mbstate_t st; + + memset(&st, 0, sizeof(st)); + + if (VIR_ALLOC_N(buf, (sz * HEX_ENCODE_LENGTH) + 1) < 0) + return NULL; + + if (!sz) + return NULL; + + ret = buf; + *width = 0; + + while (p && *p) { + if ((*p == '\\' && *(p + 1) == 'x') || + c_iscntrl(*p)) { + snprintf(buf, HEX_ENCODE_LENGTH + 1, "\\x%02x", *p); + buf += HEX_ENCODE_LENGTH; + *width += HEX_ENCODE_LENGTH; + p++; + } else { + wchar_t wc; + size_t len = mbrtowc(&wc, p, MB_CUR_MAX, &st); + + if (len == 0) + break; /* end of string */ + + if (len == (size_t) -1 || len == (size_t) -2) { + len = 1; + /* + * Not valid multibyte sequence -- maybe it's + * printable char according to the current locales. + */ + if (!c_isprint(*p)) { + snprintf(buf, HEX_ENCODE_LENGTH + 1, "\\x%02x", *p); + buf += HEX_ENCODE_LENGTH; + *width += HEX_ENCODE_LENGTH; + } else { + (*width)++; + *buf++ = *p; + } + } else if (!iswprint(wc)) { + size_t i; + for (i = 0; i < len; i++) { + snprintf(buf, HEX_ENCODE_LENGTH + 1, "\\x%02x", p[i]); + buf += HEX_ENCODE_LENGTH; + *width += HEX_ENCODE_LENGTH; + } + } else { + memcpy(buf, p, len); + buf += len; + *width += wcwidth(wc); + } + p += len; + } + } + + *buf = '\0'; + 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) + * + * Return 0 in case of success, -1 otherwise. + */ +static int +vshTableGetColumnsWidths(vshTablePtr table, + size_t *maxwidths, + size_t **widths, + bool header) +{ + size_t i; + size_t j; + + i = header? 0 : 1; + for (; i < table->nrows; i++) { + vshTableRowPtr row = table->rows[i]; + + for (j = 0; j < row->ncells; j++) { + size_t size = 0; + /* need to replace nonprintable and control characters, + * because width of some of those characters (e.g. \t, \v, \b ...) + * cannot be counted properly */ + char *tmp = vshTableSafeEncode(row->cells[j], &size); + if (!tmp) + return -1; + + VIR_FREE(row->cells[j]); + row->cells[j] = tmp; + widths[i][j] = size; + + if (widths[i][j] > maxwidths[j]) + maxwidths[j] = widths[i][j]; + } + } + + return 0; +} + +/** + * vshTableRowPrint: + * @row: table to append to + * @maxwidths: maximum count of characters for each columns + * @widths: count of character for each cell in this row + * @buf: buffer to store table (only if @toStdout == true) + */ +static void +vshTableRowPrint(vshTableRowPtr row, + size_t *maxwidths, + size_t *widths, + virBufferPtr buf) +{ + size_t i; + size_t j; + + for (i = 0; i < row->ncells; i++) { + virBufferAsprintf(buf, " %s", row->cells[i]); + + for (j = 0; j < maxwidths[i] - widths[i] + 2; j++) + virBufferAddChar(buf, ' '); + } + virBufferAddChar(buf, '\n'); +} + +/** + * vshTablePrint: + * @table: table to print + * @header: whetever to print to header (true) or not (false) + * this argument is relevant only if @ctl == NULL + * + * Get 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 + */ +static char * +vshTablePrint(vshTablePtr table, bool header) +{ + 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 */ + vshTableRowPrint(table->rows[0], maxwidths, widths[0], &buf); + + /* print dividing line */ + for (i = 0; i < table->rows[0]->ncells; i++) { + for (j = 0; j < maxwidths[i] + 3; j++) + virBufferAddChar(&buf, '-'); + } + virBufferAddChar(&buf, '\n'); + } + /* print content */ + for (i = 1; i < table->nrows; i++) + vshTableRowPrint(table->rows[i], maxwidths, widths[i], &buf); + + 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 returned in string to stdout. + * If effect on vshControl structure on priting function changes in future + * (apart from quiet mode) this code may need update + */ +void +vshTablePrintToStdout(vshTablePtr table, vshControl *ctl) +{ + bool header; + char *out; + + header = ctl ? !ctl->quiet : true; + + out = vshTablePrintToString(table, header); + if (out) + vshPrint(ctl, "%s", out); + + VIR_FREE(out); +} + +/** + * 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(table, header); +} 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 08/23/2018 05:53 PM, 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 | 449 ++++++++++++++++++++++++++++++++++++++++++++++ tools/vsh-table.h | 42 +++++ 3 files changed, 494 insertions(+), 1 deletion(-) create mode 100644 tools/vsh-table.c create mode 100644 tools/vsh-table.h
diff --git a/tools/vsh-table.c b/tools/vsh-table.c new file mode 100644 index 0000000000..ca4e9265c5 --- /dev/null +++ b/tools/vsh-table.c
+/** + * Function pulled from util-linux + * + * Function's name in util-linux: mbs_safe_encode_to_buffer + * + * Returns allocated string where all control and non-printable chars are + * replaced with \x?? hex sequence, or NULL. + */ +static char * +vshTableSafeEncode(const char *s, size_t *width) +{ + const char *p = s; + size_t sz = s ? strlen(s) : 0; + char *buf; + char *ret; + mbstate_t st; + + memset(&st, 0, sizeof(st)); + + if (VIR_ALLOC_N(buf, (sz * HEX_ENCODE_LENGTH) + 1) < 0) + return NULL; + + if (!sz) + return NULL;
You need to swap these two lines otherwise @buf is leaked.
+ + ret = buf; + *width = 0; + + while (p && *p) { + if ((*p == '\\' && *(p + 1) == 'x') || + c_iscntrl(*p)) { + snprintf(buf, HEX_ENCODE_LENGTH + 1, "\\x%02x", *p); + buf += HEX_ENCODE_LENGTH; + *width += HEX_ENCODE_LENGTH; + p++; + } else { + wchar_t wc; + size_t len = mbrtowc(&wc, p, MB_CUR_MAX, &st); + + if (len == 0) + break; /* end of string */ + + if (len == (size_t) -1 || len == (size_t) -2) { + len = 1; + /* + * Not valid multibyte sequence -- maybe it's + * printable char according to the current locales. + */ + if (!c_isprint(*p)) { + snprintf(buf, HEX_ENCODE_LENGTH + 1, "\\x%02x", *p); + buf += HEX_ENCODE_LENGTH; + *width += HEX_ENCODE_LENGTH; + } else { + (*width)++; + *buf++ = *p; + } + } else if (!iswprint(wc)) { + size_t i; + for (i = 0; i < len; i++) { + snprintf(buf, HEX_ENCODE_LENGTH + 1, "\\x%02x", p[i]); + buf += HEX_ENCODE_LENGTH; + *width += HEX_ENCODE_LENGTH; + } + } else { + memcpy(buf, p, len); + buf += len; + *width += wcwidth(wc); + } + p += len; + } + } + + *buf = '\0'; + return ret; +} +
ACK Michal
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 9 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. - testUnicode: Printing table with unicode characters. Checking correct alignment. - testUnicodeArabic: test opposite (right to left) writing - testUnicodeZeroWidthChar - testUnicodeCombiningChar - testUnicodeNonPrintableChar, - testNTables: Create and print varios types of tables - one column, one row table, table without content, standart table... Signed-off-by: Simon Kobyda <skobyda@redhat.com> --- tests/Makefile.am | 8 + tests/vshtabletest.c | 377 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 385 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..1b07c37c56 --- /dev/null +++ b/tests/vshtabletest.c @@ -0,0 +1,377 @@ +/* + * 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) +{ + if (vshTableNew(NULL)) { + fprintf(stderr, "expected failure when passing null to vshTableNew\n"); + return -1; + } + + return 0; +} + +static int +testVshTableHeader(const void *opaque ATTRIBUTE_UNUSED) +{ + int ret = 0; + char *act = NULL; + 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); + + act = vshTablePrintToString(table, false); + if (virTestCompareToString(exp, act) < 0) + ret = -1; + + VIR_FREE(act); + act = vshTablePrintToString(table, true); + if (virTestCompareToString(exp2, act) < 0) + ret = -1; + + cleanup: + VIR_FREE(act); + 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 +testUnicode(const void *opaque ATTRIBUTE_UNUSED) +{ + + int ret = 0; + char *act = NULL; + + 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); + + act = vshTablePrintToString(table, true); + if (virTestCompareToString(exp, act) < 0) + ret = -1; + + cleanup: + VIR_FREE(act); + vshTableFree(table); + return ret; +} + +/* Point of this test is to see how table behaves with right to left writing*/ +static int +testUnicodeArabic(const void *opaque ATTRIBUTE_UNUSED) +{ + + int ret = 0; + char *act = NULL; + + const char *exp = +" ﻡﺍ ﻢﻣﺍ ﻕﺎﺌﻣﺓ ﺓ ﺎﻠﺼﻋ ﺍﻸﺜﻧﺎﻧ \n" +"-------------------------------------------------------------------------------------------\n" +" 1 ﻉﺪﻴﻟ ﺎﻠﺜﻘﻴﻟ ﻕﺎﻣ ﻊﻧ, ٣٠ ﻎﻴﻨﻳﺍ ﻮﺘﻧﺎﻤﺗ ﺎﻠﺛﺎﻠﺛ، ﺄﺳﺭ, ﺩﻮﻟ ﺩﻮﻟ. ﺄﻣﺎﻣ ﺍ ﺎﻧ ﻲﻜﻧ \n" +" ﺺﻔﺣﺓ ﺖﻜﺘﻴﻛﺍً ﻊﻟ, ﺎﻠﺠﻧﻭﺩ ﻭﺎﻠﻌﺗﺍﺩ ﺵﺭ \n"; + vshTablePtr table; + + table = vshTableNew("ﻡﺍ ﻢﻣﺍ ﻕﺎﺌﻣﺓ", "ﺓ ﺎﻠﺼﻋ", "ﺍﻸﺜﻧﺎﻧ", NULL); + if (!table) + goto cleanup; + vshTableRowAppend(table, + "1", + "ﻉﺪﻴﻟ ﺎﻠﺜﻘﻴﻟ ﻕﺎﻣ ﻊﻧ, ٣٠ ﻎﻴﻨﻳﺍ ﻮﺘﻧﺎﻤﺗ ﺎﻠﺛﺎﻠﺛ، ﺄﺳﺭ, ﺩﻮﻟ", + "ﺩﻮﻟ. ﺄﻣﺎﻣ ﺍ ﺎﻧ ﻲﻜﻧ", + NULL); + vshTableRowAppend(table, "ﺺﻔﺣﺓ", "ﺖﻜﺘﻴﻛﺍً ﻊﻟ, ﺎﻠﺠﻧﻭﺩ ﻭﺎﻠﻌﺗﺍﺩ", "ﺵﺭ", + NULL); + act = vshTablePrintToString(table, true); + if (virTestCompareToString(exp, act) < 0) + ret = -1; + + cleanup: + VIR_FREE(act); + vshTableFree(table); + return ret; +} + +/* Testing zero-width characters by inserting few zero-width spaces */ +static int +testUnicodeZeroWidthChar(const void *opaque ATTRIBUTE_UNUSED) +{ + + int ret = 0; + vshTablePtr table = NULL; + const char *exp = +" I\u200Bd Name \u200BStatus \n" +"--------------------------\n" +" 1\u200B fedora28 run\u200Bning \n" +" 2 rhel7.5 running \n"; + char *act = NULL; + + table = vshTableNew("I\u200Bd", "Name", "\u200BStatus", NULL); + if (!table) + goto cleanup; + vshTableRowAppend(table, "1\u200B", "fedora28", "run\u200Bning", NULL); + vshTableRowAppend(table, "2", "rhel7.5", "running", NULL); + act = vshTablePrintToString(table, true); + + if (virTestCompareToString(exp, act) < 0) + ret = -1; + + cleanup: + VIR_FREE(act); + vshTableFree(table); + return ret; +} + +static int +testUnicodeCombiningChar(const void *opaque ATTRIBUTE_UNUSED) +{ + int ret = 0; + vshTablePtr table = NULL; + const char *exp = +" Id Náme Ⓢtatus \n" +"--------------------------\n" +" 1 fědora28 running \n" +" 2 rhel running \n"; + char *act = NULL; + + table = vshTableNew("Id", "Náme", "Ⓢtatus", NULL); + if (!table) + goto cleanup; + vshTableRowAppend(table, "1", "fědora28", "running", NULL); + vshTableRowAppend(table, "2", "rhel", "running", NULL); + act = vshTablePrintToString(table, true); + + if (virTestCompareToString(exp, act) < 0) + ret = -1; + + cleanup: + VIR_FREE(act); + vshTableFree(table); + return ret; +} + +/* Testing zero-width characters by inserting few zero-width spaces */ +static int +testUnicodeNonPrintableChar(const void *opaque ATTRIBUTE_UNUSED) +{ + int ret = 0; + vshTablePtr table = NULL; + const char *exp = +" I\\x09d Name Status \n" +"----------------------------------\n" +" 1 f\\x07edora28 running \n" +" 2 rhel7.5 running \n"; + char *act = NULL; + + table = vshTableNew("I\td", "Name", "Status", NULL); + if (!table) + goto cleanup; + vshTableRowAppend(table, "1", "f\aedora28", "running", NULL); + vshTableRowAppend(table, "2", "rhel7.5", "running", NULL); + act = vshTablePrintToString(table, true); + + if (virTestCompareToString(exp, act) < 0) + ret = -1; + + cleanup: + VIR_FREE(act); + vshTableFree(table); + return ret; +} + +static int +testNTables(const void *opaque ATTRIBUTE_UNUSED) +{ + int ret = 0; + vshTablePtr table1 = NULL; + vshTablePtr table2 = NULL; + vshTablePtr table3 = NULL; + 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 *act1 = NULL; + char *act2 = NULL; + char *act3 = NULL; + + table1 = vshTableNew("Id", "Name", "Status", NULL); + if (!table1) + goto cleanup; + vshTableRowAppend(table1, "1", "fedora28", "running", NULL); + vshTableRowAppend(table1, "2", "rhel7.5", "running", NULL); + act1 = vshTablePrintToString(table1, true); + + table2 = vshTableNew("Id", "Name", "Status", NULL); + if (!table2) + goto cleanup; + act2 = 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); + act3 = vshTablePrintToString(table3, true); + + if (virTestCompareToString(exp1, act1) < 0) + ret = -1; + if (virTestCompareToString(exp2, act2) < 0) + ret = -1; + if (virTestCompareToString(exp3, act3) < 0) + ret = -1; + + cleanup: + VIR_FREE(act1); + VIR_FREE(act2); + VIR_FREE(act3); + vshTableFree(table1); + vshTableFree(table2); + vshTableFree(table3); + return ret; +} + +static int +mymain(void) +{ + int ret = 0; + + if (!setlocale(LC_CTYPE, "en_US.UTF-8")) + return EXIT_AM_SKIP; + + 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("testUnicode", testUnicode, NULL) < 0) + ret = -1; + + if (virTestRun("testUnicodeArabic", testUnicodeArabic, NULL) < 0) + ret = -1; + + if (virTestRun("testUnicodeZeroWidthChar", + testUnicodeZeroWidthChar, + NULL) < 0) + ret = -1; + + if (virTestRun("testUnicodeCombiningChar", + testUnicodeCombiningChar, + NULL) < 0) + ret = -1; + + if (virTestRun("testUnicodeNonPrintableChar", + testUnicodeNonPrintableChar, + 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 08/23/2018 05:53 PM, Simon Kobyda wrote:
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 v5: - cleanup and merged code for calculating zero-width, nonprintable and combined character. - replaced virBufferAddStr with virBufferAddChar in some places - in tests moved code for setting correct locale - fixed few leaks and unitialized values
Changes in v4: - fixed width calculation for zero-width, nonprintable and combined character. (pulled some code from linux-util) - added tests for cases mentioned above - changed usage of vshControl variables. From now on PrintToStdout calls PrintToString and then prints returned string to stdout
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 | 377 +++++++++++++++++++++++++++++ tools/Makefile.am | 4 +- tools/virsh-domain-monitor.c | 43 ++-- tools/vsh-table.c | 449 +++++++++++++++++++++++++++++++++++ tools/vsh-table.h | 42 ++++ 7 files changed, 910 insertions(+), 27 deletions(-) create mode 100644 tests/vshtabletest.c create mode 100644 tools/vsh-table.c create mode 100644 tools/vsh-table.h
ACKed and pushed. Now we can start converting the rest of the code to use vshTable APIs. Michal
On Fri, Aug 24, 2018 at 10:59:04AM +0200, Michal Privoznik wrote:
On 08/23/2018 05:53 PM, Simon Kobyda wrote:
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 v5: - cleanup and merged code for calculating zero-width, nonprintable and combined character. - replaced virBufferAddStr with virBufferAddChar in some places - in tests moved code for setting correct locale - fixed few leaks and unitialized values
Changes in v4: - fixed width calculation for zero-width, nonprintable and combined character. (pulled some code from linux-util) - added tests for cases mentioned above - changed usage of vshControl variables. From now on PrintToStdout calls PrintToString and then prints returned string to stdout
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 | 377 +++++++++++++++++++++++++++++ tools/Makefile.am | 4 +- tools/virsh-domain-monitor.c | 43 ++-- tools/vsh-table.c | 449 +++++++++++++++++++++++++++++++++++ tools/vsh-table.h | 42 ++++ 7 files changed, 910 insertions(+), 27 deletions(-) create mode 100644 tests/vshtabletest.c create mode 100644 tools/vsh-table.c create mode 100644 tools/vsh-table.h
ACKed and pushed.
Now we can start converting the rest of the code to use vshTable APIs.
But first fix the build failures :-) On CentOS / RHEL: https://travis-ci.org/libvirt/libvirt/jobs/420024141 4) testUnicode ... Offset 30 Expect [государство ----------------------------------------- 1 fedora28 running 2 🙊🙉🙈rhel7.5🙆🙆🙅] Actual [ государство ----------------------------------------------------------------------------------------------------------------------------- 1 fedora28 running 2 \xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xffrhel7.5\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff] On Mingw: https://travis-ci.org/libvirt/libvirt/jobs/420024142 vsh-table.c: In function 'vshTableSafeEncode': vsh-table.c:274:27: error: implicit declaration of function 'wcwidth' [-Werror=implicit-function-declaration] *width += wcwidth(wc); ^~~~~~~ vsh-table.c:274:27: error: nested extern declaration of 'wcwidth' [-Werror=nested-externs] 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 08/24/2018 11:36 AM, Daniel P. Berrangé wrote:
On Fri, Aug 24, 2018 at 10:59:04AM +0200, Michal Privoznik wrote:
On 08/23/2018 05:53 PM, Simon Kobyda wrote:
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 v5: - cleanup and merged code for calculating zero-width, nonprintable and combined character. - replaced virBufferAddStr with virBufferAddChar in some places - in tests moved code for setting correct locale - fixed few leaks and unitialized values
Changes in v4: - fixed width calculation for zero-width, nonprintable and combined character. (pulled some code from linux-util) - added tests for cases mentioned above - changed usage of vshControl variables. From now on PrintToStdout calls PrintToString and then prints returned string to stdout
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 | 377 +++++++++++++++++++++++++++++ tools/Makefile.am | 4 +- tools/virsh-domain-monitor.c | 43 ++-- tools/vsh-table.c | 449 +++++++++++++++++++++++++++++++++++ tools/vsh-table.h | 42 ++++ 7 files changed, 910 insertions(+), 27 deletions(-) create mode 100644 tests/vshtabletest.c create mode 100644 tools/vsh-table.c create mode 100644 tools/vsh-table.h
ACKed and pushed.
Now we can start converting the rest of the code to use vshTable APIs.
But first fix the build failures :-)
On CentOS / RHEL:
https://travis-ci.org/libvirt/libvirt/jobs/420024141
4) testUnicode ... Offset 30 Expect [государство ----------------------------------------- 1 fedora28 running 2 🙊🙉🙈rhel7.5🙆🙆🙅] Actual [ государство ----------------------------------------------------------------------------------------------------------------------------- 1 fedora28 running 2 \xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xffrhel7.5\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff]
Okay, this is probably due to ancient gcc that's there (4.8.0) and is supposed to be fixed by adding -finput-charset= onto gcc command line. Haven't tested it though.
On Mingw:
https://travis-ci.org/libvirt/libvirt/jobs/420024142
vsh-table.c: In function 'vshTableSafeEncode': vsh-table.c:274:27: error: implicit declaration of function 'wcwidth' [-Werror=implicit-function-declaration] *width += wcwidth(wc); ^~~~~~~ vsh-table.c:274:27: error: nested extern declaration of 'wcwidth' [-Werror=nested-externs]
But this is weird. wcwidth() manpage says to include wchar.t which we are doing. Maybe _XOPEN_SOURCE macro is not defined? Anyway, I'll let Simon to figure this out O:-) Michal
On Fri, Aug 24, 2018 at 12:10:47PM +0200, Michal Privoznik wrote:
On 08/24/2018 11:36 AM, Daniel P. Berrangé wrote:
On Fri, Aug 24, 2018 at 10:59:04AM +0200, Michal Privoznik wrote:
On 08/23/2018 05:53 PM, Simon Kobyda wrote:
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 v5: - cleanup and merged code for calculating zero-width, nonprintable and combined character. - replaced virBufferAddStr with virBufferAddChar in some places - in tests moved code for setting correct locale - fixed few leaks and unitialized values
Changes in v4: - fixed width calculation for zero-width, nonprintable and combined character. (pulled some code from linux-util) - added tests for cases mentioned above - changed usage of vshControl variables. From now on PrintToStdout calls PrintToString and then prints returned string to stdout
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 | 377 +++++++++++++++++++++++++++++ tools/Makefile.am | 4 +- tools/virsh-domain-monitor.c | 43 ++-- tools/vsh-table.c | 449 +++++++++++++++++++++++++++++++++++ tools/vsh-table.h | 42 ++++ 7 files changed, 910 insertions(+), 27 deletions(-) create mode 100644 tests/vshtabletest.c create mode 100644 tools/vsh-table.c create mode 100644 tools/vsh-table.h
ACKed and pushed.
Now we can start converting the rest of the code to use vshTable APIs.
But first fix the build failures :-)
On CentOS / RHEL:
https://travis-ci.org/libvirt/libvirt/jobs/420024141
4) testUnicode ... Offset 30 Expect [государство ----------------------------------------- 1 fedora28 running 2 🙊🙉🙈rhel7.5🙆🙆🙅] Actual [ государство ----------------------------------------------------------------------------------------------------------------------------- 1 fedora28 running 2 \xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xffrhel7.5\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff]
Okay, this is probably due to ancient gcc that's there (4.8.0) and is supposed to be fixed by adding -finput-charset= onto gcc command line. Haven't tested it though.
On Mingw:
https://travis-ci.org/libvirt/libvirt/jobs/420024142
vsh-table.c: In function 'vshTableSafeEncode': vsh-table.c:274:27: error: implicit declaration of function 'wcwidth' [-Werror=implicit-function-declaration] *width += wcwidth(wc); ^~~~~~~ vsh-table.c:274:27: error: nested extern declaration of 'wcwidth' [-Werror=nested-externs]
But this is weird. wcwidth() manpage says to include wchar.t which we are doing. Maybe _XOPEN_SOURCE macro is not defined?
It is quite simple - the function doesn't exist on mingw :-) $ grep -r wcwidth /usr/i686-w64-mingw32/sys-root/mingw/include/ ...nothing... Looks like gnulib can rescue us though https://www.gnu.org/software/gnulib/manual/html_node/wcwidth.html 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 Fri, 2018-08-24 at 11:18 +0100, Daniel P. Berrangé wrote:
On Fri, Aug 24, 2018 at 12:10:47PM +0200, Michal Privoznik wrote:
On 08/24/2018 11:36 AM, Daniel P. Berrangé wrote:
On Fri, Aug 24, 2018 at 10:59:04AM +0200, Michal Privoznik wrote:
On 08/23/2018 05:53 PM, Simon Kobyda wrote:
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 v5: - cleanup and merged code for calculating zero-width, nonprintable and combined character. - replaced virBufferAddStr with virBufferAddChar in some places - in tests moved code for setting correct locale - fixed few leaks and unitialized values
Changes in v4: - fixed width calculation for zero-width, nonprintable and combined character. (pulled some code from linux-util) - added tests for cases mentioned above - changed usage of vshControl variables. From now on PrintToStdout calls PrintToString and then prints returned string to stdout
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 | 377 +++++++++++++++++++++++++++++ tools/Makefile.am | 4 +- tools/virsh-domain-monitor.c | 43 ++-- tools/vsh-table.c | 449 +++++++++++++++++++++++++++++++++++ tools/vsh-table.h | 42 ++++ 7 files changed, 910 insertions(+), 27 deletions(-) create mode 100644 tests/vshtabletest.c create mode 100644 tools/vsh-table.c create mode 100644 tools/vsh-table.h
ACKed and pushed.
Now we can start converting the rest of the code to use vshTable APIs.
But first fix the build failures :-)
On CentOS / RHEL:
https://travis-ci.org/libvirt/libvirt/jobs/420024141
4) testUnicode ... Offset 30 Expect [государство ----------------------------------------- 1 fedora28 running 2 🙊🙉🙈rhel7.5🙆🙆🙅] Actual [ государство --------------------------------------------------------------- -------------------------------------------------------------- 1 fedora28 running 2 \xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xffrhel7.5\xff \xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff]
Okay, this is probably due to ancient gcc that's there (4.8.0) and is supposed to be fixed by adding -finput-charset= onto gcc command line. Haven't tested it though.
On Mingw:
https://travis-ci.org/libvirt/libvirt/jobs/420024142
vsh-table.c: In function 'vshTableSafeEncode': vsh-table.c:274:27: error: implicit declaration of function 'wcwidth' [-Werror=implicit-function-declaration] *width += wcwidth(wc); ^~~~~~~ vsh-table.c:274:27: error: nested extern declaration of 'wcwidth' [-Werror=nested-externs]
But this is weird. wcwidth() manpage says to include wchar.t which we are doing. Maybe _XOPEN_SOURCE macro is not defined?
It is quite simple - the function doesn't exist on mingw :-)
$ grep -r wcwidth /usr/i686-w64-mingw32/sys-root/mingw/include/ ...nothing...
Looks like gnulib can rescue us though
https://www.gnu.org/software/gnulib/manual/html_node/wcwidth.html
Regards, Daniel
So I've looked it up and perhaps found something. Function iswprint (which is the root of our problem) is provided by glibc. CentOS 7 uses glibc 2.17, which was released in December 2017. I looked into ChangeLog of that version and I see they added table (or map?) for their nonascii symbols in 2009. Unicodes I used in tests (monkeyfaces) were added in 2010 in Unicode 6.0. So those specific unicodes therefore didn't find their way into nonascii table of glibc 2.17. :). I suspect the case is similar for RHEL. Btw. iswprint is using that table to determine whetever character is printable or not. Simon Kobyda.
On Fri, 2018-08-24 at 12:10 +0200, Michal Privoznik wrote:
On 08/24/2018 11:36 AM, Daniel P. Berrangé wrote:
On Fri, Aug 24, 2018 at 10:59:04AM +0200, Michal Privoznik wrote:
But first fix the build failures :-)
On CentOS / RHEL:
https://travis-ci.org/libvirt/libvirt/jobs/420024141
4) testUnicode . .. Offset 30 Expect [государство ----------------------------------------- 1 fedora28 running 2 🙊🙉🙈rhel7.5🙆🙆🙅] Actual [ государство ----------------------------------------------------------------- ------------------------------------------------------------ 1 fedora28 running 2 \xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xffrhel7.5\xff\x ff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff]
Okay, this is probably due to ancient gcc that's there (4.8.0) and is supposed to be fixed by adding -finput-charset= onto gcc command line. Haven't tested it though.
I tried but it didn't help. From what I understood, CentOS has problems with unicodes such as 🙊🙉🙈🙆🙆🙅. On that system, it can convert any of those characters to wchar_t successfully and properly, but when we pass that character to iswprint, it returns 0 (considers those wide characters nonprintable). Simon Kobyda
On Mon, Aug 27, 2018 at 05:50:22PM +0200, Simon Kobyda wrote:
On Fri, 2018-08-24 at 12:10 +0200, Michal Privoznik wrote:
On 08/24/2018 11:36 AM, Daniel P. Berrangé wrote:
On Fri, Aug 24, 2018 at 10:59:04AM +0200, Michal Privoznik wrote:
But first fix the build failures :-)
On CentOS / RHEL:
https://travis-ci.org/libvirt/libvirt/jobs/420024141
4) testUnicode . .. Offset 30 Expect [государство ----------------------------------------- 1 fedora28 running 2 🙊🙉🙈rhel7.5🙆🙆🙅] Actual [ государство ----------------------------------------------------------------- ------------------------------------------------------------ 1 fedora28 running 2 \xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xffrhel7.5\xff\x ff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff]
Okay, this is probably due to ancient gcc that's there (4.8.0) and is supposed to be fixed by adding -finput-charset= onto gcc command line. Haven't tested it though.
I tried but it didn't help. From what I understood, CentOS has problems with unicodes such as 🙊🙉🙈🙆🙆🙅. On that system, it can convert any of those characters to wchar_t successfully and properly, but when we pass that character to iswprint, it returns 0 (considers those wide characters nonprintable).
On the plus side, it appears that when this problem hits, the code is still correctly doing the column alignment taking account of these unexpected escape sequences. So how about storing 2 sets of expected data for this test case. In the unit test then call iswprint() to figure out which of the two expected data sets to compare against. 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, Aug 28, 2018 at 11:35:02AM +0100, Daniel P. Berrangé wrote:
On Mon, Aug 27, 2018 at 05:50:22PM +0200, Simon Kobyda wrote:
On Fri, 2018-08-24 at 12:10 +0200, Michal Privoznik wrote:
On 08/24/2018 11:36 AM, Daniel P. Berrangé wrote:
On Fri, Aug 24, 2018 at 10:59:04AM +0200, Michal Privoznik wrote:
But first fix the build failures :-)
On CentOS / RHEL:
https://travis-ci.org/libvirt/libvirt/jobs/420024141
4) testUnicode . .. Offset 30 Expect [государство ----------------------------------------- 1 fedora28 running 2 🙊🙉🙈rhel7.5🙆🙆🙅] Actual [ государство ----------------------------------------------------------------- ------------------------------------------------------------ 1 fedora28 running 2 \xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xffrhel7.5\xff\x ff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff]
Okay, this is probably due to ancient gcc that's there (4.8.0) and is supposed to be fixed by adding -finput-charset= onto gcc command line. Haven't tested it though.
I tried but it didn't help. From what I understood, CentOS has problems with unicodes such as 🙊🙉🙈🙆🙆🙅. On that system, it can convert any of those characters to wchar_t successfully and properly, but when we pass that character to iswprint, it returns 0 (considers those wide characters nonprintable).
On the plus side, it appears that when this problem hits, the code is still correctly doing the column alignment taking account of these unexpected escape sequences.
So how about storing 2 sets of expected data for this test case.
In the unit test then call iswprint() to figure out which of the two expected data sets to compare against.
How does it help us during runtime when someone uses such characters in a domain's name? It would still return a row consisting of escape sequences. So what's the point of providing 2 sets of expected data for a test just so it can pass, rather than use unicode characters we know would pass and everything else is a platform limitation which is out of our hands. Erik
On Tue, Aug 28, 2018 at 02:10:55PM +0200, Erik Skultety wrote:
On Tue, Aug 28, 2018 at 11:35:02AM +0100, Daniel P. Berrangé wrote:
On Mon, Aug 27, 2018 at 05:50:22PM +0200, Simon Kobyda wrote:
On Fri, 2018-08-24 at 12:10 +0200, Michal Privoznik wrote:
On 08/24/2018 11:36 AM, Daniel P. Berrangé wrote:
On Fri, Aug 24, 2018 at 10:59:04AM +0200, Michal Privoznik wrote:
But first fix the build failures :-)
On CentOS / RHEL:
https://travis-ci.org/libvirt/libvirt/jobs/420024141
4) testUnicode . .. Offset 30 Expect [государство ----------------------------------------- 1 fedora28 running 2 🙊🙉🙈rhel7.5🙆🙆🙅] Actual [ государство ----------------------------------------------------------------- ------------------------------------------------------------ 1 fedora28 running 2 \xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xffrhel7.5\xff\x ff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff]
Okay, this is probably due to ancient gcc that's there (4.8.0) and is supposed to be fixed by adding -finput-charset= onto gcc command line. Haven't tested it though.
I tried but it didn't help. From what I understood, CentOS has problems with unicodes such as 🙊🙉🙈🙆🙆🙅. On that system, it can convert any of those characters to wchar_t successfully and properly, but when we pass that character to iswprint, it returns 0 (considers those wide characters nonprintable).
On the plus side, it appears that when this problem hits, the code is still correctly doing the column alignment taking account of these unexpected escape sequences.
So how about storing 2 sets of expected data for this test case.
Two is not enough. My clang 5.0.1 produces a test that displays the monkeys correctly, but does not count their width properly: $ VIR_TEST_RANGE=4 VIR_TEST_DEBUG=1 ./run tests/vshtabletest TEST: vshtabletest 4) testUnicode ... Offset 24 Expect [ государство ----------------------------------------- 1 fedora28 ] Actual [государство ----------------------------------- 1 fedora28] ... FAILED
In the unit test then call iswprint() to figure out which of the two expected data sets to compare against.
How does it help us during runtime when someone uses such characters in a domain's name? It would still return a row consisting of escape sequences.
Not necessarily, see above.
So what's the point of providing 2 sets of expected data for a test just so it can pass, rather than use unicode characters we know would pass and everything else is a platform limitation which is out of our hands.
I still see a benefit in having testUnicodeBasic that passes everywhere (does it?), and conditionally running the monkey test on platforms where iswprint returns the proper results. Jano
On Tue, Aug 28, 2018 at 02:24:42PM +0200, Ján Tomko wrote:
On Tue, Aug 28, 2018 at 02:10:55PM +0200, Erik Skultety wrote:
On Tue, Aug 28, 2018 at 11:35:02AM +0100, Daniel P. Berrangé wrote:
On Mon, Aug 27, 2018 at 05:50:22PM +0200, Simon Kobyda wrote:
On Fri, 2018-08-24 at 12:10 +0200, Michal Privoznik wrote:
On 08/24/2018 11:36 AM, Daniel P. Berrangé wrote:
On Fri, Aug 24, 2018 at 10:59:04AM +0200, Michal Privoznik wrote:
But first fix the build failures :-)
On CentOS / RHEL:
https://travis-ci.org/libvirt/libvirt/jobs/420024141
4) testUnicode . .. Offset 30 Expect [государство ----------------------------------------- 1 fedora28 running 2 🙊🙉🙈rhel7.5🙆🙆🙅] Actual [ государство ----------------------------------------------------------------- ------------------------------------------------------------ 1 fedora28 running 2 \xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xffrhel7.5\xff\x ff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff]
Okay, this is probably due to ancient gcc that's there (4.8.0) and is supposed to be fixed by adding -finput-charset= onto gcc command line. Haven't tested it though.
I tried but it didn't help. From what I understood, CentOS has problems with unicodes such as 🙊🙉🙈🙆🙆🙅. On that system, it can convert any of those characters to wchar_t successfully and properly, but when we pass that character to iswprint, it returns 0 (considers those wide characters nonprintable).
On the plus side, it appears that when this problem hits, the code is still correctly doing the column alignment taking account of these unexpected escape sequences.
So how about storing 2 sets of expected data for this test case.
Two is not enough. My clang 5.0.1 produces a test that displays the monkeys correctly, but does not count their width properly:
$ VIR_TEST_RANGE=4 VIR_TEST_DEBUG=1 ./run tests/vshtabletest TEST: vshtabletest 4) testUnicode ... Offset 24 Expect [ государство ----------------------------------------- 1 fedora28 ] Actual [государство ----------------------------------- 1 fedora28] ... FAILED
In the unit test then call iswprint() to figure out which of the two expected data sets to compare against.
How does it help us during runtime when someone uses such characters in a domain's name? It would still return a row consisting of escape sequences.
Not necessarily, see above.
So what's the point of providing 2 sets of expected data for a test just so it can pass, rather than use unicode characters we know would pass and everything else is a platform limitation which is out of our hands.
I still see a benefit in having testUnicodeBasic that passes everywhere (does it?), and conditionally running the monkey test on platforms where iswprint returns the proper results.
Why? To see that glibc is new enough to support it? One would assume that if it works for n (given your example I'm so sure it actually does...), it would work for n+1 too, so I still don't see the point in this specific test case. Thanks for the example though. Erik
On Tue, Aug 28, 2018 at 02:30:23PM +0200, Erik Skultety wrote:
On Tue, Aug 28, 2018 at 02:24:42PM +0200, Ján Tomko wrote:
I still see a benefit in having testUnicodeBasic that passes everywhere (does it?), and conditionally running the monkey test on platforms where iswprint returns the proper results.
Why? To see that glibc is new enough to support it? One would assume
Then one might be surprised one day. Jano
that if it works for n (given your example I'm so sure it actually does...), it would work for n+1 too, so I still don't see the point in this specific test case.
Thanks for the example though. Erik
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Aug 28, 2018 at 02:24:42PM +0200, Ján Tomko wrote:
On Tue, Aug 28, 2018 at 02:10:55PM +0200, Erik Skultety wrote:
On Tue, Aug 28, 2018 at 11:35:02AM +0100, Daniel P. Berrangé wrote:
On Mon, Aug 27, 2018 at 05:50:22PM +0200, Simon Kobyda wrote:
On Fri, 2018-08-24 at 12:10 +0200, Michal Privoznik wrote:
On 08/24/2018 11:36 AM, Daniel P. Berrangé wrote:
On Fri, Aug 24, 2018 at 10:59:04AM +0200, Michal Privoznik wrote:
But first fix the build failures :-)
On CentOS / RHEL:
https://travis-ci.org/libvirt/libvirt/jobs/420024141
4) testUnicode . .. Offset 30 Expect [государство ----------------------------------------- 1 fedora28 running 2 🙊🙉🙈rhel7.5🙆🙆🙅] Actual [ государство ----------------------------------------------------------------- ------------------------------------------------------------ 1 fedora28 running 2 \xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xffrhel7.5\xff\x ff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff]
Okay, this is probably due to ancient gcc that's there (4.8.0) and is supposed to be fixed by adding -finput-charset= onto gcc command line. Haven't tested it though.
I tried but it didn't help. From what I understood, CentOS has problems with unicodes such as 🙊🙉🙈🙆🙆🙅. On that system, it can convert any of those characters to wchar_t successfully and properly, but when we pass that character to iswprint, it returns 0 (considers those wide characters nonprintable).
On the plus side, it appears that when this problem hits, the code is still correctly doing the column alignment taking account of these unexpected escape sequences.
So how about storing 2 sets of expected data for this test case.
Two is not enough. My clang 5.0.1 produces a test that displays the monkeys correctly, but does not count their width properly:
Is this a different bug though ? The issue with iswprint() is varying according to glibc version, not compiler version. So I wonder if the clang problem you mention is something that can be fixed in some way ? 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, Aug 28, 2018 at 02:24:19PM +0100, Daniel P. Berrangé wrote:
On Tue, Aug 28, 2018 at 02:24:42PM +0200, Ján Tomko wrote:
On Tue, Aug 28, 2018 at 02:10:55PM +0200, Erik Skultety wrote:
So how about storing 2 sets of expected data for this test case.
Two is not enough. My clang 5.0.1 produces a test that displays the monkeys correctly, but does not count their width properly:
Is this a different bug though ? The issue with iswprint() is varying according to glibc version, not compiler version.
The broken setup is: sys-libs/glibc-2.25-r9 sys-devel/clang-5.0.1 It works on: sys-libs/glibc-2.26-r7 with either of: sys-devel/clang-5.0.1 sys-devel/clang-6.0.1 So yes, it is a glibc bug. Depending on the version, either just wcwidth returns incorrect values for the monkeys (my case) or iswprint considers them non-printable.
So I wonder if the clang problem you mention is something that can be fixed in some way ?
Nope, red herring. My maint point was that there are more than 2 possible results. Jano
On Tue, Aug 28, 2018 at 03:41:26PM +0200, Ján Tomko wrote:
On Tue, Aug 28, 2018 at 02:24:19PM +0100, Daniel P. Berrangé wrote:
On Tue, Aug 28, 2018 at 02:24:42PM +0200, Ján Tomko wrote:
On Tue, Aug 28, 2018 at 02:10:55PM +0200, Erik Skultety wrote:
So how about storing 2 sets of expected data for this test case.
Two is not enough. My clang 5.0.1 produces a test that displays the monkeys correctly, but does not count their width properly:
Is this a different bug though ? The issue with iswprint() is varying according to glibc version, not compiler version.
The broken setup is: sys-libs/glibc-2.25-r9 sys-devel/clang-5.0.1
It works on: sys-libs/glibc-2.26-r7 with either of: sys-devel/clang-5.0.1 sys-devel/clang-6.0.1
So yes, it is a glibc bug. Depending on the version, either just wcwidth returns incorrect values for the monkeys (my case) or iswprint considers them non-printable.
It sounds like in your case we're genuinely broken in the virsh code, not merely tests broken. I wonder if we need extra logic in the virsh code to handle escaping for the cases where wcwidth is returning wrong data, so we still get column layout correct ?
So I wonder if the clang problem you mention is something that can be fixed in some way ?
Nope, red herring. My maint point was that there are more than 2 possible results.
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, Aug 28, 2018 at 02:46:08PM +0100, Daniel P. Berrangé wrote:
On Tue, Aug 28, 2018 at 03:41:26PM +0200, Ján Tomko wrote:
On Tue, Aug 28, 2018 at 02:24:19PM +0100, Daniel P. Berrangé wrote:
On Tue, Aug 28, 2018 at 02:24:42PM +0200, Ján Tomko wrote:
On Tue, Aug 28, 2018 at 02:10:55PM +0200, Erik Skultety wrote:
So how about storing 2 sets of expected data for this test case.
Two is not enough. My clang 5.0.1 produces a test that displays the monkeys correctly, but does not count their width properly:
Is this a different bug though ? The issue with iswprint() is varying according to glibc version, not compiler version.
The broken setup is: sys-libs/glibc-2.25-r9 sys-devel/clang-5.0.1
It works on: sys-libs/glibc-2.26-r7 with either of: sys-devel/clang-5.0.1 sys-devel/clang-6.0.1
So yes, it is a glibc bug. Depending on the version, either just wcwidth returns incorrect values for the monkeys (my case) or iswprint considers them non-printable.
It sounds like in your case we're genuinely broken in the virsh code, not merely tests broken.
I wonder if we need extra logic in the virsh code to handle escaping for the cases where wcwidth is returning wrong data, so we still get column layout correct ?
I don't think so. 1) wouldn't that involve reimplementing the wcwidth function? 2) users crazy enough to use new unicode characters are welcome to upgrade to new glibc, or suffer through misaligned virsh tables. Jano
So I wonder if the clang problem you mention is something that can be fixed in some way ?
Nope, red herring. My maint point was that there are more than 2 possible results.
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, Aug 28, 2018 at 03:52:56PM +0200, Ján Tomko wrote:
On Tue, Aug 28, 2018 at 02:46:08PM +0100, Daniel P. Berrangé wrote:
On Tue, Aug 28, 2018 at 03:41:26PM +0200, Ján Tomko wrote:
On Tue, Aug 28, 2018 at 02:24:19PM +0100, Daniel P. Berrangé wrote:
On Tue, Aug 28, 2018 at 02:24:42PM +0200, Ján Tomko wrote:
On Tue, Aug 28, 2018 at 02:10:55PM +0200, Erik Skultety wrote:
> So how about storing 2 sets of expected data for this test case. >
Two is not enough. My clang 5.0.1 produces a test that displays the monkeys correctly, but does not count their width properly:
Is this a different bug though ? The issue with iswprint() is varying according to glibc version, not compiler version.
The broken setup is: sys-libs/glibc-2.25-r9 sys-devel/clang-5.0.1
It works on: sys-libs/glibc-2.26-r7 with either of: sys-devel/clang-5.0.1 sys-devel/clang-6.0.1
So yes, it is a glibc bug. Depending on the version, either just wcwidth returns incorrect values for the monkeys (my case) or iswprint considers them non-printable.
It sounds like in your case we're genuinely broken in the virsh code, not merely tests broken.
I wonder if we need extra logic in the virsh code to handle escaping for the cases where wcwidth is returning wrong data, so we still get column layout correct ?
I don't think so.
1) wouldn't that involve reimplementing the wcwidth function? 2) users crazy enough to use new unicode characters are welcome to upgrade to new glibc, or suffer through misaligned virsh tables.
I second that, I don't think that in this case it's any beneficial trying to fix glibc problems just to offer users a consistent experience, we're not gnulib, besides, we're talking about table alignment which has been broken for ages and we've already made a significant progress here. Additionally, as Jano has pointed out, if someone feels adventurous, that's fine, but it may come with certain limitations. Erik
On Tue, Aug 28, 2018 at 04:10:53PM +0200, Erik Skultety wrote:
On Tue, Aug 28, 2018 at 03:52:56PM +0200, Ján Tomko wrote:
On Tue, Aug 28, 2018 at 02:46:08PM +0100, Daniel P. Berrangé wrote:
On Tue, Aug 28, 2018 at 03:41:26PM +0200, Ján Tomko wrote:
On Tue, Aug 28, 2018 at 02:24:19PM +0100, Daniel P. Berrangé wrote:
On Tue, Aug 28, 2018 at 02:24:42PM +0200, Ján Tomko wrote:
On Tue, Aug 28, 2018 at 02:10:55PM +0200, Erik Skultety wrote: > > So how about storing 2 sets of expected data for this test case. > >
Two is not enough. My clang 5.0.1 produces a test that displays the monkeys correctly, but does not count their width properly:
Is this a different bug though ? The issue with iswprint() is varying according to glibc version, not compiler version.
The broken setup is: sys-libs/glibc-2.25-r9 sys-devel/clang-5.0.1
It works on: sys-libs/glibc-2.26-r7 with either of: sys-devel/clang-5.0.1 sys-devel/clang-6.0.1
So yes, it is a glibc bug. Depending on the version, either just wcwidth returns incorrect values for the monkeys (my case) or iswprint considers them non-printable.
It sounds like in your case we're genuinely broken in the virsh code, not merely tests broken.
I wonder if we need extra logic in the virsh code to handle escaping for the cases where wcwidth is returning wrong data, so we still get column layout correct ?
I don't think so.
1) wouldn't that involve reimplementing the wcwidth function? 2) users crazy enough to use new unicode characters are welcome to upgrade to new glibc, or suffer through misaligned virsh tables.
I second that, I don't think that in this case it's any beneficial trying to fix glibc problems just to offer users a consistent experience, we're not gnulib, besides, we're talking about table alignment which has been broken for ages and we've already made a significant progress here. Additionally, as Jano has pointed out, if someone feels adventurous, that's fine, but it may come with certain limitations.
Ok, so we just need get the test to correctly skip on platforms where we know it won't get right answers 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 08/28/2018 10:11 AM, Daniel P. Berrangé wrote:
On Tue, Aug 28, 2018 at 04:10:53PM +0200, Erik Skultety wrote:
On Tue, Aug 28, 2018 at 03:52:56PM +0200, Ján Tomko wrote:
On Tue, Aug 28, 2018 at 02:46:08PM +0100, Daniel P. Berrangé wrote:
On Tue, Aug 28, 2018 at 03:41:26PM +0200, Ján Tomko wrote:
On Tue, Aug 28, 2018 at 02:24:19PM +0100, Daniel P. Berrangé wrote:
On Tue, Aug 28, 2018 at 02:24:42PM +0200, Ján Tomko wrote: > On Tue, Aug 28, 2018 at 02:10:55PM +0200, Erik Skultety wrote: >>> So how about storing 2 sets of expected data for this test case. >>> > > Two is not enough. My clang 5.0.1 produces a test that displays the > monkeys correctly, but does not count their width properly:
Is this a different bug though ? The issue with iswprint() is varying according to glibc version, not compiler version.
The broken setup is: sys-libs/glibc-2.25-r9 sys-devel/clang-5.0.1
It works on: sys-libs/glibc-2.26-r7 with either of: sys-devel/clang-5.0.1 sys-devel/clang-6.0.1
So yes, it is a glibc bug. Depending on the version, either just wcwidth returns incorrect values for the monkeys (my case) or iswprint considers them non-printable.
It sounds like in your case we're genuinely broken in the virsh code, not merely tests broken.
I wonder if we need extra logic in the virsh code to handle escaping for the cases where wcwidth is returning wrong data, so we still get column layout correct ?
I don't think so.
1) wouldn't that involve reimplementing the wcwidth function? 2) users crazy enough to use new unicode characters are welcome to upgrade to new glibc, or suffer through misaligned virsh tables.
I second that, I don't think that in this case it's any beneficial trying to fix glibc problems just to offer users a consistent experience, we're not gnulib, besides, we're talking about table alignment which has been broken for ages and we've already made a significant progress here. Additionally, as Jano has pointed out, if someone feels adventurous, that's fine, but it may come with certain limitations.
Ok, so we just need get the test to correctly skip on platforms where we know it won't get right answers
Cannot believe the effort/time on this... The problem with bypassing on such platforms is how does one then know those platforms get fixed so that the "skip" can be removed? Of course seeing test failure is not good either. There is no simple answer, we can try to code around this or we can accept on certain platforms the specific test may fail for a specific reason and move on. In the long run, it's a format that what percentage of our users will use? Is naming domains or "other" libvirt objects using emojis a real world problem that we really care to ensure that the table lines up properly? Maybe it's just best to determine that the name has one of those and accept that calculations are going to be wrong. Maybe we just decide if we see one in a name (e.g. a non normally printable or greater than some charset value via 0xNNNN) that we "assume" or "assign" the maximum width possible for each of those characters found and move on. Maybe we should "Validate" that a name doesn't have those chars and if it does fail to recognize using a new errno code we invent, "ENO🙊🙉🙈, usage of emoticons as names or keys is not supportable". John
On Tue, Aug 28, 2018 at 02:10:55PM +0200, Erik Skultety wrote:
On Tue, Aug 28, 2018 at 11:35:02AM +0100, Daniel P. Berrangé wrote:
On Mon, Aug 27, 2018 at 05:50:22PM +0200, Simon Kobyda wrote:
On Fri, 2018-08-24 at 12:10 +0200, Michal Privoznik wrote:
On 08/24/2018 11:36 AM, Daniel P. Berrangé wrote:
On Fri, Aug 24, 2018 at 10:59:04AM +0200, Michal Privoznik wrote:
But first fix the build failures :-)
On CentOS / RHEL:
https://travis-ci.org/libvirt/libvirt/jobs/420024141
4) testUnicode . .. Offset 30 Expect [государство ----------------------------------------- 1 fedora28 running 2 🙊🙉🙈rhel7.5🙆🙆🙅] Actual [ государство ----------------------------------------------------------------- ------------------------------------------------------------ 1 fedora28 running 2 \xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xffrhel7.5\xff\x ff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff]
Okay, this is probably due to ancient gcc that's there (4.8.0) and is supposed to be fixed by adding -finput-charset= onto gcc command line. Haven't tested it though.
I tried but it didn't help. From what I understood, CentOS has problems with unicodes such as 🙊🙉🙈🙆🙆🙅. On that system, it can convert any of those characters to wchar_t successfully and properly, but when we pass that character to iswprint, it returns 0 (considers those wide characters nonprintable).
On the plus side, it appears that when this problem hits, the code is still correctly doing the column alignment taking account of these unexpected escape sequences.
So how about storing 2 sets of expected data for this test case.
In the unit test then call iswprint() to figure out which of the two expected data sets to compare against.
How does it help us during runtime when someone uses such characters in a domain's name? It would still return a row consisting of escape sequences. So what's the point of providing 2 sets of expected data for a test just so it can pass, rather than use unicode characters we know would pass and everything else is a platform limitation which is out of our hands.
Well the idea is to prove that we get the column width calculation correct, even when the the glibc we have doesn't support these as printable characters 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 :|
participants (6)
-
Daniel P. Berrangé -
Erik Skultety -
John Ferlan -
Ján Tomko -
Michal Privoznik -
Simon Kobyda