On 08/11/2015 12:45 PM, Jonathan Toppins wrote:
> On 8/10/15 1:28 AM, Laine Stump wrote: >> On 08/07/2015 06:14 PM, Jonathan Toppins wrote: >>> Adds a new interface type using UDP sockets, this seems only >>> applicable to QEMU but have edited tree-wide to support the new >>> interface type. >>> >>> The interface type required the addition of a "destaddr" >>> (destination address), this then maps into the following xml and >>> qemu call. >>> >>> <interface type='udp'> <mac address='52:54:00:5c:67:56'/> <source >>> address='127.0.0.1' port='11112'/> <model type='virtio'/> <dest >>> address="127.0.0.1' port='22222'/> >> >> I think I don't like the "dest" element. Historically, <source> has >> been used to describe the resources used in the real world to >> backup the emulated device, or maybe "the real world source of the >> data that's going into the emulated device". What you're doing is >> kind of redefining it to be "the local end of real world >> resources". Aside from that, for mcast, client, and server >> interface types, <source address....> is where the remote address >> for the socket is set. >> >> I'd say that to make it fit in better with the existing types, the >> remote address should be in <source address='blah'>, and the local >> address should be a subelement of that, something like: >> >> <source address='remoteaddr' port='remoteport'> <local >> address='localaddr' port='localport'/> </source> >> >> That makes source address exactly fit the same purpose as source >> address for the existing mcast, server, and client interface types. >> (It may seem a little odd until you start thinking of "source" as >> "the thing farthest away from the virtual machine"). > > I am not partial to "dest", would be happy to change the tag to > something that makes more sense. > > Would need to think about (or be given some pointers to) how one > would process a nested entry like this. Am sure it is "simple" but > didn't really look much into it and followed what the surrounding > code had done.

Look at examples of the function virXPathString(). You can grab nested things with the virXPath* functions quite easily. For example:

   virXPathString("string(./source/@address)", ctxt)

gets the address in <source>, and

  virXPathString("string(./source/local/@address)", ctxt)

ges the address in <source ...> <local>.


> > From a, I need to generate this by hand or edit it by hand, it would > make sense to me to keep the nesting in the <interface> entry as > simple as possible, it seems right now it is only one tag level deep > (limited knowledge though).

The most important things are 1) don't paint ourselves into a corner where we will need to add something later but the grammar has no reasonable place for us to expand, and 2) remain as consistent as possible with the existing elements/attributes. Nesting things in subelements doesn't usually cause problems in practice; for example, look at how <vlan> is done - for a single vlan tag we could have made it much simpler, not nested, but then when we added vlan trunks, etc, it would have been either very ugly or impossible.

> On the other hand nesting it like this > would make writing a DTD/XSD easier, not sure if libvirt does this > for its xml configurations.
Ah, this points out that you didn't update the grammar file: docs/schemas/domaincommon.rng. Anything allowed in the XML needs to be described in the RNG files.

(and, as Jan pointed out, you will also need to add test cases in tests/qemuxml2argvdata and tests/qemuxml2xmldata, and plug them into tests/qemuxml2argvtest and tests/qemuxml2xmltest.)

> > Thanks for the comments so far. >



and thanks for putting up with them :-)