Re: [XEN PATCH 07/11] libxl: libxl__object_to_json() to json-c

[Anthony PERARD <anthony@xxxxxxxxxxxxxx>]

On Wed, Aug 27, 2025 at 06:59:14PM +0100, Andrew Cooper wrote:
> On 27/08/2025 6:51 pm, Jason Andryuk wrote:
> > On 2025-08-08 10:55, Anthony PERARD wrote:
> >> From: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> >>
> >> - libxl changes:
> >>
> >> While doing so, we rename all "*_gen_json" function to "*_gen_jso" as
> >> they have different prototype. All the function pointer are been cast
> >> to (libxl__gen_json_callback) by "gentypes.py" when generating
> >> "*_to_json()" functions.
> >>
> >> We also introduce a few more "*_gen_jso" functions for "int" and
> >> "bool" because we can't use json_object_*() functions from json-c
> >> directly like it's done with yajl.
> >>
> >> To make the generation of _libxl_types*json.[ch] with both YAJL and
> >> json-c we add "--libjsonc" to gentypes.py so it can generate
> >> functions/types for both.
> >>
> >> Also introducing "jsonc_json_gen_fn" in the IDL, to be able to point
> >> to a different function when using json-c.
> >>
> >> Also, don't export any of the new *_gen_jso() function, at the cost of
> >> having "_hidden" macro in semi-public headers.
> >>
> >> - xl changes:
> >>
> >> Also, rework the implementation of printf_info() in `xl` to avoid
> >> using libxl_domain_config_gen_json() which isn't available without
> >> YAJL. The implementation using "json_object" call
> >> libxl_domain_config_to_json() which generate a plain string of JSON,
> >> which we parse to add it to our own json; this avoid a dependency on
> >> the json library used by libxl.
> >>
> >> Signed-off-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> >
> >> @@ -358,6 +535,36 @@ int libxl__mac_parse_json(libxl__gc *gc, const
> >> libxl__json_object *o,
> >>       return libxl__parse_mac(libxl__json_object_get_string(o), *p);
> >>   }
> >>   +#ifdef HAVE_LIBJSONC
> >> +int libxl_hwcap_gen_jso(json_object **jso_r, libxl_hwcap *p)
> >> +{
> >> +    json_object *jso;
> >> +    int i;
> >> +    int rc = ERROR_FAIL;
> >> +
> >> +    jso = json_object_new_array();
> >> +    if (!jso) goto out;
> >> +
> >> +    for(i=0; i<4; i++) {
> >
> > typedef uint32_t libxl_hwcap[8];
> >
> > I see this is the same as the yajl implementation, but should this be 8?

Yeah, looks like it should be ARRAY_SIZE(p). But that would want fixing
in a different patch. And since no one complained about missing bits in
this json conversion, I guess it's not used.

We would need to also fix libxl__hwcap_parse_json(), to parse up to 8
items, and be able to deal with missing 4 of them.

> >
> > The remainder looks good:
> >
> > Reviewed-by: Jason Andryuk <jason.andryuk@xxxxxxx>
> 
> A tangent, but physinfo.hw_cap (which libxl_hwcap wraps) is as good as
> stack rubble.  It's not exactly random data, but it is a view of an
> internal Xen structure which has been reformatted multiple times before
> we even realised it was exported, then continued being reformatting it
> because it's a gross laying violation that noone could possibly be using
> anyway.

It's display in `xl info`, surely it's useful for someone :-), a long
long time ago since it seems to be inherited from `xm info`.

> It needs purging from libxl and the sysctl interface, and if that makes
> JSON easier, then lets get it done.

It would be more work, not less. Removing things from libxl's api is
annoying. But I guess we can look at removing that later.

Cheers,


-- 
Anthony PERARD