7:36 a.m.
On Wed, Jun 10, 2020 at 09:20:29AM +0800, Shi Lei wrote:
This tool is used to generate parsexml/formatbuf functions for
structs.
It is based on libclang and its python-binding.
Some directives (such as genparse, xmlattr, etc.) need to be added on
the declarations of structs to direct the tool.
Signed-off-by: Shi Lei <shi_lei(a)massclouds.com>
---
build-aux/generator/directive.py | 839 +++++++++++++++++++++++++++++++
build-aux/generator/go | 14 +
build-aux/generator/main.py | 416 +++++++++++++++
build-aux/generator/utils.py | 100 ++++
FWIW, we've tried to standardize on scripts/ directory for any helper
scripts used during the build process. Calling it "generator" is probably
too broad, as we've lots of diffferent generators.
I think this would would be a good fit as scripts/xmlgen/
po/POTFILES.in | 1 +
5 files changed, 1370 insertions(+)
create mode 100644 build-aux/generator/directive.py
create mode 100755 build-aux/generator/go
create mode 100755 build-aux/generator/main.py
create mode 100644 build-aux/generator/utils.py
+T_CLEAR_FUNC_IMPL = '''
+void
+${typename}Clear(${typename}Ptr def)
+{
+ if (!def)
+ return;
+
+ ${body}
+}
+'''
+
+T_CLEAR_FUNC_DECL = '''
+void
+${typename}Clear(${typename}Ptr def);
In general I think we have a preference for ${typename}Free
functions, rather than ${typename}Clear.
A Clear() func is needed when a struct is often stack allocated,
or when the method allocating & free'ing the struct is different
from the method clearing it. The latter case is generally considered
bad practice - it is safer and easier to verify code when clearing
and freeing is done at the same same.
+def clearMember(member):
+ mtype = TypeTable().get(member['type'])
+
+ refname = 'def->%s' % member['name']
+ if member.get('array'):
+ refname += '[i]'
+
+ code = ''
+ if mtype['meta'] == 'Struct':
+ if member['pointer'] and not mtype.get('external'):
+ code = '%sClear(%s);' % (mtype['name'], refname)
+ code += '\nVIR_FREE(%s);' % refname
We should use g_free() for any new code.
+ else:
+ code = '%sClear(&%s);' % (mtype['name'], refname)
+ elif mtype['name'] == 'String':
+ code = 'VIR_FREE(%s);' % refname
+ elif mtype['name'] in ['Chars', 'UChars']:
+ code = 'memset(%s, 0, sizeof(%s));' % (refname, refname)
+ elif not member.get('array'):
+ code = '%s = 0;' % refname
+
+ if member.get('specified'):
+ assert not member.get('array'), "'specified' can't come
with 'array'."
+ code += '\n%s_specified = false;' % refname
+
+ if member.get('array') and code:
+ counter = counterName(member['name'])
+ if singleline(code):
+ code = render(T_LOOP_SINGLE, counter=counter, body=code)
+ else:
+ code = render(T_LOOP_MULTI,
+ counter=counter, body=indent(code, 2))
+ code += '\nVIR_FREE(def->%s);' % member['name']
+ code += '\ndef->%s = 0;' % counter
+
+ return code
+
+
+T_CLEAR_NAMESPACE = '''
+if (def->namespaceData && def->ns.free)
+ (def->ns.free)(def->namespaceData);
+'''
+
+
+def makeClearFunc(writer, atype):
+ if 'genparse' not in atype:
+ return
+
+ blocks = BlockAssembler()
+ for member in atype['members']:
+ blocks.append(clearMember(member))
+
+ if 'namespace' in atype:
+ blocks.append(T_CLEAR_NAMESPACE.strip())
+
+ body = indent(blocks.output('\n\n'), 1)
+
+ impl = render(T_CLEAR_FUNC_IMPL, typename=atype['name'], body=body)
+ writer.write(atype, 'clearfunc', '.c', impl)
+
+ decl = render(T_CLEAR_FUNC_DECL, typename=atype['name'])
+ writer.write(atype, 'clearfunc', '.h', decl)
Bearing in mind the above, I'd say we should create Free() funcs,
not Clear() funcs here.
+
+
+#
+# Templates for parsing member block
+#
+T_READ_ATTR_BY_PROP = '${mdvar} = virXMLPropString(node,
"${tname}");'
+T_READ_ELEM_BY_PROP = '${mdvar} = virXMLChildNode(node, "${tname}");'
+T_READ_ELEM_CONTENT = '${mdvar} = virXMLChildNodeContent(node,
"${tname}");'
+
+T_PARSE_MEMBER_MORE = '''
+${number} = virXMLChildNodeSet(node, "${tname}", &nodes);
+if (${number} > 0) {
+ size_t i;
+
+ if (VIR_ALLOC_N(def->${name}, ${number}) < 0)
+ goto error;
+
+ for (i = 0; i < ${number}; i++) {
+ xmlNodePtr tnode = nodes[i];
+ ${item}
+ }
+ def->${counter} = ${number};
+ VIR_FREE(nodes);
+} else if (${number} < 0) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("Invalid ${tname} element found."));
For error messages we should avoid using python string substitution.
Instead we want todo
virReportError(VIR_ERR_XML_ERROR,
_("Invalid %s element found.").
"${tname}");
This means that when we repeat this code with many different values
of ${tname}, we're only getting 1 string for the translation team
to deal with.
The same point for other virReportError error callsin this file,
I won't mention them all individually.
+T_ALLOC_MEMORY = '''
+if (VIR_ALLOC(def->${name}) < 0)
+ goto error;
+'''
We prefer g_new0 for new code now.
As a general rule for any code, if there is a GLib API that works
then we should use that, instead of a libvirt API.
+T_PARSE_FUNC_DECL = '''
+int
+${funcname}(${args});
+'''
+
+T_PARSE_FUNC_IMPL = '''
+int
+${funcname}(${args})
+{
+ ${declare_vars}
+ VIR_USED(instname);
+ VIR_USED(opaque);
There should not be any need for this macro. Instead change
the function declaration to add G_GNUC_UNUSED for "instname"
and "opaque". This annotation is harmless if the variable
really is used.
+
+ if (!def)
+ goto error;
+
+ ${body}
+
+ return 0;
+
+ error:
+ ${cleanup_vars}
+ ${typename}Clear(def);
IIUC, the caller of this method is responsible for allocating
'def', and so the caller should be responsible for running
${typename}Free(def). IOW, we should not need to call Clear
for this.
+ return -1;
+}
+def _handleTmpVars(tmpvars):
+ heads, tails = [], []
+ tmpvars = dedup(tmpvars)
+ for var in tmpvars:
+ if var == 'nodes':
+ heads.append('xmlNodePtr *nodes = NULL;')
+ tails.append('VIR_FREE(nodes);')
+ elif var.endswith('Str'):
+ heads.append('g_autofree char *%s = NULL;' % var)
+ elif var.endswith('Node'):
+ heads.append('xmlNodePtr %s = NULL;' % var)
+ else:
+ assert var.endswith('Nodes') and var.startswith('n')
+ heads.append('int %s = 0;' % var)
+
+ return '\n'.join(heads), '\n'.join(tails)
One thing I noticed in the generated code is that we;re
duplicating the strings twice:
Consider this snippet of generated code:
g_autofree char *nameStr = NULL;
nameStr = virXMLPropString(node, "name");
if (nameStr == NULL) {
virReportError(VIR_ERR_XML_ERROR,
_("Missing 'name' setting in '%s'"),
instname);
goto error;
}
def->name = g_strdup(nameStr);
virXMLPropString already duplicates the string, and then
we g_strdup again. So we should just get rid of the
g_autofree and directly assign def->name = nameStr.
diff --git a/build-aux/generator/go b/build-aux/generator/go
new file mode 100755
index 0000000..9e30e08
--- /dev/null
+++ b/build-aux/generator/go
@@ -0,0 +1,14 @@
+# This is a command-line tool
+
+libclang_line=`ldconfig -p | grep libclang`
This needs to be libclang.so, otherwise it can match on the
wrong library:
$ ldconfig -p | grep libclang
libclang.so.10 (libc6,x86-64) => /lib64/libclang.so.10
libclang.so (libc6,x86-64) => /lib64/libclang.so
libclang-cpp.so.10 (libc6,x86-64) => /lib64/libclang-cpp.so.10
libclang-cpp.so (libc6,x86-64) => /lib64/libclang-cpp.so
+export libclang_path=`expr "$libclang_line" : '.*
=> \(.*\)$'`
+if test -z "$libclang_path"; then
+ echo "libclang is required by libvirt\n"
+ exit -1
+fi
diff --git a/build-aux/generator/main.py
b/build-aux/generator/main.py
new file mode 100755
index 0000000..78a90f1
--- /dev/null
+++ b/build-aux/generator/main.py
@@ -0,0 +1,416 @@
+#!/usr/bin/env python3
+#
+# Copyright (C) 2020 Shandong Massclouds Co.,Ltd.
+#
+# This library is free software; you can redistribute it and/or
+# modify it under the terms of the GNU Lesser General Public
+# License as published by the Free Software Foundation; either
+# version 2.1 of the License, or (at your option) any later version.
+#
+# This library is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+# Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public
+# License along with this library. If not, see
+# <http://www.gnu.org/licenses/>;.
+#
+if __name__ == "__main__":
+
+ libclang_path = os.environ.get('libclang_path')
+ assert libclang_path, 'No libclang library.'
+ Config.set_library_file(libclang_path)
I'm wondering if we really need this at all ? AFAICT, the libclang
python library works fine without it, so this is only required if
trying to use a non-standard libclang, which I think ought to be
possible already by setting LD_LIBRARY_PATH
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 :|