[RFCv2 00/46] RFC: Generate parsexml/formatbuf functions based on directives

Shi Lei posted 46 patches 3 years, 7 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20200904033538.418579-1-shi_lei@massclouds.com
There is a newer version of this series
meson.build                      |    5 +
po/POTFILES.in                   |    3 +
scripts/meson.build              |    8 +
scripts/xmlgen/directive.py      | 1115 ++++++++++++++++++++
scripts/xmlgen/go                |    7 +
scripts/xmlgen/main.py           |  439 ++++++++
scripts/xmlgen/utils.py          |  121 +++
src/conf/domain_conf.c           | 1650 +++++++++---------------------
src/conf/domain_conf.h           |  179 ++--
src/conf/meson.build             |   41 +
src/conf/network_conf.c          |  467 ++-------
src/conf/network_conf.h          |   54 +-
src/conf/virconftypes.h          |   18 +
src/libvirt_private.syms         |    9 +
src/meson.build                  |    6 +
src/qemu/qemu_command.c          |    4 +
src/qemu/qemu_driver.c           |    2 +
src/qemu/qemu_hotplug.c          |    2 +
src/qemu/qemu_migration_cookie.c |    1 +
src/qemu/qemu_process.c          |    5 +
src/qemu/qemu_validate.c         |    2 +
src/util/virsocketaddr.c         |   42 +
src/util/virsocketaddr.h         |   26 +-
src/util/virstring.c             |   57 ++
src/util/virstring.h             |    9 +
src/util/virxml.c                |  105 ++
src/util/virxml.h                |    6 +
src/vmx/vmx.c                    |    1 +
tests/meson.build                |    1 +
tools/meson.build                |    2 +
30 files changed, 2738 insertions(+), 1649 deletions(-)
create mode 100644 scripts/xmlgen/directive.py
create mode 100755 scripts/xmlgen/go
create mode 100755 scripts/xmlgen/main.py
create mode 100644 scripts/xmlgen/utils.py
[RFCv2 00/46] RFC: Generate parsexml/formatbuf functions based on directives
Posted by Shi Lei 3 years, 7 months ago
V1 here: [https://www.redhat.com/archives/libvir-list/2020-June/msg00357.html]

Differ from V1:

  * Move the generator into scripts/xmlgen and rename it 'xmlgen'.

  * Declare virXMLChildNode and virXMLChildNodeSet in libvirt_private.syms.

  * Replace VIR_FREE with g_free and VIR_ALLOC[_N] with g_new0.

  * Adjust virReportError to avoid unnecessary translation.

  * Remove the macro VIR_USED and use G_GNUC_UNUSED to declare arguments.

  * When parsing string member, assign value to it directly instead of
    using middle variable.

  * Don't set libclang_path. Just use python-clang's default setting.

  * Use virEscapeString for escaping xml characters.

  * Enable directive 'genformat' with a parameter to support separation mode.

  * Add directive 'xmlswitch' and 'xmlgroup' to support discriminated unions. 

  * Allow directive 'array' and 'specified' to carry with a parameter,
    which specifies its counterpart explicitly.

  * Enable directive 'xmlattr' with path.

  * Add directive 'formatflag' and 'formathook'.


For those new and changed directives, illustrate them by an example:

struct _virDomainGraphicsAuthDef {  /* genparse, genformat:separate */
    char *passwd;                   /* xmlattr, formatflag:VIR_DOMAIN_DEF_FORMAT_SECURE */
    bool expires;
    time_t validTo;                 /* xmlattr:passwdValidTo, specified:expires */
    virDomainGraphicsAuthConnectedType connected;   /* xmlattr */
};

struct _virDomainGraphicsListenDef {    /* genparse:withhook, genformat */
    virDomainGraphicsListenType type;   /* xmlattr */
    char *address;                      /* xmlattr, formathook */
    char *network;                      /* xmlattr, formatflag:VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK */
    char *socket;                       /* xmlattr, formathook */
    int fromConfig;                     /* xmlattr, formatflag:%VIR_DOMAIN_DEF_FORMAT_STATUS */
    bool autoGenerated;                 /* xmlattr, formatflag:%VIR_DOMAIN_DEF_FORMAT_STATUS */
};

struct _virDomainGraphicsSDLDef {   /* genparse, genformat:separate */
    char *display;                  /* xmlattr */
    char *xauth;                    /* xmlattr */
    bool fullscreen;                /* xmlattr */
    virTristateBool gl;             /* xmlattr:gl/enable */
};

struct _virDomainGraphicsDef {  /* genparse:concisehook, genformat */
    virObjectPtr privateData;
    virDomainGraphicsType type;                         /* xmlattr */

    size_t nListens;
    virDomainGraphicsListenDefPtr listens;  /* xmlelem, array:nListens */

    union {
        virDomainGraphicsSDLDef sdl;                    /* xmlgroup */
        virDomainGraphicsVNCDef vnc;                    /* xmlgroup */
        virDomainGraphicsRDPDef rdp;                    /* xmlgroup */
        virDomainGraphicsDesktopDef desktop;            /* xmlgroup */
        virDomainGraphicsSpiceDef spice;                /* xmlgroup */
        virDomainGraphicsEGLHeadlessDef egl_headless;   /* xmlgroup */
    } data;                                             /* xmlswitch:type */
};


Explanation for these directives:

    - genformat[:separate|onlyattrs|onlyelems]

      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'.

      When 'genformat:separate' is specified, generate two formatbuf functions
      rather than a single full-mode formatbuf function.
      One for formatting attributes and another for formatting elements.
      These function names are based on struct-name and suffixed with 'FormatAttr'
      and 'FormatElem' respectively.

      The 'onlyattrs' and 'onlyelems' are just like 'separate', but only
      generate one of those two functions according to its denotation.

    - xmlattr[:[parentname/]thename]

      Parse/Format the field as an XML attribute or
      attribute wrapped by an XML element.
      If only 'thename' is specified, use it as the XML attribute name;
      or use the filed name.
      The 'parentname' is the name of the attribute's parent element.
      If 'parentname/thename' is specified, the corresponding form is
      <parentname thename='..' />.

    - xmlgroup

      The field is a struct, but its corresponding form in XML is a group
      rather than an element.

    - xmlswitch:thename

      Only for discriminated union. 'thename' is the name of its relative enum.
      The name of each union member should match a shortname of the enum.

    - array[:countername]

      Parse/Format the field as an array.
      Each array field must have an related counter field, which name is
      specified by 'countername'.
      If 'countername' is omitted, follow the pattern:
      n + 'field_name'.

    - specified[:thename]

      This field has an related field to indicate its existence, and
      'thename' specifies the name of this related field.
      When 'thename' is omitted, follow the pattern:
      'field_name' + '_specified'.

    - formatflag:[!|%]flag

      This field will be formatted and written out to XML only if the 'flag'
      hits a target flagset.
      The target flagset is passed into the formatbuf function through the
      argument 'opaque'.

      Adding a '!' before 'flag' means NOT hitting.

      Adding a '%' before 'flag' means that flag hitting-check is the unique
      condition for formatting this field. For example,
      for 'passwd' in 'virDomainGraphicsAuthDef', the directive is:

        formatflag:VIR_DOMAIN_DEF_FORMAT_SECURE

      then the generated code:

        if (def->passwd && (virXMLFlag(opaque) & VIR_DOMAIN_DEF_FORMAT_SECURE))
            virBufferEscapeString(buf, " passwd='%s'", def->passwd);

      If '%' is inserted like this:

        formatflag:%VIR_DOMAIN_DEF_FORMAT_SECURE

      then the generated code:

        if ((virXMLFlag(opaque) & VIR_DOMAIN_DEF_FORMAT_SECURE))
            virBufferEscapeString(buf, " passwd='%s'", def->passwd);

    - formathook
      Introduce hooks to handle the field if xmlgen can't deal with it now.

      E.g., virDomainGraphicsListenDef have two fields with 'formathook',
      which are 'address' and 'socket'.
      The xmlgen will generate the declaration of some hooks for formatting
      these fields and developers should implement them.

      1) Check the declaration of hook by a commandline.

        # ./scripts/xmlgen/go show virDomainGraphicsListenDef -kf

        int
        virDomainGraphicsListenDefFormatHook(const virDomainGraphicsListenDef *def,
                                             const void *parent,
                                             const void *opaque,
                                             virBufferPtr addressBuf,
                                             virBufferPtr socketBuf);

        bool
        virDomainGraphicsListenDefCheckHook(const virDomainGraphicsListenDef *def,
                                            const void *parent,
                                            void *opaque,
                                            bool result);

      2) Implement these two hooks in src/conf/domain_conf.c.

      2.1) virXXXFormatHook
        It is the hook for formatting field 'address' and 'socket'.
        The 'addressBuf' and 'socketBuf' are used for output destinations respectively.

      2.2) virXXXCheckHook
        For structs, the xmlgen generates virXXXCheck function to come with
        the virXXXFormatBuf. The virXXXCheck reports whether the corresponding
        XML element is null.

        The virXXXCheckHook intercepts the 'result' of virXXXCheck. It changes 'result'
        or just forwards it according to those fields with 'formathook'.

    Thanks!


