Re: cleanups with long-term benefits (was Re: [PATCH] schemas: Add vim modeline)

[Daniel P . Berrangé]

On Fri, Jul 31, 2020 at 06:28:06PM +0200, Paolo Bonzini wrote:
> On 31/07/20 17:07, Daniel P. Berrangé wrote:
> > The QAPI JSON-but-not file format is a case where I think we should just
> > adopt a standard file format no matter what. A conversion will have some
> > short term work, but this is really simple data to deal with and the code
> > involved is nicely self contained. Again I'm not saying QAPI maintainers
> > must do it, just put the idea out there as a piece of work that would
> > be welcomed if someone is interested in working ont.
> 
> The main issues with JSON-but-not in QEMU are:
> 
> - the 64-bit integers, which does not apply to the QAPI schema though
> 
> - the comments, which are a common extension to JSON (see JSON5, NodeJS
> config files, json_verify's -c option, etc.) so I find it quite surprising
> that no off-the-shelf Python component can parse JSON + comments
> 
> - the single-quote strings, which are not particularly useful in QAPI schema

NB our files are not JSON documents, they are a concatenation of a list
of JSON documents. 

> 
> If we changed the single-quote strings to double-quote, jsonc.vim
> (https://github.com/neoclide/jsonc.vim) seems to support JSON + comments.
> In Emacs you'd probably add an epilogue like
> 
> (defconst json-mode-comments-re (rx (group "#" (zero-or-more nonl) line-end)))
> (push (list json-mode-comments-re 1 font-lock-comment-face) 
> json-font-lock-keywords-1)
> 
> Did I miss anything?
> 
> Besides that, why are we using Python and not JavaScript in the mode line?

If you use javascript mode, then emacs will highlight all the javascript
language keywords in the comments because it doesn't recognise "#" as a
comment - you need // or /* .. */ for comments in JavaScript

We could of course convert to genuinely use Javascript comment syntax
if we want to consider these files to be JavaScript code instead of
JSON data.


> > Another example would be elimination of anything in QEMU code that is
> > duplicating functionality in GLib, even if there zero functional
> > difference between the two impls.
> 
> Would you consider intrusive lists vs. GList/GSList to be duplicating 
> functionality?  I don't think we have that many duplicates at this 
> point.

Yep, I'd have a preference for use of GList / GSList. Anything which
uses G_DEFINE_AUTOPTR_CLEANUP_FUNC() automatically gets support for
auto cleanup of GList, GSList and GQueue, deep free'ing each element.

https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.html#g-autolist

Some of the io/ subsystem could start making more use of GIO APIs. We
didn't use GIO originally as our min-GLib version at the time meant
we couldn't use the functionality we needed.

I'd also suggest replacing  fprintf/printf/sprintf with the g*printf*
equivalents. The benefit here is that GLib guarantees that its variants
work the same way on Windows as on Linux. They pulled in the GNULIB
printf impl to replace Microsoft's broken impl.

There's various silly little things like ARRAY_SIZE vs G_N_ELEMENTS,
and __attribute__ macros - QEMU_NORETURN vs G_GNUC_NORETURN.
QEMU_BUILD_BUG_ON vs G_STATIC_ASSERT

There's QEMU's threads related wrappers. I vaguely there were a couple of
cases where GLib's threads APIs didn't do what we needed. Even if we can't
eliminate all our threads APIs, I expect we can cull alot.

There's util/buffer.{c.h} that can be replaced by GString or GArray.


There's some places where we have misc utility functions that we should
just contribute back to GLib upstream. eg our util/base64.c which offers
improved error checking on base64 decoding. GLib would benefit from this,
and while it won't help qemu immediately, in the long term it will.


> We have qemu_strto*, but unfortunately the GLib function g_ascii_strtoll
> does nothing to fix the awful design of the C standard strto* functions
> (especially the overloading of the return value for error and result).
> If there are cases that are clear cut against adopting the GLib version,
> I think patches would be easily accepted.

The g_ascii_strtoll aren't a drop in replacement for qemu_strto*, as they
have intentionally different semantics from each other, so not an example
I was considering here.

The GLib functions are explicitly always using C-locale, while QEMU's
current places are honouring current locale.

There are some places in QEMU that really should be forcing C-locale,
and so g_ascii_strtoll would actually be a bug fix in those cases.
Other places simply have to stick with what they're currently using
to honour the locale.

> > Another example would be adopting a standard code style and using a
> > tool like clang-format to enforce this for entire of existing code
> > base and future contributions and throwing away our checkpatch.pl
> > which nearly everyone is scared of touching as it is Perl code.
> 
> Checkpatch does much more than that, though the scary part is indeed the 
> one that enfoces coding style.  I wouldn't have a problem with using
> clang-format and calling it a day.

If there are things missing that we consider important, a long term
better strategy would be to use the Python binding to libclang to
detect the problem, instead of trying to parse C code with Perl and
regexes.

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