On Wed, Jul 01, 2020 at 12:06:32AM +0800, Shi Lei wrote:
> >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.
>
> I suggest we can now check this in a hook function.
> If we deal with discriminated unions, I think we should introduce some
> directives
> or rules to indicate the relationship between union values and child struct.
> I'm afraid that the generator and the new directives will be too complicated.
>
> Do you have any suggestion about this?
The current layout of many of our structs is unhelpful, because while we
treat the elements contents as a union, the struct is often flat. I think
we'll need to fix this as part of adopting the generator, so that we always
use a union for structs.
Taking one of the more simple examples
struct _virDomainHostdevSubsys {
int type; /* enum virDomainHostdevSubsysType */
union {
virDomainHostdevSubsysUSB usb;
virDomainHostdevSubsysPCI pci;
virDomainHostdevSubsysSCSI scsi;
virDomainHostdevSubsysSCSIVHost scsi_host;
virDomainHostdevSubsysMediatedDev mdev;
} u;
};
The "int type" field is the union discriminator and has a string
conversion via the enum.
So we can annotate the union to say which field to use as the
discriminator to switch parsing based on:
struct _virDomainHostdevSubsys {
virDomainHostdevSubsysType type; /* xmlattr */
union {
virDomainHostdevSubsysUSB usb;
virDomainHostdevSubsysPCI pci;
virDomainHostdevSubsysSCSI scsi;
virDomainHostdevSubsysSCSIVHost scsi_host;
virDomainHostdevSubsysMediatedDev mdev;
} u; /* xmlswitch:type */
};
We can declare that the name of each union member should match the string
form of the "type" attribute. eg with type="usb", we expect the union to
have a field "usb". So with type="usb", we'll parse the
virDomainHostdevSubsysUSB
struct as the content.
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 :|