Shi Lei (46):
  scripts: Add a tool to generate xml parse/format functions
  maint: Check python3-clang
  maint: Call xmlgen automatically when c-head-files change
  util: Add some xml-helper-functions to cooperate with xmlgen
  util: Add helper functions for 'bool' and 'time_t' and cooperate with
    xmlgen
  util: Add parsexml/formatbuf helper functions for virSocketAddr
  conf: Extract error-checking code from virNetworkDNSTxtDefParseXML
  conf: Replace virNetworkDNSTxtDefParseXML(hardcoded) with
    namesake(generated)
  conf: Generate virNetworkDNSTxtDefFormatBuf
  conf: Extract error-checking code from virNetworkDNSSrvDefParseXML
  conf: Replace virNetworkDNSSrvDefParseXML(hardcoded) with
    namesake(generated)
  conf: Generate virNetworkDNSSrvDefFormatBuf
  conf: Extract error-checking code from virNetworkDNSHostDefParseXML
  conf: Replace virNetworkDNSHostDefParseXML(hardcoded) with
    namesake(generated)
  conf: Generate virNetworkDNSHostDefFormatBuf
  conf: Extract virNetworkDNSForwarderParseXML from
    virNetworkDNSParseXML
  conf: Replace virNetworkDNSForwarderParseXML(hardcoded) with
    namesake(generated)
  conf: Generate virNetworkDNSForwarderFormatBuf
  conf: Extract error-checking code from virNetworkDNSDefParseXML
  conf: Replace virNetworkDNSDefParseXML(hardcoded) with
    namesake(generated)
  conf: Generate virNetworkDNSDefFormatBuf
  conf: Extract embedded structs from virDomainGraphicsDef as standalone
    structs
  conf: Replace virDomainGraphicsDefParseXMLSDL(hardcoded) with
    virDomainGraphicsSDLDefParseXML(generated)
  conf: Generate format functions for virDomainGraphicsSDLDef
  conf: Replace virDomainGraphicsAuthDefParseXML(hardcoded) with
    namesake(generated)
  conf: Generate format functions for virDomainGraphicsAuthDef
  conf: Extract error-checking code from virDomainGraphicsDefParseXMLVNC
  conf: Replace virDomainGraphicsDefParseXMLVNC(hardcoded) with
    virDomainGraphicsVNCDefParseXML(generated)
  conf: Generate virDomainGraphicsVNCDefFormatAttr
  conf: Extract error-checking code from virDomainGraphicsDefParseXMLRDP
  conf: Replace virDomainGraphicsDefParseXMLRDP(hardcoded) with
    virDomainGraphicsRDPDefParseXML(generated)
  conf: Generate virDomainGraphicsRDPDefFormatAttr
  conf: Replace virDomainGraphicsDefParseXMLDesktop(hardcoded) with
    virDomainGraphicsDesktopDefParseXML(generated)
  conf: Generate virDomainGraphicsDesktopDefFormatAttr
  conf: Add virSpiceChannelDef to help to parse the member 'channels' of
    virDomainGraphicsSpiceDef
  conf: Extract error-checking code from
    virDomainGraphicsDefParseXMLSpice
  conf: Replace virDomainGraphicsDefParseXMLSpice(hardcoded) with
    virDomainGraphicsSpiceDefParseXML(generated)
  conf: Generate virDomainGraphicsSpiceDefFormatElem and
    virDomainGraphicsSpiceDefFormatAttr
  conf: Replace virDomainGraphicsDefParseXMLEGLHeadless(hardcoded) with
    virDomainGraphicsEGLHeadlessDefParseXML(generated)
  conf: Generate virDomainGraphicsEGLHeadlessDefFormatElem
  conf: Extract error-checking code from
    virDomainGraphicsListenDefParseXML
  conf: Replace virDomainGraphicsListenDefParseXML(hardcoded) with
    namesake(generated)
  conf: Generate virDomainGraphicsListenDefFormatBuf
  conf: Extract error-checking code from virDomainGraphicsDefParseXML
  conf: Replace virDomainGraphicsDefParseXML(hardcoded) with
    namesake(generated)
  conf: Replace virDomainGraphicsDefFormat(hardcoded) with
    virDomainGraphicsDefFormatBuf(generated)

 meson.build                      |    5 +
 po/POTFILES.in                   |    3 +
 scripts/meson.build              |    8 +
 scripts/xmlgen/directive.py      | 1115 ++++++++++++++++++++
 scripts/xmlgen/go                |    7 +
 scripts/xmlgen/main.py           |  439 ++++++++
 scripts/xmlgen/utils.py          |  121 +++
 src/conf/domain_conf.c           | 1650 +++++++++---------------------
 src/conf/domain_conf.h           |  179 ++--
 src/conf/meson.build             |   41 +
 src/conf/network_conf.c          |  467 ++-------
 src/conf/network_conf.h          |   54 +-
 src/conf/virconftypes.h          |   18 +
 src/libvirt_private.syms         |    9 +
 src/meson.build                  |    6 +
 src/qemu/qemu_command.c          |    4 +
 src/qemu/qemu_driver.c           |    2 +
 src/qemu/qemu_hotplug.c          |    2 +
 src/qemu/qemu_migration_cookie.c |    1 +
 src/qemu/qemu_process.c          |    5 +
 src/qemu/qemu_validate.c         |    2 +
 src/util/virsocketaddr.c         |   42 +
 src/util/virsocketaddr.h         |   26 +-
 src/util/virstring.c             |   57 ++
 src/util/virstring.h             |    9 +
 src/util/virxml.c                |  105 ++
 src/util/virxml.h                |    6 +
 src/vmx/vmx.c                    |    1 +
 tests/meson.build                |    1 +
 tools/meson.build                |    2 +
 30 files changed, 2738 insertions(+), 1649 deletions(-)
 create mode 100644 scripts/xmlgen/directive.py
 create mode 100755 scripts/xmlgen/go
 create mode 100755 scripts/xmlgen/main.py
 create mode 100644 scripts/xmlgen/utils.py

