Re: [libnftables PATCH 4/6] internal: add a selector for parsing ops

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Wed, Jan 08, 2014 at 01:31:44PM +0100, Arturo Borrero Gonzalez wrote:
> On 8 January 2014 00:29, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >
> > You should pass a callback function instead, eg.
> >
> > static int nft_chain_json_parse(struct nft_chain *c, const char *json,
> >                                 struct nft_parse_err *err,
> >                                 json_t *(*jsonbuilder)(const void *input_data,
> >                                                        const char *treename,
> >                                                        struct nft_parse_err *e))
> 
> Ok.
> 
> > But I don't understand yet what you save (in terms of lines of code)
> > by using this aproach.
> 
> I avoid doing something like:
> 
> nft_*_parse() {
> switch (type)
>  if XML return xml_parse()
>  if JSON return json_parse()
> }
> 
> nft_*_parse_file() {
> switch (type)
>  if XML return xml_parse_file()
>  if JSON return json_parse_file()
> }
> 
> We double the format switch, and also two functions per format are
> needed to do build and parsing.
> Total = 6 functions heavily duplicating code.
> 
> With my approach, we have 1 function that decides which format to
> parse, and a one function per format to build and do parsing.
> Total = 3 functions, no duplicate code.

I guess this is saving you code, but I think this abstraction needs to
be refined, your functions:

* nft_mxml_do_build_tree
* nft_mxml_do_build_tree_file

look almost the same, only difference is mxmlLoadString / mxmlLoadFile.

I really think you can save more code and make this look better if you
rework the internal nft_*_json_parse functions to receive the xml/json
trees, eg.

static int nft_set_json_parse(struct nft_set *s, json_t tree
                              struct nft_parse_err *err)
{
        ...
}

So you nft_set_do_parse() creates the tree and pass it to it.

You can also add an enum like:

enum {
        NFT_PARSE_BUFFER,
        NFT_PARSE_FILE,
};

json_t *nft_json_build_tree(uint32_t type, void *data)
{
        json_t *tree;

        switch (type) {
        case NFT_PARSE_BUFFER:
                tree = nft_json_build_tree(data, ...);
                break;
        case NFT_PARSE_FILE:
                tree = nft_json_build_tree_file(data, ...);
                break;
        }

        return tree;
}

that you can pass this enum to nft_set_do_parse() to indicate how the
tree need to be build, eg.

statiuc int nft_set_do_parse(..., uint32_t format)
{
        switch (format) {
        ...
        case NFT_PARSE_JSON:
                tree = nft_json_build_tree(type, data);
                break;
        ...
        }
        ...
}

Where data is the file descriptor / buffer area.