Re: [PATCH RFC V2 06/10] libxl_json: allow basic JSON type objects generation

[Wei Liu <wei.liu2@citrix.com>]

On Wed, Apr 23, 2014 at 11:30:03AM +0100, Ian Campbell wrote:
> On Wed, 2014-04-23 at 11:15 +0100, Wei Liu wrote:
> > On Tue, Apr 22, 2014 at 04:22:20PM +0100, Ian Campbell wrote:
> > > On Thu, 2014-04-17 at 12:13 +0100, Wei Liu wrote:
> > > > The original logic is that basic JSON types (number, string and null)
> > > > must be an element of JSON map or array. This assumption doesn't hold
> > > > true anymore when we need to return basic JSON types.
> > > > 
> > > > Returning basic JSON types is required for parsing number, string and
> > > > null objects back into libxl__json_object.
> > > > 
> > > > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > > > ---
> > > >  tools/libxl/libxl_json.c |   40 ++++++++++++++++++++++++++++++++--------
> > > >  1 file changed, 32 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/tools/libxl/libxl_json.c b/tools/libxl/libxl_json.c
> > > > index e2d5dbe..6b2946d 100644
> > > > --- a/tools/libxl/libxl_json.c
> > > > +++ b/tools/libxl/libxl_json.c
> > > > @@ -629,8 +629,14 @@ static int json_callback_null(void *opaque)
> > > >  
> > > >      obj = libxl__json_object_alloc(ctx->gc, JSON_NULL);
> > > >  
> > > > -    if (libxl__json_object_append_to(ctx->gc, obj, ctx->current) == -1) {
> > > > -        return 0;
> > > > +    if (ctx->current) {
> > > > +        if (libxl__json_object_append_to(ctx->gc, obj, ctx->current) == -1) {
> > > > +            return 0;
> > > > +        }
> > > > +    }
> > > > +
> > > > +    if (ctx->head == NULL) {
> > > > +        ctx->head = obj;
> > > 
> > > It seems that this pattern is now pretty much universal on all callers
> > > of libxl__json_object_append_to. Perhaps much of this functionality
> > > should be pushed down into the helper (which might now need to take ctx
> > > and not ctx->current).
> > > 
> > 
> > But not every new allocated object needs to be appended to some other
> > object, so pushing that functionality to libxl__json_object_append_to is
> > not correct.
> 
> ???
> 
> AFAICT every call of libxl__json_object_append_to after this patch now
> has exactly the same boiler plate around it. The need append or not is a
> property of the current context (in a map) not of the current object
> being handled.
> 

But setting ctx->head doesn't really belong to append_to does it? The
semantic looks wrong to me.

Are you suggesting me change the semantic of libxl__json_object_append_to
to "append this object to current context" instead of "append this
object to the map or array pointed to by 'current'"?

Wei.

> > > The existing callers seem to all set ctx->current too, just before the
> > > ctx->head == NULL check. Why don't these versions need that too?
> > > 
> > 
> > Because "current" is used to reference to "the array or map that we're
> > currently in", so that we can call
> > libxl__json_object_append_to(current).
> > 
> > Not very useful to set "current" for basic types because no other
> > objects can be appended to them.
> 
> OK, I think this bit could be left in the relevant callers rather than
> being made generic. Or it could be a parameter to object_append_to (e.g.
> bool_t is_a_container, or pass the JSON_*).
> 
> Ian.