-- 
2.25.1


Re:[RFCv2 00/46] RFC: Generate parsexml/formatbuf functions based on directives
Posted by 石磊 3 years, 5 months ago
Polite ping.

Regards,
Shi Lei


From: "Shi Lei" <shi_lei@massclouds.com>
Date: 2020-09-04 11:34:52
To:  libvir-list@redhat.com
Subject: [RFCv2 00/46] RFC: Generate parsexml/formatbuf functions based on directives>V1 here: [https://www.redhat.com/archives/libvir-list/2020-June/msg00357.html]
>
>Differ from V1:
>
>  * Move the generator into scripts/xmlgen and rename it 'xmlgen'.
>
>  * Declare virXMLChildNode and virXMLChildNodeSet in libvirt_private.syms.
>
>  * Replace VIR_FREE with g_free and VIR_ALLOC[_N] with g_new0.
>
>  * Adjust virReportError to avoid unnecessary translation.
>
>  * Remove the macro VIR_USED and use G_GNUC_UNUSED to declare arguments.
>
>  * When parsing string member, assign value to it directly instead of
>    using middle variable.
>
>  * Don't set libclang_path. Just use python-clang's default setting.
>
>  * Use virEscapeString for escaping xml characters.
>
>  * Enable directive 'genformat' with a parameter to support separation mode.
>
>  * Add directive 'xmlswitch' and 'xmlgroup' to support discriminated unions. 
>
>  * Allow directive 'array' and 'specified' to carry with a parameter,
>    which specifies its counterpart explicitly.
>
>  * Enable directive 'xmlattr' with path.
>
>  * Add directive 'formatflag' and 'formathook'.
>
>
>For those new and changed directives, illustrate them by an example:
>
>struct _virDomainGraphicsAuthDef {  /* genparse, genformat:separate */
>    char *passwd;                   /* xmlattr, formatflag:VIR_DOMAIN_DEF_FORMAT_SECURE */
>    bool expires;
>    time_t validTo;                 /* xmlattr:passwdValidTo, specified:expires */
>    virDomainGraphicsAuthConnectedType connected;   /* xmlattr */
>};
>
>struct _virDomainGraphicsListenDef {    /* genparse:withhook, genformat */
>    virDomainGraphicsListenType type;   /* xmlattr */
>    char *address;                      /* xmlattr, formathook */
>    char *network;                      /* xmlattr, formatflag:VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK */
>    char *socket;                       /* xmlattr, formathook */
>    int fromConfig;                     /* xmlattr, formatflag:%VIR_DOMAIN_DEF_FORMAT_STATUS */
>    bool autoGenerated;                 /* xmlattr, formatflag:%VIR_DOMAIN_DEF_FORMAT_STATUS */
>};
>
>struct _virDomainGraphicsSDLDef {   /* genparse, genformat:separate */
>    char *display;                  /* xmlattr */
>    char *xauth;                    /* xmlattr */
>    bool fullscreen;                /* xmlattr */
>    virTristateBool gl;             /* xmlattr:gl/enable */
>};
>
>struct _virDomainGraphicsDef {  /* genparse:concisehook, genformat */
>    virObjectPtr privateData;
>    virDomainGraphicsType type;                         /* xmlattr */
>
>    size_t nListens;
>    virDomainGraphicsListenDefPtr listens;  /* xmlelem, array:nListens */
>
>    union {
>        virDomainGraphicsSDLDef sdl;                    /* xmlgroup */
>        virDomainGraphicsVNCDef vnc;                    /* xmlgroup */
>        virDomainGraphicsRDPDef rdp;                    /* xmlgroup */
>        virDomainGraphicsDesktopDef desktop;            /* xmlgroup */
>        virDomainGraphicsSpiceDef spice;                /* xmlgroup */
>        virDomainGraphicsEGLHeadlessDef egl_headless;   /* xmlgroup */
>    } data;                                             /* xmlswitch:type */
>};
>
>
>Explanation for these directives:
>
>    - genformat[:separate|onlyattrs|onlyelems]
>
>      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'.
>
>      When 'genformat:separate' is specified, generate two formatbuf functions
>      rather than a single full-mode formatbuf function.
>      One for formatting attributes and another for formatting elements.
>      These function names are based on struct-name and suffixed with 'FormatAttr'
>      and 'FormatElem' respectively.
>
>      The 'onlyattrs' and 'onlyelems' are just like 'separate', but only
>      generate one of those two functions according to its denotation.
>
>    - xmlattr[:[parentname/]thename]
>
>      Parse/Format the field as an XML attribute or
>      attribute wrapped by an XML element.
>      If only 'thename' is specified, use it as the XML attribute name;
>      or use the filed name.
>      The 'parentname' is the name of the attribute's parent element.
>      If 'parentname/thename' is specified, the corresponding form is
>      <parentname thename='..' />.
>
>    - xmlgroup
>
>      The field is a struct, but its corresponding form in XML is a group
>      rather than an element.
>
>    - xmlswitch:thename
>
>      Only for discriminated union. 'thename' is the name of its relative enum.
>      The name of each union member should match a shortname of the enum.
>
>    - array[:countername]
>
>      Parse/Format the field as an array.
>      Each array field must have an related counter field, which name is
>      specified by 'countername'.
>      If 'countername' is omitted, follow the pattern:
>      n + 'field_name'.
>
>    - specified[:thename]
>
>      This field has an related field to indicate its existence, and
>      'thename' specifies the name of this related field.
>      When 'thename' is omitted, follow the pattern:
>      'field_name' + '_specified'.
>
>    - formatflag:[!|%]flag
>
>      This field will be formatted and written out to XML only if the 'flag'
>      hits a target flagset.
>      The target flagset is passed into the formatbuf function through the
>      argument 'opaque'.
>
>      Adding a '!' before 'flag' means NOT hitting.
>
>      Adding a '%' before 'flag' means that flag hitting-check is the unique
>      condition for formatting this field. For example,
>      for 'passwd' in 'virDomainGraphicsAuthDef', the directive is:
>
>        formatflag:VIR_DOMAIN_DEF_FORMAT_SECURE
>
>      then the generated code:
>
>        if (def->passwd && (virXMLFlag(opaque) & VIR_DOMAIN_DEF_FORMAT_SECURE))
>            virBufferEscapeString(buf, " passwd='%s'", def->passwd);
>
>      If '%' is inserted like this:
>
>        formatflag:%VIR_DOMAIN_DEF_FORMAT_SECURE
>
>      then the generated code:
>
>        if ((virXMLFlag(opaque) & VIR_DOMAIN_DEF_FORMAT_SECURE))
>            virBufferEscapeString(buf, " passwd='%s'", def->passwd);
>
>    - formathook
>      Introduce hooks to handle the field if xmlgen can't deal with it now.
>
>      E.g., virDomainGraphicsListenDef have two fields with 'formathook',
>      which are 'address' and 'socket'.
>      The xmlgen will generate the declaration of some hooks for formatting
>      these fields and developers should implement them.
>
>      1) Check the declaration of hook by a commandline.
>
>        # ./scripts/xmlgen/go show virDomainGraphicsListenDef -kf
>
>        int
>        virDomainGraphicsListenDefFormatHook(const virDomainGraphicsListenDef *def,
>                                             const void *parent,
>                                             const void *opaque,
>                                             virBufferPtr addressBuf,
>                                             virBufferPtr socketBuf);
>
>        bool
>        virDomainGraphicsListenDefCheckHook(const virDomainGraphicsListenDef *def,
>                                            const void *parent,
>                                            void *opaque,
>                                            bool result);
>
>      2) Implement these two hooks in src/conf/domain_conf.c.
>
>      2.1) virXXXFormatHook
>        It is the hook for formatting field 'address' and 'socket'.
>        The 'addressBuf' and 'socketBuf' are used for output destinations respectively.
>
>      2.2) virXXXCheckHook
>        For structs, the xmlgen generates virXXXCheck function to come with
>        the virXXXFormatBuf. The virXXXCheck reports whether the corresponding
>        XML element is null.
>
>        The virXXXCheckHook intercepts the 'result' of virXXXCheck. It changes 'result'
>        or just forwards it according to those fields with 'formathook'.
>
>    Thanks!
>
>
>Shi Lei (46):
>  scripts: Add a tool to generate xml parse/format functions
>  maint: Check python3-clang
>  maint: Call xmlgen automatically when c-head-files change
>  util: Add some xml-helper-functions to cooperate with xmlgen
>  util: Add helper functions for 'bool' and 'time_t' and cooperate with
>    xmlgen
>  util: Add parsexml/formatbuf helper functions for virSocketAddr
>  conf: Extract error-checking code from virNetworkDNSTxtDefParseXML
>  conf: Replace virNetworkDNSTxtDefParseXML(hardcoded) with
>    namesake(generated)
>  conf: Generate virNetworkDNSTxtDefFormatBuf
>  conf: Extract error-checking code from virNetworkDNSSrvDefParseXML
>  conf: Replace virNetworkDNSSrvDefParseXML(hardcoded) with
>    namesake(generated)
>  conf: Generate virNetworkDNSSrvDefFormatBuf
>  conf: Extract error-checking code from virNetworkDNSHostDefParseXML
>  conf: Replace virNetworkDNSHostDefParseXML(hardcoded) with
>    namesake(generated)
>  conf: Generate virNetworkDNSHostDefFormatBuf
>  conf: Extract virNetworkDNSForwarderParseXML from
>    virNetworkDNSParseXML
>  conf: Replace virNetworkDNSForwarderParseXML(hardcoded) with
>    namesake(generated)
>  conf: Generate virNetworkDNSForwarderFormatBuf
>  conf: Extract error-checking code from virNetworkDNSDefParseXML
>  conf: Replace virNetworkDNSDefParseXML(hardcoded) with
>    namesake(generated)
>  conf: Generate virNetworkDNSDefFormatBuf
>  conf: Extract embedded structs from virDomainGraphicsDef as standalone
>    structs
>  conf: Replace virDomainGraphicsDefParseXMLSDL(hardcoded) with
>    virDomainGraphicsSDLDefParseXML(generated)
>  conf: Generate format functions for virDomainGraphicsSDLDef
>  conf: Replace virDomainGraphicsAuthDefParseXML(hardcoded) with
>    namesake(generated)
>  conf: Generate format functions for virDomainGraphicsAuthDef
>  conf: Extract error-checking code from virDomainGraphicsDefParseXMLVNC
>  conf: Replace virDomainGraphicsDefParseXMLVNC(hardcoded) with
>    virDomainGraphicsVNCDefParseXML(generated)
>  conf: Generate virDomainGraphicsVNCDefFormatAttr
>  conf: Extract error-checking code from virDomainGraphicsDefParseXMLRDP
>  conf: Replace virDomainGraphicsDefParseXMLRDP(hardcoded) with
>    virDomainGraphicsRDPDefParseXML(generated)
>  conf: Generate virDomainGraphicsRDPDefFormatAttr
>  conf: Replace virDomainGraphicsDefParseXMLDesktop(hardcoded) with
>    virDomainGraphicsDesktopDefParseXML(generated)
>  conf: Generate virDomainGraphicsDesktopDefFormatAttr
>  conf: Add virSpiceChannelDef to help to parse the member 'channels' of
>    virDomainGraphicsSpiceDef
>  conf: Extract error-checking code from
>    virDomainGraphicsDefParseXMLSpice
>  conf: Replace virDomainGraphicsDefParseXMLSpice(hardcoded) with
>    virDomainGraphicsSpiceDefParseXML(generated)
>  conf: Generate virDomainGraphicsSpiceDefFormatElem and
>    virDomainGraphicsSpiceDefFormatAttr
>  conf: Replace virDomainGraphicsDefParseXMLEGLHeadless(hardcoded) with
>    virDomainGraphicsEGLHeadlessDefParseXML(generated)
>  conf: Generate virDomainGraphicsEGLHeadlessDefFormatElem
>  conf: Extract error-checking code from
>    virDomainGraphicsListenDefParseXML
>  conf: Replace virDomainGraphicsListenDefParseXML(hardcoded) with
>    namesake(generated)
>  conf: Generate virDomainGraphicsListenDefFormatBuf
>  conf: Extract error-checking code from virDomainGraphicsDefParseXML
>  conf: Replace virDomainGraphicsDefParseXML(hardcoded) with
>    namesake(generated)
>  conf: Replace virDomainGraphicsDefFormat(hardcoded) with
>    virDomainGraphicsDefFormatBuf(generated)
>
> meson.build                      |    5 +
> po/POTFILES.in                   |    3 +
> scripts/meson.build              |    8 +
> scripts/xmlgen/directive.py      | 1115 ++++++++++++++++++++
> scripts/xmlgen/go                |    7 +
> scripts/xmlgen/main.py           |  439 ++++++++
> scripts/xmlgen/utils.py          |  121 +++
> src/conf/domain_conf.c           | 1650 +++++++++---------------------
> src/conf/domain_conf.h           |  179 ++--
> src/conf/meson.build             |   41 +
> src/conf/network_conf.c          |  467 ++-------
> src/conf/network_conf.h          |   54 +-
> src/conf/virconftypes.h          |   18 +
> src/libvirt_private.syms         |    9 +
> src/meson.build                  |    6 +
> src/qemu/qemu_command.c          |    4 +
> src/qemu/qemu_driver.c           |    2 +
> src/qemu/qemu_hotplug.c          |    2 +
> src/qemu/qemu_migration_cookie.c |    1 +
> src/qemu/qemu_process.c          |    5 +
> src/qemu/qemu_validate.c         |    2 +
> src/util/virsocketaddr.c         |   42 +
> src/util/virsocketaddr.h         |   26 +-
> src/util/virstring.c             |   57 ++
> src/util/virstring.h             |    9 +
> src/util/virxml.c                |  105 ++
> src/util/virxml.h                |    6 +
> src/vmx/vmx.c                    |    1 +
> tests/meson.build                |    1 +
> tools/meson.build                |    2 +
> 30 files changed, 2738 insertions(+), 1649 deletions(-)
> create mode 100644 scripts/xmlgen/directive.py
> create mode 100755 scripts/xmlgen/go
> create mode 100755 scripts/xmlgen/main.py
> create mode 100644 scripts/xmlgen/utils.py
>
>-- 
>2.25.1
>
Re: [RFCv2 00/46] RFC: Generate parsexml/formatbuf functions based on directives
Posted by Daniel P. Berrangé 3 years, 4 months ago
On Fri, Sep 04, 2020 at 11:34:52AM +0800, Shi Lei wrote:
> V1 here: [https://www.redhat.com/archives/libvir-list/2020-June/msg00357.html]
> 
> Differ from V1:
> 
>   * Move the generator into scripts/xmlgen and rename it 'xmlgen'.
> 
>   * Declare virXMLChildNode and virXMLChildNodeSet in libvirt_private.syms.
> 
>   * Replace VIR_FREE with g_free and VIR_ALLOC[_N] with g_new0.
> 
>   * Adjust virReportError to avoid unnecessary translation.
> 
>   * Remove the macro VIR_USED and use G_GNUC_UNUSED to declare arguments.
> 
>   * When parsing string member, assign value to it directly instead of
>     using middle variable.
> 
>   * Don't set libclang_path. Just use python-clang's default setting.
> 
>   * Use virEscapeString for escaping xml characters.
> 
>   * Enable directive 'genformat' with a parameter to support separation mode.
> 
>   * Add directive 'xmlswitch' and 'xmlgroup' to support discriminated unions. 
> 
>   * Allow directive 'array' and 'specified' to carry with a parameter,
>     which specifies its counterpart explicitly.
> 
>   * Enable directive 'xmlattr' with path.
> 
>   * Add directive 'formatflag' and 'formathook'.

I'm very sorry for the extremely long delay in reviewing this new
series.

Overall I like this series and would like to see how we can focus on
getting at least part of it merged.  It is interesting to see how it
is forcing a separation of the parsing and validation of data. I
also like that it is making the code more consistent in style.

The main blocker I think is that we need to temporarily commit the
generated files to git, and not run the generator during normal
builds, until we ditch RHEL-7 support in April 2021.  This should
be quite easy to deal with.

The next thing is that wierd need to insert a "_NONE" field in to
all the enums. I'm not understanding what reqiures this, but I think
we need to come up with a different solution to whatever the problem
is.  It looks like this only affects the DomainGraphics conversion
in your series. So we could start by only merging the Network conversion
patches, until we figure out a solution to avoid adding _NONE fields.

That's it for the main blockers to merge from my POV.

After that I think a priority is to get some test coverage of the
generator script, and to add the docs about the how the generator
annotations work. This can be done after the initial merge.

Then there's a large ongoing work to actually convert everything,
but there's no rush to finish that, as you've shown that we can
do it incrementally.

So if you want to re-post an update of the series with the few
blocking items addressed, then personally I'd look at trying to
merge that unless other people have objections they want to
raise that can't be solved after merging the initial support.

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 :|

Re: [RFCv2 00/46] RFC: Generate parsexml/formatbuf functions based on directives
Posted by Daniel P. Berrangé 3 years, 4 months ago
On Fri, Sep 04, 2020 at 11:34:52AM +0800, Shi Lei wrote:
> V1 here: [https://www.redhat.com/archives/libvir-list/2020-June/msg00357.html]


> For those new and changed directives, illustrate them by an example:
> 
> struct _virDomainGraphicsAuthDef {  /* genparse, genformat:separate */
>     char *passwd;                   /* xmlattr, formatflag:VIR_DOMAIN_DEF_FORMAT_SECURE */
>     bool expires;
>     time_t validTo;                 /* xmlattr:passwdValidTo, specified:expires */
>     virDomainGraphicsAuthConnectedType connected;   /* xmlattr */
> };
> 
> struct _virDomainGraphicsListenDef {    /* genparse:withhook, genformat */
>     virDomainGraphicsListenType type;   /* xmlattr */
>     char *address;                      /* xmlattr, formathook */
>     char *network;                      /* xmlattr, formatflag:VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK */
>     char *socket;                       /* xmlattr, formathook */
>     int fromConfig;                     /* xmlattr, formatflag:%VIR_DOMAIN_DEF_FORMAT_STATUS */
>     bool autoGenerated;                 /* xmlattr, formatflag:%VIR_DOMAIN_DEF_FORMAT_STATUS */
> };
> 
> struct _virDomainGraphicsSDLDef {   /* genparse, genformat:separate */
>     char *display;                  /* xmlattr */
>     char *xauth;                    /* xmlattr */
>     bool fullscreen;                /* xmlattr */
>     virTristateBool gl;             /* xmlattr:gl/enable */
> };
> 
> struct _virDomainGraphicsDef {  /* genparse:concisehook, genformat */
>     virObjectPtr privateData;
>     virDomainGraphicsType type;                         /* xmlattr */
> 
>     size_t nListens;
>     virDomainGraphicsListenDefPtr listens;  /* xmlelem, array:nListens */
> 
>     union {
>         virDomainGraphicsSDLDef sdl;                    /* xmlgroup */
>         virDomainGraphicsVNCDef vnc;                    /* xmlgroup */
>         virDomainGraphicsRDPDef rdp;                    /* xmlgroup */
>         virDomainGraphicsDesktopDef desktop;            /* xmlgroup */
>         virDomainGraphicsSpiceDef spice;                /* xmlgroup */
>         virDomainGraphicsEGLHeadlessDef egl_headless;   /* xmlgroup */
>     } data;                                             /* xmlswitch:type */
> };
> 
> 
> Explanation for these directives:
> 
>     - genformat[:separate|onlyattrs|onlyelems]
> 
>       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'.
> 
>       When 'genformat:separate' is specified, generate two formatbuf functions
>       rather than a single full-mode formatbuf function.
>       One for formatting attributes and another for formatting elements.
>       These function names are based on struct-name and suffixed with 'FormatAttr'
>       and 'FormatElem' respectively.
> 
>       The 'onlyattrs' and 'onlyelems' are just like 'separate', but only
>       generate one of those two functions according to its denotation.
> 
>     - xmlattr[:[parentname/]thename]
> 
>       Parse/Format the field as an XML attribute or
>       attribute wrapped by an XML element.
>       If only 'thename' is specified, use it as the XML attribute name;
>       or use the filed name.
>       The 'parentname' is the name of the attribute's parent element.
>       If 'parentname/thename' is specified, the corresponding form is
>       <parentname thename='..' />.
> 
>     - xmlgroup
> 
>       The field is a struct, but its corresponding form in XML is a group
>       rather than an element.
> 
>     - xmlswitch:thename
> 
>       Only for discriminated union. 'thename' is the name of its relative enum.
>       The name of each union member should match a shortname of the enum.
> 
>     - array[:countername]
> 
>       Parse/Format the field as an array.
>       Each array field must have an related counter field, which name is
>       specified by 'countername'.
>       If 'countername' is omitted, follow the pattern:
>       n + 'field_name'.
> 
>     - specified[:thename]
> 
>       This field has an related field to indicate its existence, and
>       'thename' specifies the name of this related field.
>       When 'thename' is omitted, follow the pattern:
>       'field_name' + '_specified'.
> 
>     - formatflag:[!|%]flag
> 
>       This field will be formatted and written out to XML only if the 'flag'
>       hits a target flagset.
>       The target flagset is passed into the formatbuf function through the
>       argument 'opaque'.
> 
>       Adding a '!' before 'flag' means NOT hitting.
> 
>       Adding a '%' before 'flag' means that flag hitting-check is the unique
>       condition for formatting this field. For example,
>       for 'passwd' in 'virDomainGraphicsAuthDef', the directive is:
> 
>         formatflag:VIR_DOMAIN_DEF_FORMAT_SECURE
> 
>       then the generated code:
> 
>         if (def->passwd && (virXMLFlag(opaque) & VIR_DOMAIN_DEF_FORMAT_SECURE))
>             virBufferEscapeString(buf, " passwd='%s'", def->passwd);
> 
>       If '%' is inserted like this:
> 
>         formatflag:%VIR_DOMAIN_DEF_FORMAT_SECURE
> 
>       then the generated code:
> 
>         if ((virXMLFlag(opaque) & VIR_DOMAIN_DEF_FORMAT_SECURE))
>             virBufferEscapeString(buf, " passwd='%s'", def->passwd);
> 
>     - formathook
>       Introduce hooks to handle the field if xmlgen can't deal with it now.
> 
>       E.g., virDomainGraphicsListenDef have two fields with 'formathook',
>       which are 'address' and 'socket'.
>       The xmlgen will generate the declaration of some hooks for formatting
>       these fields and developers should implement them.
> 
>       1) Check the declaration of hook by a commandline.
> 
>         # ./scripts/xmlgen/go show virDomainGraphicsListenDef -kf
> 
>         int
>         virDomainGraphicsListenDefFormatHook(const virDomainGraphicsListenDef *def,
>                                              const void *parent,
>                                              const void *opaque,
>                                              virBufferPtr addressBuf,
>                                              virBufferPtr socketBuf);
> 
>         bool
>         virDomainGraphicsListenDefCheckHook(const virDomainGraphicsListenDef *def,
>                                             const void *parent,
>                                             void *opaque,
>                                             bool result);
> 
>       2) Implement these two hooks in src/conf/domain_conf.c.
> 
>       2.1) virXXXFormatHook
>         It is the hook for formatting field 'address' and 'socket'.
>         The 'addressBuf' and 'socketBuf' are used for output destinations respectively.
> 
>       2.2) virXXXCheckHook
>         For structs, the xmlgen generates virXXXCheck function to come with
>         the virXXXFormatBuf. The virXXXCheck reports whether the corresponding
>         XML element is null.
> 
>         The virXXXCheckHook intercepts the 'result' of virXXXCheck. It changes 'result'
>         or just forwards it according to those fields with 'formathook'.

