Re: Re: [RFC 00/21] RFC: Generate parsexml/formatbuf functions based on directives

On Wed, Jun 10, 2020 at 09:20:28AM +0800, Shi Lei wrote: > > Last RFC: [https://www.redhat.com/archives/libvir-list/2020-April/msg00970.html] > In last RFC, I suggested we can generate object-model code based on relax-ng > files and Daniel gave it some comments. > > Follow the suggestion from Daniel, I make another try to generate parsexml/formatbuf > functions automatically. > > > ============ >  Directives > ============ > > Still, we need several directives that can direct a tool to generate functions. > These directives work on the declarations of structs. For example: > >     typedef struct _virNetworkDNSTxtDef virNetworkDNSTxtDef; >     typedef virNetworkDNSTxtDef *virNetworkDNSTxtDefPtr; >     struct _virNetworkDNSTxtDef {   /* genparse:concisehook, genformat */ >         char *name;                 /* xmlattr, required */ >         char *value;                /* xmlattr */ >     }; > >     typedef struct _virNetworkDNSSrvDef virNetworkDNSSrvDef; >     typedef virNetworkDNSSrvDef *virNetworkDNSSrvDefPtr; >     struct _virNetworkDNSSrvDef {   /* genparse:withhook, genformat */ >         char *service;              /* xmlattr */ >         char *protocol;             /* xmlattr */ >         char *domain;               /* xmlattr */ >         char *target;               /* xmlattr */ >         unsigned int port;          /* xmlattr */ >         unsigned int priority;      /* xmlattr */ >         unsigned int weight;        /* xmlattr */ >     }; > >     typedef struct _virNetworkDNSHostDef virNetworkDNSHostDef; >     typedef virNetworkDNSHostDef *virNetworkDNSHostDefPtr; >     struct _virNetworkDNSHostDef {  /* genparse:withhook, genformat */ >         virSocketAddr ip;           /* xmlattr */ >         size_t nnames; >         char **names;               /* xmlelem:hostname, array */ >     }; > >     typedef struct _virNetworkDNSForwarder virNetworkDNSForwarder; >     typedef virNetworkDNSForwarder *virNetworkDNSForwarderPtr; >     struct _virNetworkDNSForwarder {    /* genparse:withhook, genformat */ >         char *domain;                   /* xmlattr */ >         virSocketAddr addr;             /* xmlattr */ >     }; > >     typedef struct _virNetworkDNSDef virNetworkDNSDef; >     typedef virNetworkDNSDef *virNetworkDNSDefPtr; >     struct _virNetworkDNSDef {              /* genparse:withhook, genformat */ >         virTristateBool enable;                 /* xmlattr */ >         virTristateBool forwardPlainNames;      /* xmlattr */ >         size_t nforwarders; >         virNetworkDNSForwarderPtr forwarders;   /* xmlelem, array */ >         size_t ntxts; >         virNetworkDNSTxtDefPtr txts;            /* xmlelem, array */ >         size_t nsrvs; >         virNetworkDNSSrvDefPtr srvs;            /* xmlelem, array */ >         size_t nhosts; >         virNetworkDNSHostDefPtr hosts;          /* xmlelem, array */ >     }; > > > Explanation for these directives: > > >     - genparse[:withhook|concisehook] > >       Only work on a struct. >       Generate parsexml function for this struct only if 'genparse' is specified. >       The function name is based on the struct-name and suffixed with 'ParseXML'. >       E.g., for struct virNetworkDNSDef, its parsexml function is >       virNetworkDNSDefParseXML. > >       If a parsexml function includes some error-checking code, it needs a >       post-process hook to hold them. Specify 'withhook' or 'concisehook' to >       setup a hook point in the parsexml function. > >       Executing the tool manually can show the declaration of hook function. >       E.g. check declaration of 'virNetworkDNSDefParseXMLHook'. > >         # ./build-aux/generator/go show virNetworkDNSDef -kp > >         int >         virNetworkDNSDefParseXMLHook(xmlNodePtr node, >                                      virNetworkDNSDefPtr def, >                                      const char *instname, >                                      void *opaque, >                                      const char *enableStr, >                                      const char *forwardPlainNamesStr, >                                      int nForwarderNodes, >                                      int nTxtNodes, >                                      int nSrvNodes, >                                      int nHostNodes); > >       Some helper arguments (such as 'enableStr', 'nTxtNodes', etc.) are >       passed in to indicate the existence of fields. >       If these arguments are useless, specify 'concisehook' to omit them. > >       When 'genparse' is specified, clear function for this struct is also >       generated implicitly, it is because the generated parsexml function needs >       to call the clear function. > > >     - genformat > >       Only work on a struct. >       Generate formatbuf function for this struct only if 'genformat' is specified. >       The function name is based on struct-name and suffixed with 'FormatBuf'. > > >     - xmlattr[:thename] > >       Parse/Format the field as an XML attribute. >       If 'thename' is specified, use it as the XML attribute name; >       or use the filed name. > > >     - xmlelem[:thename] >       >       Parse/Format the field as a child struct. >       If 'thename' is specified, use it as the XML element name; >       or use the filed name. > > >     - array > >       Parse/Format the field as an array. >       Its related field is a counter, which name should follow the pattern: >       n + 'field_name'. > > >     - required > >       The field must have a corresponding item defined in XML. This all looks pretty reasonable and simple to understand to me. I think the complex one is going to arrive when we need to deal with discriminated unions. eg the   <forward mode='passthrough|nat|hostdev'> where the @mode attribute determines which child struct and elements are permitted.

> ======= >  Tool > ======= > > The Tool is based on libclang and its python-binding. > > It has three subcommands: 'list', 'show' and 'generate'. > The 'list' and 'show' are used for previewing generated code; > the 'generate' is prepared to be invoked by Makefile whenever the c header files > under 'src/conf' and 'src/util' have changed. It looks like libclang is available on all platforms that we need to support.  The python binding appears available except on RHEL-7, but even then we "pip install" it if required. Looking at the generator code you've provided, I think it makes sense to use libclang. Any code based on a custom parser would have been more complex than what you've done and less reliable. >  build-aux/generator/directive.py | 839 +++++++++++++++++++++++++++++++ >  build-aux/generator/go           |  14 + >  build-aux/generator/main.py      | 416 +++++++++++++++ >  build-aux/generator/utils.py     | 100 ++++ Once our xml generator is feature complete, this is fixed overhead, so... >  src/conf/network_conf.c          | 487 ++++-------------- >  src/conf/network_conf.h          |  54 +- These two are the really compelling parts of the diffstat. Overall I like the way this series looks. The code generator is naturally more complex than the current code, but the code it generates will be better than the hand written code because it will be behaving consistently with our desired spec. eg it will be capable of optimizing XML output to use self-closing <tag/> instead of empty <tag></tag>. It can guarantee that we have all XML escaping done correctly. It can ensure we're free'ing stuff in all code paths in a consistent manner, etc. Also the generator is fixed one time overhead, and that eliminates ongoing overhead of writing XML. Now that we're using struct annotations instead of XML RHG schema additions I think the effects are easier to follow. So overall, I think this idea of doing XML generators is a good one and will benefit us over the long term. 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 :|