Re: [libvirt] [PATCH v4 1/3] vsh: Add API for printing tables.

[Daniel P . Berrangé]

On Thu, Aug 23, 2018 at 03:19:39PM +0200, Michal Privoznik wrote:
> On 08/22/2018 07:42 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 
> > ---
> >  tools/Makefile.am |   4 +-
> >  tools/vsh-table.c | 483 ++
> >  tools/vsh-table.h |  42 
> >  3 files changed, 528 insertions(+), 1 deletion(-)
> >  create mode 100644 tools/vsh-table.c
> >  create mode 100644 tools/vsh-table.h


> > +/**
> > + * Function pulled from util-linux
> > + *
> > + * Function's name in util-linux: mbs_safe_encode_to_buffer
> > + *
> > + * Copy @s to @buf and replace control and non-printable chars with
> > + * \x?? hex sequence. The @width returns number of cells. The @safechars
> > + * are not encoded.
> > + *
> > + * The @buf has to be big enough to store mbs_safe_encode_size(strlen(s)))
> > + * bytes.
> 
> It's nice to give others credit, but the arguments make no sense to me.
> mbs_safe_encode_size ain't no libvirt function. But
> vshTableSafeEncodeSize() is.
> 
> Also, since we don't need the intermediate buffer anywhere, nor the safe
> buffer size can we merge those two functions together? This would have
> also a benefit of not duplicating some operations (e.g. strlen).
> 
> Moreover, safechars is always NULL. Do we need that argument?
> 
> NB, do we need to re-encode the string? All that we care about is its
> width, isn't it?

It is a design tradeoff to make the implementation more practical.

The code that determines the width doesn't work correctly for all
possibly unicode characters. By encoding the string we rewrite the
characters we can't handle into something we can handle. This means
our width calculation is always correct, but there are some
characters that we will end up displaying as escape sequences
instead.

Alternatively we can display every charcter normally, but have possibly
screwed up alignment due to incorrect width calculation.

IMHO if the escaping was good enough for util-linux to use, it is good
enough for us to use too.

Or to put it another way, if someone complains about this, we can
we can file the same bug against util-linux, let them fix it, and
then copy their fix back into our code :-)

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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 1/3] vsh: Add API for printing tables.

[Simon Kobyda]

On Thu, 2018-08-23 at 15:19 +0200, Michal Privoznik wrote:
> NB, do we need to re-encode the string? All that we care about is its
> width, isn't it?

Yes. Many nonprintable or control characters such as tabulator,
vertical tabulator, backspace... would be problematic, especially when
calculating width. So its better to just replace them with their
hexadecimal value.

Simon Kobyda

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 1/3] vsh: Add API for printing tables.

[Michal Privoznik]

On 08/22/2018 07:42 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 
> ---
>  tools/Makefile.am |   4 +-
>  tools/vsh-table.c | 483 ++
>  tools/vsh-table.h |  42 
>  3 files changed, 528 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 00..6e1793e4e3
> --- /dev/null
> +++ b/tools/vsh-table.c
> @@ -0,0 +1,483 @@
> +/*
> + * 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
> + * .
> + *
> + * Authors:
> + *   Simon Kobyda 
> + *
> + */
> +
> +#include 
> +#include "vsh-table.h"
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#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.

There should be an empty line between the text and "Returns:"

> + */
> +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"));

Misaligned line ;-) Here and on some other places.

> +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;

The @tmp just leaked if VIR_APPEND_ELEMENT() fails. How about:

1) moving the variable declaration into this while() loop,
2) calling VIR_FREE(tmp) just before this 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 

Re: [libvirt] [PATCH v4 1/3] vsh: Add API for printing tables.

[Simon Kobyda]

On Wed, 2018-08-22 at 19:42 +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 
> ---
>  tools/Makefile.am |   4 +-
>  tools/vsh-table.c | 483
> ++
>  tools/vsh-table.h |  42 
>  3 files changed, 528 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 00..6e1793e4e3
> --- /dev/null
> +++ b/tools/vsh-table.c
> @@ -0,0 +1,483 @@
> +/*
> + * 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
> + * .
> + *
> + * Authors:
> + *   Simon Kobyda 
> + *
> + */
> +
> +#include 
> +#include "vsh-table.h"
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#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;
> +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,