[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v5 0/3] vsh: Introduce new API for printing tables



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.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]