[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 <skobyda at redhat.com>
> > ---
> >  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 :|