It is probably a good idea to put all this doucmentation from
the cover letter  into a docs/xmlgenerator.rst file as it'll be
useful reference for developers in future.



>  meson.build                      |    5 +
>  po/POTFILES.in                   |    3 +
>  scripts/meson.build              |    8 +
>  scripts/xmlgen/directive.py      | 1115 ++++++++++++++++++++
>  scripts/xmlgen/go                |    7 +
>  scripts/xmlgen/main.py           |  439 ++++++++
>  scripts/xmlgen/utils.py          |  121 +++
>  src/conf/domain_conf.c           | 1650 +++++++++---------------------
>  src/conf/domain_conf.h           |  179 ++--
>  src/conf/meson.build             |   41 +
>  src/conf/network_conf.c          |  467 ++-------
>  src/conf/network_conf.h          |   54 +-
>  src/conf/virconftypes.h          |   18 +
>  src/libvirt_private.syms         |    9 +
>  src/meson.build                  |    6 +
>  src/qemu/qemu_command.c          |    4 +
>  src/qemu/qemu_driver.c           |    2 +
>  src/qemu/qemu_hotplug.c          |    2 +
>  src/qemu/qemu_migration_cookie.c |    1 +
>  src/qemu/qemu_process.c          |    5 +
>  src/qemu/qemu_validate.c         |    2 +
>  src/util/virsocketaddr.c         |   42 +
>  src/util/virsocketaddr.h         |   26 +-
>  src/util/virstring.c             |   57 ++
>  src/util/virstring.h             |    9 +
>  src/util/virxml.c                |  105 ++
>  src/util/virxml.h                |    6 +
>  src/vmx/vmx.c                    |    1 +
>  tests/meson.build                |    1 +
>  tools/meson.build                |    2 +

I think it'd be a good idea to have a test case to validate the
XML generator. For example create a simpe tests/xmlgen/demo.h that
illustrates all the different features we can use.

Then have tests/xmlgen/demo.generated.{c,h}, and the write a
test case that generates a new copy of demo.generated.{c,h}
and compares to what we have in git.

This will help us validate that changes to the xmlgenerator in
future don't result in unexpected changes to the generated
code.


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 :|