Re: [Xen-devel] [PATCH v4 18/21] libxlu: rework internal representation of setting

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Wei Liu writes ("[PATCH v4 18/21] libxlu: rework internal representation of 
setting"):
> This patches does following things:
...
> +void xlu__cfg_list_append(CfgParseContext *ctx,
> +                          XLU_ConfigValue *list,
> +                          char *atom)
> +{
> +    XLU_ConfigValue **new_val = NULL;
> +    XLU_ConfigValue *val = NULL;
>      if (ctx->err) return;
...
> -    if (set->nvalues >= set->avalues) {
> -        int new_avalues;
> -        char **new_values;
> -
> -        if (set->avalues > INT_MAX / 100) { ctx->err= ERANGE; return; }
> -        new_avalues= set->avalues * 4;
> -        new_values= realloc(set->values,
> -                            sizeof(*new_values) * new_avalues);
> -        if (!new_values) { ctx->err= errno; free(atom); return; }
> -        set->values= new_values;
> -        set->avalues= new_avalues;

This is a standard expanding-array pattern which arranges not to
realloc the array each time.

> -    }
> -    set->values[set->nvalues++]= atom;
> +    new_val = realloc(list->u.list.values,
> +                      sizeof(*new_val) * (list->u.list.nvalues+1));
> +    if (!new_val) goto xe;

But you replace it here with one which has quadradic performance.

I don't know whether people are going to specify lists with hundreds
or thousands of elements, but it doesn't seem impossible.

I think you should retain the existing avalues stuff.

Apart from that this all looks good to me.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel