Re: [libvirt PATCH 05/10] conf: parse/format <portOptions isolated='yes|no'/>
On 2/18/20 12:39 PM, Ján Tomko wrote:
On Sun, Feb 16, 2020 at 11:22:54PM -0500, Laine Stump wrote:
> This is a very simple thing to parse and format, but needs to be done
> in 4 places, so two trivial utility functions have been made that can
> be called from all the higher level parser/formatters:
>
> <domain><interface>
> <domain><interface><actual> (only in domain status)
> <network>
> <networkport>
>
> Signed-off-by: Laine Stump <laine(a)redhat.com>
> ---
> docs/schemas/domaincommon.rng | 3 +
> docs/schemas/network.rng | 3 +
> docs/schemas/networkcommon.rng | 11 ++++
> docs/schemas/networkport.rng | 3 +
> src/conf/domain_conf.c | 19 ++++++
> src/conf/domain_conf.h | 4 ++
> src/conf/network_conf.c | 32 ++++++++++
> src/conf/network_conf.h | 9 +++
> src/conf/virnetworkportdef.c | 3 +
> src/conf/virnetworkportdef.h | 1 +
> src/libvirt_private.syms | 1 +
> tests/networkxml2xmlin/isolated-ports.xml | 7 +++
> tests/networkxml2xmlout/isolated-ports.xml | 7 +++
> tests/networkxml2xmltest.c | 1 +
> tests/qemuxml2argvdata/net-isolated-port.xml | 34 ++++++++++
> .../net-isolated-port.x86_64-latest.xml | 63 +++++++++++++++++++
> tests/qemuxml2xmltest.c | 1 +
> 17 files changed, 202 insertions(+)
> create mode 100644 tests/networkxml2xmlin/isolated-ports.xml
> create mode 100644 tests/networkxml2xmlout/isolated-ports.xml
> create mode 100644 tests/qemuxml2argvdata/net-isolated-port.xml
> create mode 100644
> tests/qemuxml2xmloutdata/net-isolated-port.x86_64-latest.xml
>
Not a fan of multi-word elements, because they bring up our
inconsistency in using camelCase vs snake_case.
Yeah, it always bothers me when I see a multiword element or attribute
for that reason. I always use camelCase because I remember asking about
which is preferred > 10 years ago and being told that we wanted to have
camelCase in libvirt XML. That could even be a false memory, but it has
always stuck with me.
But I assume you chose the name to make it compatible with all four
containing elements.
Would something like:
<networkport>
<port isolated='yes'/>
</networkport>
look too odd?
ummmm.... God I *HATE* coming up with element and attribute names!
(That's the only response I can think of right now since it's late in
the day. Let me sleep on it, but in the end I was expecting, even
*hoping* someone would object to portOptions and propose an alternative,
and yours doesn't really sound any *worse* than mine, so it might be
worthwhile to use it just so I wouldn't have to shoulder the blame :-)
Code-wise:
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano