[libvirt] [PATCH 00/10] network: physical device abstraction aka 'virtual switch'

This patch is in response to the following bug reports: https://bugzilla.redhat.com/show_bug.cgi?id=643947 (RHEL) https://bugzilla.redhat.com/show_bug.cgi?id=636106 (upstream) It is functionally complete, and has gone through rudimentary testing for bridge networks (host bridge) and direct networks in bridge mode (macvtap). The patch series doesn't yet include updates to the domain and network XML documentation, though, so it isn't ready to push. I am sending it now to get feedback, both on the specifics of the code, as well as on how it is designed and how it works. I will be transferring info from my design document (at the end of this message) into the libvirt doc files during this week, and will have them ready for the V2 of the series that is sure to be requested. ***************** (The working design document) Network device abstraction aka virtual switch - V4 ================================================== The <interface> element of a guest's domain config in libvirt has a <source> element that describes what resources on a host will be used to connect the guest's network interface to the rest of the world. This is very flexible, allowing several different types of connection (virtual network, host bridge, direct macvtap connection to physical interface, qemu usermode, user-defined via an external script), but currently has the problem that unnecessary details of the host resources are embedded into the guest's config; if the guest is migrated to a different host, and that host has a different hardware or network config (or possibly the same hardware, but that hardware is currently in use by a different guest), the migration will fail. This document outlines a change to libvirt's network XML that will allow us to (optionally - old configs will remain valid) remove the host details from the guest's domain XML (which can move around from host to host) and place them in the network XML (which remains with a single host); the domain XML will then use existing config elements to associate each guest interface with a "network". The motivating use case for this change is the "direct" connection type (which uses macvtap for vepa and vnlink connections directly between a guest and a physical interface, rather than through a bridge), but it is applicable for all types of connection. (Another hopeful side effect of this change will be to make libvirt's network connection model easier to realize on non-Linux hypervisors (eg, VMWare ESX) and for other network technologies, such as openvswitch, VDE, and various VPN implementations). Background ========== (parts lifted from Dan Berrange's mail on this subject) Currently <network> supports 3 connectivity modes - Non-routed network, separate subnet (no <forward> element present) - Routed network, separate subnet with NAT (<forward mode='nat'/>) - Routed network, separate subnet (<forward mode='route'/>) Each of these is implemented in the existing network driver by creating a bridge device using brctl, and connecting the guest network interfaces via tap devices (a detail which, now that I've stated it, you should promptly forget!). All traffic between that bridge and the outside network is done via the host's IP routing stack (ie, there is no physical device directly connected to the bridge) In the future, these two additional routed modes might be useful: - Routed network, IP subnetting - Routed network, separate subnet with VPN The core goal of this proposal, though, is to replace type=bridge and type=direct from the domain interface XML with new types of <network> definitions so that the domain can just give "type='network'" and have all the necessary details filled in at runtime. This basically means we're adding several bridging modes (the submodes of "direct" have been flattened out here): - Bridged network, eth + bridge + tap - Bridged network, eth + macvtap + vepa - Bridged network, eth + macvtap + private - Bridged network, eth + macvtap + passthrough - Bridged network, eth + macvtap + bridge Another "future expansion" could be to add: - Bridged network, with VPN Likewise, support for other technologies, such as openvswitch and VDE would each be another entry on this list. (Dan also listed each of the above "+sriov" separately, but that ends up being handled in an orthogonal manner (by just specifying a pool of interfaces for a single network), so I'm only giving the abbreviated list) I. Changes to domain <interface> element ======================================== In many cases, the <interface> element of the domain XML will be identical to what is used now when connecting the interface to a libvirt-style virtual network: <interface type='network'> <source network='red-network'/> <mac address='xx:xx:xx:xx:xx:xx'/> </interface> Depending on the definition of the network "red-network" on the host the guest was started on / migrated to, this could be either a direct (macvtap) connection using one of the various direct modes (vepa/private/bridge/passthrough), a bridge (again, pointed to by the definition of 'red-network'), or a virtual network (using the current network definition syntax). This way the same guest could be migrated not only between macvtap-enabled hosts, but from there to a host using a bridge, or maybe a host in a remote location that used a virtual network with a secure tunnel to connect back to the rest of the red-network. (Part of the migration process would of course check that the destination host had a network of the proper name with adequate available resources, and fail if it didn't; management software at a level above libvirt would probably filter a list of candidate migration destinations based on available networks and any various details of those networks (eg. it could search for only networks using vepa for the connection), and only attempt migration to one that had the matching network available). <virtualport> element of <interface> ------------------------------------ Since mamy of the attributes/sub-elements of <virtualport> (used by some modes of "direct" interface connections) are identical for all interfaces connecting to any given switch, most of the information in <virtualport> will be optional in the domain's interface definition - it can be filled in from a similar <virtualport> element that will be added to the <network> definition. Some parameters in <virtualport> ("instanceid", for example) must be unique for every interface, though, so those will still be specified in the <interface> XML. The two <virtualport> elements will be OR'ed at runtime to arrive at the actual set of parameters that are used. (Open Question: What should be the policy when a parameter is specified in both places? Should one take precedence? Or should it be considered an error?) portgroup attribute of <source> ------------------------------- The <source> element of an interface definition will be able to optionally specify a "portgroup" attribute. If portgroup is *NOT* given, the default portgroup of the network will be used (if a default is defined, otherwise no portgroup will be used). If portgroup *IS* specified, the source network must have a portgroup by that name (or the domain startup/migration will fail), and the attributes of that portgroup will be used for the connection. Here is an example <interface> definition that has both a reduced <virtualport> element, as well as a portgroup attribute: <interface type='network'> <source network='red-network' portgroup='engineering'/> <virtualport type="802.1Qbg"> <parameters managerid="11" typeid="1193047" typeidversion="2" instanceid="09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f"/> </virtualport> <mac address='de:ad:be:ef:ca:fe'/> </interface> (The specifics of what can be in a portgroup are given below) storing the actual chosen/running config in state dir ----------------------------------------------------- Note: the following additions to the XML will only ever be visible in the statedir copy of the domain config, which is used to keep track of the state of running domains in case libvirtd is restarted while domains are still running. The described element cannot be used in a user-generated config file, and will never be present in a domain interface config produced by the libvirt public API, nor by "virsh dumpxml". In order to remind libvirt about which interfaces are actually in use in the event that libvirtd is restarted while domains are still running, the copy of the domain XML stored in "statedir" (/var/lib/libvirt/qemu/*.xml) may have an extra element <actual> stored as a subelement of each <interface>: <interface type='network'> <source network='red-network' portgroup='engineering'/> <virtualport type="802.1Qbg"> <parameters managerid="11" typeid="1193047" typeidversion="2" instanceid="09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f"/> </virtualport> <mac address='de:ad:be:ef:ca:fe'/> <actual type='direct'> <source dev='eth1' mode='vepa'/> </actual> </interface> In short, merging the <auto> element up into <interface> will yield the full interface as it was actually instantiated for the domain. In this case, the interface still has a mac address of de:ad:be:ef:ca:fe, and will use the same <virtualport> parameters, but the actual type of the interface will be 'direct' (so macvtap will be used to connect the interface), and the connection will be via physical device eth1 in vepa mode. network II. Changes to <network> definition =================================== As Dan has pointed out, any additions to <network> must be designed so that existing management applications (written to understand <network> prior to these new additions) will at least recognize that the XML they've been given is for something new that they don't fully understand. At the same time, the new types of network definition should attempt to re-use as much of the existing elements/attributes as possible, both to make it easier to extend these applications, as well as to make the status displays of un-updated applications make as much sense as possible. The new types of network will be specified by extending the choices for <forward mode='....'>. The current modes are: <forward mode='route|nat'/> (in addition to not listing any mode, which equates to "isolated") Here are suggested new modes: <forward mode='bridge|vepa|private|passthrough'/> A description of each: bridge - equivalent to "<interface type='bridge'>" in the interface definition. The bridge device to use would be given in the existing <bridge name='xxx'>. or <interface type='direct'> ... <source mode='bridge'/> (ie, macvtap bridge mode) with the physical interface name given in <forward dev='xxx'> or from the pool of devices given as subelements of <forward> (see below) vepa - same as "<interface type='direct'>..." with <source mode='vepa'/> private - <interface type='direct'> ... <source mode='private'/> passthrough - <interface type='direct'> ... <source mode='passthrough'/> Interface Pools --------------- In many cases, a single host network may have multiple physical network devices associated with it (especially in the case of an SRIOV-capable ethernet card, which will have several "virtual functions" associated with a single physical ethernet connection). The host will at least want to balance the load of multiple guests between these multiple devices, and may even require (in the case of passthrough mode, for example) that only a single guest interface be attached to each host device. The current specification for <forward> only allows for a single "dev" attribute, though. In order to support multiple device names, we will extend <forward> to allow 0 or more <interface> subelements: <forward mode='vepa'> <interface dev='eth10'/> <interface dev='eth11'/> <interface dev='eth12'/> <interface dev='eth13'/> </forward> Note that, as a convenience, *on output* the first of these elements will always be a duplicate of the "dev" attribute in <forward> itself. When sending XML definnitions to libvirt, either a single interface should be send in <forward>, or a pool of them as sub-elements, but not both (if you do this, and the first in the pool matches the one given in <forward>, it will be ignored, but if they don't match, that is an error). In the case of mode='passthrough' (as well as mode='private' if the virtPortProfile has a mode setting of 802.1Qbh), only one guest interface can be connected to a device at a time. libvirt will keep track of which devices are in use, and attempt to assign a free device; failure to assign a device will result in a failure of the domain to start/migrate. For the other direct modes, libvirt will simply keep track of the number of guest interfaces currently using each device, and attempt to keep them balanced. Portgroups ----------- A <portgroup> (subelement of <network>) is just a way of easily putting connections to the network into different classes, with each class having a different level/type of service. Each <network> can have multiple <portgroup> elements, and each <portgroup> has a name, as well as various attributes associated with it. If an interface definition specifies a portgroup, that portgroup's info will be used to modify the interface's setup. If no portgroup is given and one of the network's portgroups has "default='yes'", that default portgroup will be used. If no portgroup is given in the interface definition, and there is no default portgroup, then none will be used. The first thing we will use portgroups for is as an alternate place to specify <virtualport> parameters: <portgroup name='engineering' default='yes'> <virtualport type="802.1Qbg"> <parameters managerid="11" typeid="1193047" typeidversion="2"/> </virtualport> </portgroup> Anything that is valid in an interface's <virtualport> is also valid here. The next thing to specify in a portgroup will be bandwidth limiting / QoS configuration. Since I don't know exactly what's needed for that, I won't specify it here. If anything is specified both directly under <network> and in a <portgroup>, the value in portgroup will take precedence. (Again - what will the precedence of items specified in the <interface> be?) EXAMPLES -------- Examples of 'red-network' for different types of connections (all of these would work with minor variations of the interface XML given above, eg the 'vepa' version would require <virtualport> in the interface that specified an instanceid, and if the <interface> specified a portgroup, it would need to also be in the <network> definition (even if it was empty aside from name). <!-- Existing usage - a libvirt virtual network --> <network> <name>red-network</name> <bridge name='virbr0'/> <forward mode='route'/> ... </network> <!-- The simplest - an existing host bridge --> <network> <name>red-network</name> <forward mode='bridge'/> <bridge name='eth0'/> </network> <!-- A macvtap connection to a vepa bridge --> <network> <name>red-network</name> <forward mode='vepa' dev='eth10'/> <virtualport type='802.1Qbg'> <parameters managerid='11' typeid='1193047' typeidversion='2'/> </virtualport> <!-- NB: if <interface> doesn't specify portgroup, --> <!-- 'accounting' is assumed --> <portgroup name='accounting'> <virtualport> <parameters typeid='22'/> </virtualport> </portgroup> <portgroup name='engineering'> <virtualport> <parameters typeid='33'/> </virtualport> </portgroup> </network> <!-- A macvtap passthrough connection (one guest interface per dev) --> <network> <name>red-network</name> <forward mode='passthrough'> <interface dev='eth10'/> <interface dev='eth11'/> <interface dev='eth12'/> <interface dev='eth13'/> <interface dev='eth14'/> <interface dev='eth15'/> <interface dev='eth16'/> <interface dev='eth17'/> </forward> </network> ============= Keeping Track of Interface Usage by Guests ========================================== While libvirtd is running, each physical interface in a network's pool will maintain a count of how many guest interfaces are using that physical interface. Each guest interface will also maintain information about which network, and which physical interface on that network, it is using. The following situations could occur: 1) A guest is terminated while libvirtd is running. libvirtd will notice this, and decrement the usage count for each interface used by the guest, as maintained in the guest's state info. 2) The host system is rebooted When the libvirt network driver is restarted, no guests will yet be running, so the usage count of each physical interface will be 0, and get incremented as guests are started up. 3) libvirtd is restarted When the network is restarted, the usage count for all physical interfaces will be set to 0, just as if the entire system had been rebooted. One of two situations might be encountered: 3a) The guest is still running when libvirtd is restarted. In this case, the existing state information of the guest will be examined to determine which physical interface usage count to increment. 3b) The guest has been terminated while libvirtd wasn't present. Since the guest is no longer running, its state information will be thrown away.

--- docs/schemas/Makefile.am | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/schemas/Makefile.am b/docs/schemas/Makefile.am index dbad35f..36b9aeb 100644 --- a/docs/schemas/Makefile.am +++ b/docs/schemas/Makefile.am @@ -2,16 +2,16 @@ schemadir = $(pkgdatadir)/schemas schema_DATA = \ + capability.rng \ domain.rng \ domainsnapshot.rng \ interface.rng \ network.rng \ + nodedev.rng \ + nwfilter.rng \ secret.rng \ storageencryption.rng \ storagepool.rng \ - storagevol.rng \ - nodedev.rng \ - capability.rng \ - nwfilter.rng + storagevol.rng EXTRA_DIST = $(schema_DATA) -- 1.7.3.4

On Tue, Jul 05, 2011 at 03:45:49AM -0400, Laine Stump wrote:
--- docs/schemas/Makefile.am | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/docs/schemas/Makefile.am b/docs/schemas/Makefile.am index dbad35f..36b9aeb 100644 --- a/docs/schemas/Makefile.am +++ b/docs/schemas/Makefile.am @@ -2,16 +2,16 @@
schemadir = $(pkgdatadir)/schemas schema_DATA = \ + capability.rng \ domain.rng \ domainsnapshot.rng \ interface.rng \ network.rng \ + nodedev.rng \ + nwfilter.rng \ secret.rng \ storageencryption.rng \ storagepool.rng \ - storagevol.rng \ - nodedev.rng \ - capability.rng \ - nwfilter.rng + storagevol.rng
EXTRA_DIST = $(schema_DATA)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 07/05/2011 05:45 AM, Daniel P. Berrange wrote:
On Tue, Jul 05, 2011 at 03:45:49AM -0400, Laine Stump wrote:
--- docs/schemas/Makefile.am | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-) ACK
Thanks. I pushed this, and the other 3 ACKed patches from the series, now to avoid repeating them in upcoming V2...Vn, since they are simple refactoring that don't need the other patches (which, except for 05/10, are adding the new functionality).

Although most functions with flags check to verify no application is passing in flag bits that are currently undefined, for some reason this function wasn't. --- src/qemu/qemu_driver.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 363a361..8105910 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3825,6 +3825,10 @@ static char *qemuDomainGetXMLDesc(virDomainPtr dom, unsigned long balloon; int err; + virCheckFlags(VIR_DOMAIN_XML_SECURE | + VIR_DOMAIN_XML_INACTIVE | + VIR_DOMAIN_XML_UPDATE_CPU, NULL); + qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); -- 1.7.3.4

On Tue, Jul 05, 2011 at 03:45:50AM -0400, Laine Stump wrote:
Although most functions with flags check to verify no application is passing in flag bits that are currently undefined, for some reason this function wasn't. --- src/qemu/qemu_driver.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 363a361..8105910 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3825,6 +3825,10 @@ static char *qemuDomainGetXMLDesc(virDomainPtr dom, unsigned long balloon; int err;
+ virCheckFlags(VIR_DOMAIN_XML_SECURE | + VIR_DOMAIN_XML_INACTIVE | + VIR_DOMAIN_XML_UPDATE_CPU, NULL); + qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid);
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 07/05/2011 05:45 AM, Daniel P. Berrange wrote:
On Tue, Jul 05, 2011 at 03:45:50AM -0400, Laine Stump wrote:
Although most functions with flags check to verify no application is passing in flag bits that are currently undefined, for some reason this function wasn't. --- src/qemu/qemu_driver.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-)
ACK
Thanks, pushed.

* Change all flags args from int to unsigned int * Allow passing flags in virDomainObjParseFile (and propogate those flags all the way down the call chain). Previously the flags were hardcoded (to VIR_DOMAIN_XML_INTERNAL_STATUS) several layers down the chain. Pass that value in at the one place that is currently calling virDomainObjParseFile. --- src/conf/domain_conf.c | 30 +++++++++++++++++------------- src/conf/domain_conf.h | 13 +++++++------ 2 files changed, 24 insertions(+), 19 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 109a947..772422d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5160,7 +5160,7 @@ error: virDomainDeviceDefPtr virDomainDeviceDefParse(virCapsPtr caps, const virDomainDefPtr def, const char *xmlStr, - int flags) + unsigned int flags) { xmlDocPtr xml; xmlNodePtr node; @@ -6703,7 +6703,8 @@ no_memory: static virDomainObjPtr virDomainObjParseXML(virCapsPtr caps, xmlDocPtr xml, - xmlXPathContextPtr ctxt) + xmlXPathContextPtr ctxt, + unsigned int flags) { char *tmp = NULL; long val; @@ -6726,8 +6727,7 @@ static virDomainObjPtr virDomainObjParseXML(virCapsPtr caps, oldnode = ctxt->node; ctxt->node = config; - obj->def = virDomainDefParseXML(caps, xml, config, ctxt, - VIR_DOMAIN_XML_INTERNAL_STATUS); + obj->def = virDomainDefParseXML(caps, xml, config, ctxt, flags); ctxt->node = oldnode; if (!obj->def) goto error; @@ -6816,14 +6816,14 @@ virDomainDefParse(const char *xmlStr, virDomainDefPtr virDomainDefParseString(virCapsPtr caps, const char *xmlStr, - int flags) + unsigned int flags) { return virDomainDefParse(xmlStr, NULL, caps, flags); } virDomainDefPtr virDomainDefParseFile(virCapsPtr caps, const char *filename, - int flags) + unsigned int flags) { return virDomainDefParse(NULL, filename, caps, flags); } @@ -6832,7 +6832,7 @@ virDomainDefPtr virDomainDefParseFile(virCapsPtr caps, virDomainDefPtr virDomainDefParseNode(virCapsPtr caps, xmlDocPtr xml, xmlNodePtr root, - int flags) + unsigned int flags) { xmlXPathContextPtr ctxt = NULL; virDomainDefPtr def = NULL; @@ -6861,7 +6861,8 @@ cleanup: static virDomainObjPtr virDomainObjParseNode(virCapsPtr caps, xmlDocPtr xml, - xmlNodePtr root) + xmlNodePtr root, + unsigned int flags) { xmlXPathContextPtr ctxt = NULL; virDomainObjPtr obj = NULL; @@ -6879,7 +6880,7 @@ virDomainObjParseNode(virCapsPtr caps, } ctxt->node = root; - obj = virDomainObjParseXML(caps, xml, ctxt); + obj = virDomainObjParseXML(caps, xml, ctxt, flags); cleanup: xmlXPathFreeContext(ctxt); @@ -6888,13 +6889,15 @@ cleanup: virDomainObjPtr virDomainObjParseFile(virCapsPtr caps, - const char *filename) + const char *filename, + unsigned int flags) { xmlDocPtr xml; virDomainObjPtr obj = NULL; if ((xml = virXMLParseFile(filename))) { - obj = virDomainObjParseNode(caps, xml, xmlDocGetRootElement(xml)); + obj = virDomainObjParseNode(caps, xml, + xmlDocGetRootElement(xml), flags); xmlFreeDoc(xml); } @@ -9500,7 +9503,7 @@ virDomainHostdevDefFormat(virBufferPtr buf, char *virDomainDefFormat(virDomainDefPtr def, - int flags) + unsigned int flags) { virBuffer buf = VIR_BUFFER_INITIALIZER; unsigned char *uuid; @@ -10138,7 +10141,8 @@ static virDomainObjPtr virDomainLoadStatus(virCapsPtr caps, if ((statusFile = virDomainConfigFile(statusDir, name)) == NULL) goto error; - if (!(obj = virDomainObjParseFile(caps, statusFile))) + if (!(obj = virDomainObjParseFile(caps, statusFile, + VIR_DOMAIN_XML_INTERNAL_STATUS))) goto error; virUUIDFormat(obj->def->uuid, uuidstr); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 258289a..9d28f51 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1378,20 +1378,21 @@ void virDomainRemoveInactive(virDomainObjListPtr doms, virDomainDeviceDefPtr virDomainDeviceDefParse(virCapsPtr caps, const virDomainDefPtr def, const char *xmlStr, - int flags); + unsigned int flags); virDomainDefPtr virDomainDefParseString(virCapsPtr caps, const char *xmlStr, - int flags); + unsigned int flags); virDomainDefPtr virDomainDefParseFile(virCapsPtr caps, const char *filename, - int flags); + unsigned int flags); virDomainDefPtr virDomainDefParseNode(virCapsPtr caps, xmlDocPtr doc, xmlNodePtr root, - int flags); + unsigned int flags); virDomainObjPtr virDomainObjParseFile(virCapsPtr caps, - const char *filename); + const char *filename, + unsigned int flags); bool virDomainDefCheckABIStability(virDomainDefPtr src, virDomainDefPtr dst); @@ -1399,7 +1400,7 @@ bool virDomainDefCheckABIStability(virDomainDefPtr src, int virDomainDefAddImplicitControllers(virDomainDefPtr def); char *virDomainDefFormat(virDomainDefPtr def, - int flags); + unsigned int flags); int virDomainCpuSetParse(const char **str, char sep, -- 1.7.3.4

On Tue, Jul 05, 2011 at 03:45:51AM -0400, Laine Stump wrote:
* Change all flags args from int to unsigned int
* Allow passing flags in virDomainObjParseFile (and propogate those flags all the way down the call chain). Previously the flags were hardcoded (to VIR_DOMAIN_XML_INTERNAL_STATUS) several layers down the chain. Pass that value in at the one place that is currently calling virDomainObjParseFile. --- src/conf/domain_conf.c | 30 +++++++++++++++++------------- src/conf/domain_conf.h | 13 +++++++------ 2 files changed, 24 insertions(+), 19 deletions(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 07/05/2011 05:47 AM, Daniel P. Berrange wrote:
On Tue, Jul 05, 2011 at 03:45:51AM -0400, Laine Stump wrote:
* Change all flags args from int to unsigned int
* Allow passing flags in virDomainObjParseFile (and propogate those flags all the way down the call chain). Previously the flags were hardcoded (to VIR_DOMAIN_XML_INTERNAL_STATUS) several layers down the chain. Pass that value in at the one place that is currently calling virDomainObjParseFile. --- src/conf/domain_conf.c | 30 +++++++++++++++++------------- src/conf/domain_conf.h | 13 +++++++------ 2 files changed, 24 insertions(+), 19 deletions(-) ACK
Thanks, pushed.

domain.rng, network.rng, and interface.rng already use a few of the same types (or in some cases *should* but don't), and an upcoming code change will have them sharing even more. To prepare for that, this patch takes those common data type definitions and moves them into basictypes.rng. This may break some rule about the need to RNG files to be autonomous or something, but I saw that storageencryption.rng is used in this way, so I figured it must not be completely against the law... --- docs/schemas/Makefile.am | 1 + docs/schemas/basictypes.rng | 130 +++++++++++++++++++++++++++++++++++++++++++ docs/schemas/domain.rng | 86 +--------------------------- docs/schemas/interface.rng | 71 ++++------------------- docs/schemas/network.rng | 79 +++++++------------------- libvirt.spec.in | 1 + 6 files changed, 168 insertions(+), 200 deletions(-) create mode 100644 docs/schemas/basictypes.rng diff --git a/docs/schemas/Makefile.am b/docs/schemas/Makefile.am index 36b9aeb..5ef7737 100644 --- a/docs/schemas/Makefile.am +++ b/docs/schemas/Makefile.am @@ -2,6 +2,7 @@ schemadir = $(pkgdatadir)/schemas schema_DATA = \ + basictypes.rng \ capability.rng \ domain.rng \ domainsnapshot.rng \ diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng new file mode 100644 index 0000000..b3267f5 --- /dev/null +++ b/docs/schemas/basictypes.rng @@ -0,0 +1,130 @@ +<?xml version="1.0"?> +<!-- network-related definitions used in multiple grammars --> +<grammar xmlns="http://relaxng.org/ns/structure/1.0" datatypeLibrary="http://www.w3.org/2001/XMLSchema-datatypes"> + + <!-- Our unsignedInt doesn"t allow a leading "+" in its lexical form --> + <define name="unsignedInt"> + <data type="unsignedInt"> + <param name="pattern">[0-9]+</param> + </data> + </define> + + <define name="positiveInteger"> + <data type="positiveInteger"> + <param name="pattern">[0-9]+</param> + </data> + </define> + + <define name="uint8range"> + <choice> + <data type="string"> + <param name="pattern">0x[0-9a-fA-F]{1,2}</param> + </data> + <data type="int"> + <param name="minInclusive">0</param> + <param name="maxInclusive">255</param> + </data> + </choice> + </define> + <define name="uint24range"> + <choice> + <data type="string"> + <param name="pattern">0x[0-9a-fA-F]{1,6}</param> + </data> + <data type="int"> + <param name="minInclusive">0</param> + <param name="maxInclusive">16777215</param> + </data> + </choice> + </define> + + <define name="UUID"> + <choice> + <data type="string"> + <param name="pattern">[a-fA-F0-9]{32}</param> + </data> + <data type="string"> + <param name="pattern">[a-fA-F0-9]{8}\-([a-fA-F0-9]{4}\-){3}[a-fA-F0-9]{12}</param> + </data> + </choice> + </define> + + <!-- a 6 byte MAC address in ASCII-hex format, eg "12:34:56:78:9A:BC" --> + <define name="macAddr"> + <data type="string"> + <param name="pattern">([a-fA-F0-9]{2}:){5}[a-fA-F0-9]{2}</param> + </data> + </define> + + <!-- An ipv4 "dotted quad" address --> + <define name="ipv4Addr"> + <data type="string"> + <param name="pattern">(((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9]))\.){3}((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9]))</param> + </data> + </define> + + <!-- Based on http://blog.mes-stats.fr/2008/10/09/regex-ipv4-et-ipv6 --> + <define name="ipv6Addr"> + <data type="string"> + <!-- To understand this better, take apart the toplevel "|"s --> +<param name="pattern">(([0-9A-Fa-f]{1,4}:){7}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){6}:[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){5}:([0-9A-Fa-f]{1,4}:)?[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){4}:([0-9A-Fa-f]{1,4}:){0,2}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){3}:([0-9A-Fa-f]{1,4}:){0,3}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){2}:([0-9A-Fa-f]{1,4}:){0,4}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){6}(((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9]))\.){3}((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9])))|(([0-9A-Fa-f]{1,4}:){0,5}:(((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9]))\.){3}((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9])))|(::([0-9A-Fa-f]{1,4}:){0,5}(((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9]))\.){3}((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9])))|([0-9A-Fa-f]{1,4}::([0-9A-Fa-f]{1,4}:){0,5}[0-9A-Fa-f]{1,4})|(::([0-9A-Fa-f]{1,4}:){0,6}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){1,7}:)</param> + </data> + </define> + + <define name="ipAddr"> + <choice> + <ref name="ipv4Addr"/> + <ref name="ipv6Addr"/> + </choice> + </define> + + <define name="ipv4Prefix"> + <data type="unsignedInt"> + <param name="maxInclusive">32</param> + </data> + </define> + + <define name="ipv6Prefix"> + <data type="unsignedInt"> + <param name="maxInclusive">128</param> + </data> + </define> + + <define name="ipPrefix"> + <choice> + <ref name="ipv4Prefix"/> + <ref name="ipv6Prefix"/> + </choice> + </define> + + <define name="dnsName"> + <data type="string"> + <param name="pattern">[a-zA-Z0-9\.\-]+</param> + </data> + </define> + + <define name="deviceName"> + <data type="string"> + <param name="pattern">[a-zA-Z0-9_\.\-\\:/]+</param> + </data> + </define> + + <define name="filePath"> + <data type="string"> + <param name="pattern">[a-zA-Z0-9_\.\+\-\\&"'<>/%]+</param> + </data> + </define> + + <define name="absFilePath"> + <data type="string"> + <param name="pattern">/[a-zA-Z0-9_\.\+\-\\&"'<>/%]+</param> + </data> + </define> + + <define name="absDirPath"> + <data type="string"> + <param name="pattern">/[a-zA-Z0-9_\.\+\-\\&"'<>/%]*</param> + </data> + </define> + +</grammar> diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng index fb1497b..3c8414e 100644 --- a/docs/schemas/domain.rng +++ b/docs/schemas/domain.rng @@ -5,6 +5,7 @@ <ref name="domain"/> </start> + <include href='basictypes.rng'/> <include href='storageencryption.rng'/> <!-- @@ -723,7 +724,7 @@ <zeroOrMore> <element name="host"> <attribute name="name"> - <ref name="hostName"/> + <ref name="dnsName"/> </attribute> <attribute name="port"> <ref name="unsignedInt"/> @@ -1092,7 +1093,7 @@ <optional> <element name="mac"> <attribute name="address"> - <ref name="addrMAC"/> + <ref name="macAddr"/> </attribute> <empty/> </element> @@ -1100,7 +1101,7 @@ <optional> <element name="ip"> <attribute name="address"> - <ref name="addrIP"/> + <ref name="ipv4Addr"/> </attribute> <empty/> </element> @@ -2344,22 +2345,10 @@ <!-- Type library - Our unsignedInt doesn't allow a leading '+' in its lexical form A domain name should be made of ascii, numbers, _-+ and is non-empty - UUID currently allows only the 32 characters strict syntax memoryKB request at least 4Mbytes though Xen will grow bigger if too low weight currently is in range [100, 1000] --> - <define name="unsignedInt"> - <data type="unsignedInt"> - <param name="pattern">[0-9]+</param> - </data> - </define> - <define name='positiveInteger'> - <data type='positiveInteger'> - <param name="pattern">[0-9]+</param> - </data> - </define> <define name="cpuset"> <data type="string"> <param name="pattern">([0-9]+(-[0-9]+)?|\^[0-9]+)(,([0-9]+(-[0-9]+)?|\^[0-9]+))*</param> @@ -2381,11 +2370,6 @@ <param name="pattern">[0-9]+</param> </data> </define> - <define name="hostName"> - <data type="string"> - <param name="pattern">[a-zA-Z0-9\.\-]+</param> - </data> - </define> <define name="PortNumber"> <data type="short"> <param name="minInclusive">-1</param> @@ -2419,51 +2403,11 @@ <param name="pattern">[a-zA-Z0-9_\+\-]+</param> </data> </define> - <define name="UUID"> - <choice> - <data type="string"> - <param name="pattern">[a-fA-F0-9]{32}</param> - </data> - <data type="string"> - <param name="pattern">[a-fA-F0-9]{8}\-([a-fA-F0-9]{4}\-){3}[a-fA-F0-9]{12}</param> - </data> - </choice> - </define> - <define name="filePath"> - <data type="string"> - <param name="pattern">[a-zA-Z0-9_\.\+\-\\&"'<>/%]+</param> - </data> - </define> - <define name="absFilePath"> - <data type="string"> - <param name="pattern">/[a-zA-Z0-9_\.\+\-\\&"'<>/%]+</param> - </data> - </define> - <define name="absDirPath"> - <data type="string"> - <param name="pattern">/[a-zA-Z0-9_\.\+\-\\&"'<>/%]*</param> - </data> - </define> - <define name="deviceName"> - <data type="string"> - <param name="pattern">[a-zA-Z0-9_\.\-\\:/]+</param> - </data> - </define> <define name="bridgeMode"> <data type="string"> <param name="pattern">(vepa|bridge|private|passthrough)</param> </data> </define> - <define name="addrMAC"> - <data type="string"> - <param name="pattern">([a-fA-F0-9]{2}:){5}[a-fA-F0-9]{2}</param> - </data> - </define> - <define name="addrIP"> - <data type="string"> - <param name="pattern">([0-2]?[0-9]?[0-9]\.){3}[0-2]?[0-9]?[0-9]</param> - </data> - </define> <define name="addrIPorName"> <data type="string"> <param name="pattern">(([0-2]?[0-9]?[0-9]\.){3}[0-2]?[0-9]?[0-9])|(([0-9a-fA-F]+|:)+[0-9a-fA-F]+)|([a-zA-Z0-9_\.\+\-]*)</param> @@ -2539,28 +2483,6 @@ <param name="pattern">[a-zA-Z0-9_\.:]+</param> </data> </define> - <define name="uint8range"> - <choice> - <data type="string"> - <param name="pattern">0x[0-9a-fA-F]{1,2}</param> - </data> - <data type="int"> - <param name="minInclusive">0</param> - <param name="maxInclusive">255</param> - </data> - </choice> - </define> - <define name="uint24range"> - <choice> - <data type="string"> - <param name="pattern">0x[0-9a-fA-F]{1,6}</param> - </data> - <data type="int"> - <param name="minInclusive">0</param> - <param name="maxInclusive">16777215</param> - </data> - </choice> - </define> <define name="virtualPortProfileID"> <data type="string"> <param name="maxLength">39</param> diff --git a/docs/schemas/interface.rng b/docs/schemas/interface.rng index 4dd1bb4..53fa18a 100644 --- a/docs/schemas/interface.rng +++ b/docs/schemas/interface.rng @@ -1,3 +1,4 @@ +<?xml version="1.0"?> <!-- A Relax NG schema for network interfaces --> <grammar xmlns="http://relaxng.org/ns/structure/1.0" xmlns:v="http://netcf.org/xml/version/1.0" @@ -16,6 +17,7 @@ </choice> </start> + <include href='basictypes.rng'/> <!-- FIXME: How do we handle VLAN's ? Should they be their own interface or should we treat them as an option on the base interface ? For @@ -36,7 +38,7 @@ FIXME: What if device name and MAC don't specify the same NIC ? --> <optional> <element name="mac"> - <attribute name="address"><ref name="mac-addr"/></attribute> + <attribute name="address"><ref name="macAddr"/></attribute> </element> </optional> <!-- FIXME: Allow (some) ethtool options --> @@ -75,7 +77,7 @@ <element name="vlan"> <attribute name="tag"><ref name="vlan-id"/></attribute> <element name="interface"> - <attribute name="name"><ref name="device-name"/></attribute> + <attribute name="name"><ref name="deviceName"/></attribute> </element> </element> </define> @@ -202,7 +204,7 @@ </element> <element name="arpmon"> <attribute name="interval"><ref name="uint"/></attribute> - <attribute name="target"><ref name="ipv4-addr"/></attribute> + <attribute name="target"><ref name="ipv4Addr"/></attribute> <optional> <attribute name="validate"> <choice> @@ -244,7 +246,7 @@ <!-- Basic attributes for all interface types --> <define name="name-attr"> <!-- The device name, like eth0 or br2 --> - <attribute name="name"><ref name="device-name"/></attribute> + <attribute name="name"><ref name="deviceName"/></attribute> </define> <define name="mtu"> @@ -303,14 +305,14 @@ <ref name="dhcp-element"/> <group> <element name="ip"> - <attribute name="address"><ref name="ipv4-addr"/></attribute> + <attribute name="address"><ref name="ipv4Addr"/></attribute> <optional> - <attribute name="prefix"><ref name="ipv4-prefix"/></attribute> + <attribute name="prefix"><ref name="ipv4Prefix"/></attribute> </optional> </element> <optional> <element name="route"> - <attribute name="gateway"><ref name="ipv4-addr"/></attribute> + <attribute name="gateway"><ref name="ipv4Addr"/></attribute> </element> </optional> </group> @@ -331,15 +333,15 @@ </optional> <zeroOrMore> <element name="ip"> - <attribute name="address"><ref name="ipv6-addr"/></attribute> + <attribute name="address"><ref name="ipv6Addr"/></attribute> <optional> - <attribute name="prefix"><ref name="ipv6-prefix"/></attribute> + <attribute name="prefix"><ref name="ipv6Prefix"/></attribute> </optional> </element> </zeroOrMore> <optional> <element name="route"> - <attribute name="gateway"><ref name="ipv6-addr"/></attribute> + <attribute name="gateway"><ref name="ipv6Addr"/></attribute> </element> </optional> </element> @@ -417,55 +419,6 @@ </data> </define> - <define name='device-name'> - <data type='string'> - <param name="pattern">[a-zA-Z0-9_\.\-:/]+</param> - </data> - </define> - - <define name='UUID'> - <choice> - <data type='string'> - <param name="pattern">[a-fA-F0-9]{32}</param> - </data> - <data type='string'> - <param name="pattern">[a-fA-F0-9]{8}\-([a-fA-F0-9]{4}\-){3}[a-fA-F0-9]{12}</param> - </data> - </choice> - </define> - - <define name='mac-addr'> - <data type='string'> - <param name="pattern">([a-fA-F0-9]{2}:){5}[a-fA-F0-9]{2}</param> - </data> - </define> - - <define name='ipv4-addr'> - <data type='string'> - <param name="pattern">(((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9]))\.){3}((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9]))</param> - </data> - </define> - - <define name='ipv4-prefix'> - <data type='unsignedInt'> - <param name="maxInclusive">32</param> - </data> - </define> - - <!-- Based on http://blog.mes-stats.fr/2008/10/09/regex-ipv4-et-ipv6 --> - <define name='ipv6-addr'> - <data type='string'> - <!-- To understand this better, take apart the toplevel '|'s --> - <param name="pattern">(([0-9A-Fa-f]{1,4}:){7}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){6}:[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){5}:([0-9A-Fa-f]{1,4}:)?[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){4}:([0-9A-Fa-f]{1,4}:){0,2}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){3}:([0-9A-Fa-f]{1,4}:){0,3}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){2}:([0-9A-Fa-f]{1,4}:){0,4}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){6}((((25[0-5])|(1[0-9]{2})|(2[0-4][0-9])|([0-9]{1,2})))\.){3}(((25[0-5])|(1[0-9]{2})|(2[0-4][0-9])|([0-9]{1,2}))))|(([0-9A-Fa-f]{1,4}:){0,5}:((((25[0-5])|(1[0-9]{2})|(2[0-4][0-9])|([0-9]{1,2})))\.){3}(((25[0-5])|(1[0-9]{2})|(2[0-4][0-9])|([0-9]{1,2}))))|(::([0-9A-Fa-f]{1,4}:){0,5}((((25[0-5])|(1[0-9]{2})|(2[0-4][0-9])|([0-9]{1,2})))\.){3}(((25[0-5])|(1[0-9]{2})|(2[0-4][0-9])|([0-9]{1,2}))))|([0-9A-Fa-f]{1,4}::([0-9A-Fa-f]{1,4}:){0,5}[0-9A-Fa-f]{1,4})|(::([0-9A-Fa-f]{1,4}:){0,6}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){1,7}:)</param> - </data> - </define> - - <define name='ipv6-prefix'> - <data type='unsignedInt'> - <param name="maxInclusive">128</param> - </data> - </define> - <define name='vlan-id'> <data type="unsignedInt"> <param name="maxInclusive">4096</param> diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng index 05f45e5..6d9f06b 100644 --- a/docs/schemas/network.rng +++ b/docs/schemas/network.rng @@ -1,3 +1,4 @@ +<?xml version="1.0"?> <!-- A Relax NG schema for the libvirt network XML format --> <grammar xmlns="http://relaxng.org/ns/structure/1.0" datatypeLibrary="http://www.w3.org/2001/XMLSchema-datatypes"> @@ -5,6 +6,8 @@ <ref name="network"/> </start> + <include href='basictypes.rng'/> + <define name="network"> <element name="network"> @@ -18,7 +21,7 @@ <!-- <uuid> element --> <optional> - <element name="uuid"><text/></element> + <element name="uuid"><ref name="UUID"/></element> </optional> <!-- <bridge> element --> @@ -28,7 +31,7 @@ <element name="bridge"> <optional> <attribute name="name"> - <text/> + <ref name="deviceName"/> </attribute> </optional> @@ -53,7 +56,7 @@ <!-- <mac> element --> <optional> <element name="mac"> - <attribute name="address"><ref name="mac-addr"/></attribute> + <attribute name="address"><ref name="macAddr"/></attribute> <empty/> </element> </optional> @@ -65,7 +68,7 @@ <element name="forward"> <optional> <attribute name="dev"> - <text/> + <ref name="deviceName"/> </attribute> </optional> @@ -83,7 +86,7 @@ <!-- <domain> element --> <optional> <element name="domain"> - <attribute name="name"><text/></attribute> + <attribute name="name"><ref name="dnsName"/></attribute> </element> </optional> @@ -93,15 +96,15 @@ <element name="dns"> <zeroOrMore> <element name="txt"> - <attribute name="name"><ref name="dns-name"/></attribute> + <attribute name="name"><ref name="dnsName"/></attribute> <attribute name="value"><text/></attribute> </element> </zeroOrMore> <zeroOrMore> <element name="host"> - <attribute name="ip"><ref name="ipv4-addr"/></attribute> + <attribute name="ip"><ref name="ipv4Addr"/></attribute> <oneOrMore> - <element name="hostname"><text/></element> + <element name="hostname"><ref name="dnsName"/></element> </oneOrMore> </element> </zeroOrMore> @@ -114,12 +117,12 @@ local to the host. --> <element name="ip"> <optional> - <attribute name="address"><ref name="ip-addr"/></attribute> + <attribute name="address"><ref name="ipAddr"/></attribute> </optional> <optional> <choice> - <attribute name="netmask"><ref name="ipv4-addr"/></attribute> - <attribute name="prefix"><ref name="ip-prefix"/></attribute> + <attribute name="netmask"><ref name="ipv4Addr"/></attribute> + <attribute name="prefix"><ref name="ipPrefix"/></attribute> </choice> </optional> <optional> @@ -136,22 +139,22 @@ <element name="dhcp"> <zeroOrMore> <element name="range"> - <attribute name="start"><ref name="ipv4-addr"/></attribute> - <attribute name="end"><ref name="ipv4-addr"/></attribute> + <attribute name="start"><ref name="ipv4Addr"/></attribute> + <attribute name="end"><ref name="ipv4Addr"/></attribute> </element> </zeroOrMore> <zeroOrMore> <element name="host"> - <attribute name="mac"><ref name="mac-addr"/></attribute> + <attribute name="mac"><ref name="macAddr"/></attribute> <attribute name="name"><text/></attribute> - <attribute name="ip"><ref name="ipv4-addr"/></attribute> + <attribute name="ip"><ref name="ipv4Addr"/></attribute> </element> </zeroOrMore> <optional> <element name="bootp"> - <attribute name="file"><text/></attribute> + <attribute name="file"><ref name="filePath"/></attribute> <optional> - <attribute name="server"><text/></attribute> + <attribute name="server"><ref name="dnsName"/></attribute> </optional> </element> </optional> @@ -163,52 +166,10 @@ </element> </define> - <!-- An ipv4 "dotted quad" address --> - <define name='ipv4-addr'> - <data type='string'> - <param name="pattern">(((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9]))\.){3}((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9]))</param> - </data> - </define> - - <!-- Based on http://blog.mes-stats.fr/2008/10/09/regex-ipv4-et-ipv6 --> - <define name='ipv6-addr'> - <data type='string'> - <!-- To understand this better, take apart the toplevel '|'s --> - <param name="pattern">(([0-9A-Fa-f]{1,4}:){7}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){6}:[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){5}:([0-9A-Fa-f]{1,4}:)?[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){4}:([0-9A-Fa-f]{1,4}:){0,2}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){3}:([0-9A-Fa-f]{1,4}:){0,3}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){2}:([0-9A-Fa-f]{1,4}:){0,4}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){6}(((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9]))\.){3}((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9])))|(([0-9A-Fa-f]{1,4}:){0,5}:(((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9]))\.){3}((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9])))|(::([0-9A-Fa-f]{1,4}:){0,5}(((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9]))\.){3}((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9])))|([0-9A-Fa-f]{1,4}::([0-9A-Fa-f]{1,4}:){0,5}[0-9A-Fa-f]{1,4})|(::([0-9A-Fa-f]{1,4}:){0,6}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){1,7}:)</param> - </data> - </define> - - <define name='ip-addr'> - <choice> - <ref name='ipv4-addr'/> - <ref name='ipv6-addr'/> - </choice> - </define> - - <define name='ip-prefix'> - <data type='unsignedInt'> - <param name="maxInclusive">128</param> - </data> - </define> - <define name='addr-family'> <data type='string'> <param name="pattern">(ipv4)|(ipv6)</param> </data> </define> - <!-- a 6 byte MAC address in ASCII-hex format, eg "12:34:56:78:9A:BC" --> - <define name='mac-addr'> - <data type='string'> - <param name="pattern">([a-fA-F0-9]{2}:){5}[a-fA-F0-9]{2}</param> - </data> - </define> - - <!-- a valid DNS name --> - <define name='dns-name'> - <data type='string'> - <param name="pattern">([a-zA-Z\-]+)</param> - </data> - </define> - </grammar> diff --git a/libvirt.spec.in b/libvirt.spec.in index feca5ad..bf220f3 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -1065,6 +1065,7 @@ fi %{_datadir}/libvirt/schemas/secret.rng %{_datadir}/libvirt/schemas/storageencryption.rng %{_datadir}/libvirt/schemas/nwfilter.rng +%{_datadir}/libvirt/schemas/basictypes.rng %{_datadir}/libvirt/cpu_map.xml -- 1.7.3.4

On Tue, Jul 05, 2011 at 03:45:52AM -0400, Laine Stump wrote:
domain.rng, network.rng, and interface.rng already use a few of the same types (or in some cases *should* but don't), and an upcoming code change will have them sharing even more. To prepare for that, this patch takes those common data type definitions and moves them into basictypes.rng.
This may break some rule about the need to RNG files to be autonomous or something, but I saw that storageencryption.rng is used in this way, so I figured it must not be completely against the law... --- docs/schemas/Makefile.am | 1 + docs/schemas/basictypes.rng | 130 +++++++++++++++++++++++++++++++++++++++++++ docs/schemas/domain.rng | 86 +--------------------------- docs/schemas/interface.rng | 71 ++++------------------- docs/schemas/network.rng | 79 +++++++------------------- libvirt.spec.in | 1 + 6 files changed, 168 insertions(+), 200 deletions(-) create mode 100644 docs/schemas/basictypes.rng
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On Tue, Jul 05, 2011 at 03:45:52AM -0400, Laine Stump wrote:
domain.rng, network.rng, and interface.rng already use a few of the same types (or in some cases *should* but don't), and an upcoming code change will have them sharing even more. To prepare for that, this patch takes those common data type definitions and moves them into basictypes.rng.
This may break some rule about the need to RNG files to be autonomous or something, but I saw that storageencryption.rng is used in this way, so I figured it must not be completely against the law... --- docs/schemas/Makefile.am | 1 + docs/schemas/basictypes.rng | 130 +++++++++++++++++++++++++++++++++++++++++++ docs/schemas/domain.rng | 86 +--------------------------- docs/schemas/interface.rng | 71 ++++------------------- docs/schemas/network.rng | 79 +++++++------------------- libvirt.spec.in | 1 + 6 files changed, 168 insertions(+), 200 deletions(-) create mode 100644 docs/schemas/basictypes.rng
Oh, you need to change mingw32-libvirt.spec.in too Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 07/05/2011 05:55 AM, Daniel P. Berrange wrote:
On Tue, Jul 05, 2011 at 03:45:52AM -0400, Laine Stump wrote:
domain.rng, network.rng, and interface.rng already use a few of the same types (or in some cases *should* but don't), and an upcoming code change will have them sharing even more. To prepare for that, this patch takes those common data type definitions and moves them into basictypes.rng.
This may break some rule about the need to RNG files to be autonomous or something, but I saw that storageencryption.rng is used in this way, so I figured it must not be completely against the law... --- docs/schemas/Makefile.am | 1 + docs/schemas/basictypes.rng | 130 +++++++++++++++++++++++++++++++++++++++++++ docs/schemas/domain.rng | 86 +--------------------------- docs/schemas/interface.rng | 71 ++++------------------- docs/schemas/network.rng | 79 +++++++------------------- libvirt.spec.in | 1 + 6 files changed, 168 insertions(+), 200 deletions(-) create mode 100644 docs/schemas/basictypes.rng Oh, you need to change mingw32-libvirt.spec.in too
Okay, I made that change and pushed the result. (also added a similar mingw32-libvirt.spec.in change to patch 06/10 (fornetworkcommon.rng).

virtPortProfiles are currently only used in the domain XML, but will soon also be used in the network XML. To prepare for that change, this patch moves the structure definition into util/network.h and the parse and format functions into util/network.c (I decided that this was a better choice than macvtap.h/c for something that needed to always be available on all platforms). Additionally, the virtPortProfile in the domain interface struct is now a separately allocated object rather *pointed to by* (rather than contained in) the main virDomainNetDef object. This is done to make is easier to figure out when a virtualPortProfile has/hasn't been specified in a particular config. --- src/conf/domain_conf.c | 208 +++------------------------------------------ src/conf/domain_conf.h | 2 +- src/libvirt_private.syms | 2 + src/qemu/qemu_command.c | 4 +- src/qemu/qemu_hotplug.c | 2 +- src/qemu/qemu_migration.c | 4 +- src/qemu/qemu_process.c | 2 +- src/util/macvtap.c | 6 +- src/util/macvtap.h | 36 +-------- src/util/network.c | 196 ++++++++++++++++++++++++++++++++++++++++++ src/util/network.h | 47 ++++++++++ 11 files changed, 271 insertions(+), 238 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 772422d..66a7c59 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -447,11 +447,6 @@ VIR_ENUM_IMPL(virDomainSeclabel, VIR_DOMAIN_SECLABEL_LAST, "dynamic", "static") -VIR_ENUM_IMPL(virVirtualPort, VIR_VIRTUALPORT_TYPE_LAST, - "none", - "802.1Qbg", - "802.1Qbh") - VIR_ENUM_IMPL(virDomainClockOffset, VIR_DOMAIN_CLOCK_OFFSET_LAST, "utc", "localtime", @@ -755,6 +750,7 @@ void virDomainNetDefFree(virDomainNetDefPtr def) case VIR_DOMAIN_NET_TYPE_DIRECT: VIR_FREE(def->data.direct.linkdev); + VIR_FREE(def->data.direct.virtPortProfile); break; case VIR_DOMAIN_NET_TYPE_USER: @@ -2571,146 +2567,6 @@ cleanup: } -static int -virVirtualPortProfileParamsParseXML(xmlNodePtr node, - virVirtualPortProfileParamsPtr virtPort) -{ - int ret = -1; - char *virtPortType; - char *virtPortManagerID = NULL; - char *virtPortTypeID = NULL; - char *virtPortTypeIDVersion = NULL; - char *virtPortInstanceID = NULL; - char *virtPortProfileID = NULL; - xmlNodePtr cur = node->children; - const char *msg = NULL; - - virtPortType = virXMLPropString(node, "type"); - if (!virtPortType) - return -1; - - while (cur != NULL) { - if (xmlStrEqual(cur->name, BAD_CAST "parameters")) { - - virtPortManagerID = virXMLPropString(cur, "managerid"); - virtPortTypeID = virXMLPropString(cur, "typeid"); - virtPortTypeIDVersion = virXMLPropString(cur, "typeidversion"); - virtPortInstanceID = virXMLPropString(cur, "instanceid"); - virtPortProfileID = virXMLPropString(cur, "profileid"); - - break; - } - - cur = cur->next; - } - - virtPort->virtPortType = VIR_VIRTUALPORT_NONE; - - switch (virVirtualPortTypeFromString(virtPortType)) { - - case VIR_VIRTUALPORT_8021QBG: - if (virtPortManagerID != NULL && virtPortTypeID != NULL && - virtPortTypeIDVersion != NULL) { - unsigned int val; - - if (virStrToLong_ui(virtPortManagerID, NULL, 0, &val)) { - msg = _("cannot parse value of managerid parameter"); - goto err_exit; - } - - if (val > 0xff) { - msg = _("value of managerid out of range"); - goto err_exit; - } - - virtPort->u.virtPort8021Qbg.managerID = (uint8_t)val; - - if (virStrToLong_ui(virtPortTypeID, NULL, 0, &val)) { - msg = _("cannot parse value of typeid parameter"); - goto err_exit; - } - - if (val > 0xffffff) { - msg = _("value for typeid out of range"); - goto err_exit; - } - - virtPort->u.virtPort8021Qbg.typeID = (uint32_t)val; - - if (virStrToLong_ui(virtPortTypeIDVersion, NULL, 0, &val)) { - msg = _("cannot parse value of typeidversion parameter"); - goto err_exit; - } - - if (val > 0xff) { - msg = _("value of typeidversion out of range"); - goto err_exit; - } - - virtPort->u.virtPort8021Qbg.typeIDVersion = (uint8_t)val; - - if (virtPortInstanceID != NULL) { - if (virUUIDParse(virtPortInstanceID, - virtPort->u.virtPort8021Qbg.instanceID)) { - msg = _("cannot parse instanceid parameter as a uuid"); - goto err_exit; - } - } else { - if (virUUIDGenerate(virtPort->u.virtPort8021Qbg.instanceID)) { - msg = _("cannot generate a random uuid for instanceid"); - goto err_exit; - } - } - - virtPort->virtPortType = VIR_VIRTUALPORT_8021QBG; - ret = 0; - } else { - msg = _("a parameter is missing for 802.1Qbg description"); - goto err_exit; - } - break; - - case VIR_VIRTUALPORT_8021QBH: - if (virtPortProfileID != NULL) { - if (virStrcpyStatic(virtPort->u.virtPort8021Qbh.profileID, - virtPortProfileID) != NULL) { - virtPort->virtPortType = VIR_VIRTUALPORT_8021QBH; - ret = 0; - } else { - msg = _("profileid parameter too long"); - goto err_exit; - } - } else { - msg = _("profileid parameter is missing for 802.1Qbh descripion"); - goto err_exit; - } - break; - - - default: - case VIR_VIRTUALPORT_NONE: - case VIR_VIRTUALPORT_TYPE_LAST: - msg = _("unknown virtualport type"); - goto err_exit; - break; - } - -err_exit: - - if (msg) - virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", msg); - - VIR_FREE(virtPortManagerID); - VIR_FREE(virtPortTypeID); - VIR_FREE(virtPortTypeIDVersion); - VIR_FREE(virtPortInstanceID); - VIR_FREE(virtPortProfileID); - VIR_FREE(virtPortType); - - return ret; -} - - /* Parse the XML definition for a network interface * @param node XML nodeset to parse for net definition * @return 0 on success, -1 on failure @@ -2742,8 +2598,7 @@ virDomainNetDefParseXML(virCapsPtr caps, char *devaddr = NULL; char *mode = NULL; virNWFilterHashTablePtr filterparams = NULL; - virVirtualPortProfileParams virtPort; - bool virtPortParsed = false; + virVirtualPortProfileParamsPtr virtPort = NULL; xmlNodePtr oldnode = ctxt->node; int ret; @@ -2789,12 +2644,15 @@ virDomainNetDefParseXML(virCapsPtr caps, xmlStrEqual(cur->name, BAD_CAST "source")) { dev = virXMLPropString(cur, "dev"); mode = virXMLPropString(cur, "mode"); - } else if (!virtPortParsed && + } else if ((virtPort == NULL) && (def->type == VIR_DOMAIN_NET_TYPE_DIRECT) && xmlStrEqual(cur->name, BAD_CAST "virtualport")) { - if (virVirtualPortProfileParamsParseXML(cur, &virtPort)) + const char *errmsg; + if (virVirtualPortProfileParamsParseXML(cur, &virtPort, &errmsg) < 0) { + if (errmsg) + virDomainReportError(VIR_ERR_XML_ERROR, "%s", errmsg); goto error; - virtPortParsed = true; + } } else if ((network == NULL) && ((def->type == VIR_DOMAIN_NET_TYPE_SERVER) || (def->type == VIR_DOMAIN_NET_TYPE_CLIENT) || @@ -2978,8 +2836,10 @@ virDomainNetDefParseXML(virCapsPtr caps, } else def->data.direct.mode = VIR_MACVTAP_MODE_VEPA; - if (virtPortParsed) + if (virtPort) { def->data.direct.virtPortProfile = virtPort; + virtPort = NULL; + } def->data.direct.linkdev = dev; dev = NULL; @@ -3087,6 +2947,7 @@ cleanup: VIR_FREE(port); VIR_FREE(ifname); VIR_FREE(dev); + VIR_FREE(virtPort); VIR_FREE(script); VIR_FREE(bridge); VIR_FREE(model); @@ -5270,49 +5131,6 @@ virDomainChrTargetTypeToString(int deviceType, return type; } -static void -virVirtualPortProfileFormat(virBufferPtr buf, - virVirtualPortProfileParamsPtr virtPort, - const char *indent) -{ - char uuidstr[VIR_UUID_STRING_BUFLEN]; - - if (virtPort->virtPortType == VIR_VIRTUALPORT_NONE) - return; - - virBufferAsprintf(buf, "%s<virtualport type='%s'>\n", - indent, - virVirtualPortTypeToString(virtPort->virtPortType)); - - switch (virtPort->virtPortType) { - case VIR_VIRTUALPORT_NONE: - case VIR_VIRTUALPORT_TYPE_LAST: - break; - - case VIR_VIRTUALPORT_8021QBG: - virUUIDFormat(virtPort->u.virtPort8021Qbg.instanceID, - uuidstr); - virBufferAsprintf(buf, - "%s <parameters managerid='%d' typeid='%d' " - "typeidversion='%d' instanceid='%s'/>\n", - indent, - virtPort->u.virtPort8021Qbg.managerID, - virtPort->u.virtPort8021Qbg.typeID, - virtPort->u.virtPort8021Qbg.typeIDVersion, - uuidstr); - break; - - case VIR_VIRTUALPORT_8021QBH: - virBufferAsprintf(buf, - "%s <parameters profileid='%s'/>\n", - indent, - virtPort->u.virtPort8021Qbh.profileID); - break; - } - - virBufferAsprintf(buf, "%s</virtualport>\n", indent); -} - int virDomainDiskIndexByName(virDomainDefPtr def, const char *name) { virDomainDiskDefPtr vdisk; @@ -8674,7 +8492,7 @@ virDomainNetDefFormat(virBufferPtr buf, virBufferAsprintf(buf, " mode='%s'", virMacvtapModeTypeToString(def->data.direct.mode)); virBufferAddLit(buf, "/>\n"); - virVirtualPortProfileFormat(buf, &def->data.direct.virtPortProfile, + virVirtualPortProfileFormat(buf, def->data.direct.virtPortProfile, " "); break; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 9d28f51..f8771a9 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -386,7 +386,7 @@ struct _virDomainNetDef { struct { char *linkdev; int mode; /* enum virMacvtapMode from util/macvtap.h */ - virVirtualPortProfileParams virtPortProfile; + virVirtualPortProfileParamsPtr virtPortProfile; } direct; } data; struct { diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 626ac6c..a7540d2 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -695,6 +695,8 @@ virSocketParseAddr; virSocketParseIpv4Addr; virSocketParseIpv6Addr; virSocketSetPort; +virVirtualPortProfileFormat; +virVirtualPortProfileParamsParseXML; # network_conf.h diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 6e4480e..bd47eae 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -127,7 +127,7 @@ qemuPhysIfaceConnect(virDomainDefPtr def, rc = openMacvtapTap(net->ifname, net->mac, net->data.direct.linkdev, net->data.direct.mode, vnet_hdr, def->uuid, - &net->data.direct.virtPortProfile, &res_ifname, + net->data.direct.virtPortProfile, &res_ifname, vmop, driver->stateDir); if (rc >= 0) { qemuAuditNetDevice(def, net, res_ifname, true); @@ -150,7 +150,7 @@ qemuPhysIfaceConnect(virDomainDefPtr def, VIR_FORCE_CLOSE(rc); delMacvtap(net->ifname, net->mac, net->data.direct.linkdev, net->data.direct.mode, - &net->data.direct.virtPortProfile, + net->data.direct.virtPortProfile, driver->stateDir); VIR_FREE(net->ifname); } diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index a7f11ab..5049f6e 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1611,7 +1611,7 @@ int qemuDomainDetachNetDevice(struct qemud_driver *driver, if (detach->type == VIR_DOMAIN_NET_TYPE_DIRECT) { delMacvtap(detach->ifname, detach->mac, detach->data.direct.linkdev, detach->data.direct.mode, - &detach->data.direct.virtPortProfile, + detach->data.direct.virtPortProfile, driver->stateDir); VIR_FREE(detach->ifname); } diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index d7b27a0..9622021 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2398,7 +2398,7 @@ qemuMigrationVPAssociatePortProfiles(virDomainDefPtr def) { if (vpAssociatePortProfileId(net->ifname, net->mac, net->data.direct.linkdev, - &net->data.direct.virtPortProfile, + net->data.direct.virtPortProfile, def->uuid, VIR_VM_OP_MIGRATE_IN_FINISH) != 0) goto err_exit; @@ -2415,7 +2415,7 @@ err_exit: vpDisassociatePortProfileId(net->ifname, net->mac, net->data.direct.linkdev, - &net->data.direct.virtPortProfile, + net->data.direct.virtPortProfile, VIR_VM_OP_MIGRATE_IN_FINISH); } } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f2c439b..fa7face 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2905,7 +2905,7 @@ void qemuProcessStop(struct qemud_driver *driver, if (net->type == VIR_DOMAIN_NET_TYPE_DIRECT) { delMacvtap(net->ifname, net->mac, net->data.direct.linkdev, net->data.direct.mode, - &net->data.direct.virtPortProfile, driver->stateDir); + net->data.direct.virtPortProfile, driver->stateDir); VIR_FREE(net->ifname); } } diff --git a/src/util/macvtap.c b/src/util/macvtap.c index 30343c8..052f76b 100644 --- a/src/util/macvtap.c +++ b/src/util/macvtap.c @@ -50,6 +50,7 @@ #include "util.h" #include "macvtap.h" +#include "network.h" VIR_ENUM_IMPL(virMacvtapMode, VIR_MACVTAP_MODE_LAST, "vepa", @@ -1089,7 +1090,7 @@ vpAssociatePortProfileId(const char *macvtap_ifname, VIR_DEBUG("%s: VM OPERATION: %s", __FUNCTION__, virVMOperationTypeToString(vmOp)); - if (vmOp == VIR_VM_OP_NO_OP) + if (!virtPort || vmOp == VIR_VM_OP_NO_OP) return 0; switch (virtPort->virtPortType) { @@ -1145,6 +1146,9 @@ vpDisassociatePortProfileId(const char *macvtap_ifname, VIR_DEBUG("%s: VM OPERATION: %s", __FUNCTION__, virVMOperationTypeToString(vmOp)); + if (!virtPort) + return 0; + switch (virtPort->virtPortType) { case VIR_VIRTUALPORT_NONE: case VIR_VIRTUALPORT_TYPE_LAST: diff --git a/src/util/macvtap.h b/src/util/macvtap.h index 1b85989..8e8613d 100644 --- a/src/util/macvtap.h +++ b/src/util/macvtap.h @@ -25,15 +25,6 @@ # include <config.h> - -enum virVirtualPortType { - VIR_VIRTUALPORT_NONE, - VIR_VIRTUALPORT_8021QBG, - VIR_VIRTUALPORT_8021QBH, - - VIR_VIRTUALPORT_TYPE_LAST, -}; - /* the mode type for macvtap devices */ enum virMacvtapMode { VIR_MACVTAP_MODE_VEPA, @@ -44,31 +35,6 @@ enum virMacvtapMode { VIR_MACVTAP_MODE_LAST, }; - -# ifdef IFLA_VF_PORT_PROFILE_MAX -# define LIBVIRT_IFLA_VF_PORT_PROFILE_MAX IFLA_VF_PORT_PROFILE_MAX -# else -# define LIBVIRT_IFLA_VF_PORT_PROFILE_MAX 40 -# endif - -/* profile data for macvtap (VEPA) */ -typedef struct _virVirtualPortProfileParams virVirtualPortProfileParams; -typedef virVirtualPortProfileParams *virVirtualPortProfileParamsPtr; -struct _virVirtualPortProfileParams { - enum virVirtualPortType virtPortType; - union { - struct { - uint8_t managerID; - uint32_t typeID; /* 24 bit valid */ - uint8_t typeIDVersion; - unsigned char instanceID[VIR_UUID_BUFLEN]; - } virtPort8021Qbg; - struct { - char profileID[LIBVIRT_IFLA_VF_PORT_PROFILE_MAX]; - } virtPort8021Qbh; - } u; -}; - enum virVMOperationType { VIR_VM_OP_CREATE, VIR_VM_OP_SAVE, @@ -85,6 +51,7 @@ enum virVMOperationType { # if WITH_MACVTAP # include "internal.h" +# include "network.h" int openMacvtapTap(const char *ifname, const unsigned char *macaddress, @@ -119,7 +86,6 @@ int vpDisassociatePortProfileId(const char *macvtap_ifname, # endif /* WITH_MACVTAP */ -VIR_ENUM_DECL(virVirtualPort) VIR_ENUM_DECL(virVMOperation) VIR_ENUM_DECL(virMacvtapMode) diff --git a/src/util/network.c b/src/util/network.c index eb16e0c..6f5458a 100644 --- a/src/util/network.c +++ b/src/util/network.c @@ -12,6 +12,7 @@ #include <arpa/inet.h> #include "memory.h" +#include "uuid.h" #include "network.h" #include "util.h" #include "virterror_internal.h" @@ -674,3 +675,198 @@ virSocketAddrPrefixToNetmask(unsigned int prefix, error: return result; } + +/* virtualPortProfile utilities */ + +VIR_ENUM_IMPL(virVirtualPort, VIR_VIRTUALPORT_TYPE_LAST, + "none", + "802.1Qbg", + "802.1Qbh") + +int +virVirtualPortProfileParamsParseXML(xmlNodePtr node, + virVirtualPortProfileParamsPtr *virtPort, + const char **errmsg) +{ + int ret = -1; + char *virtPortType; + char *virtPortManagerID = NULL; + char *virtPortTypeID = NULL; + char *virtPortTypeIDVersion = NULL; + char *virtPortInstanceID = NULL; + char *virtPortProfileID = NULL; + xmlNodePtr cur = node->children; + + *errmsg = NULL; + if (VIR_ALLOC(*virtPort) < 0) { + virReportOOMError(); + return -1; + } + + virtPortType = virXMLPropString(node, "type"); + if (!virtPortType) { + *errmsg = _("missing virtualportprofile type"); + goto error; + } + + while (cur != NULL) { + if (xmlStrEqual(cur->name, BAD_CAST "parameters")) { + + virtPortManagerID = virXMLPropString(cur, "managerid"); + virtPortTypeID = virXMLPropString(cur, "typeid"); + virtPortTypeIDVersion = virXMLPropString(cur, "typeidversion"); + virtPortInstanceID = virXMLPropString(cur, "instanceid"); + virtPortProfileID = virXMLPropString(cur, "profileid"); + + break; + } + + cur = cur->next; + } + + (*virtPort)->virtPortType = VIR_VIRTUALPORT_NONE; + + switch (virVirtualPortTypeFromString(virtPortType)) { + + case VIR_VIRTUALPORT_8021QBG: + if (virtPortManagerID != NULL && virtPortTypeID != NULL && + virtPortTypeIDVersion != NULL) { + unsigned int val; + + if (virStrToLong_ui(virtPortManagerID, NULL, 0, &val)) { + *errmsg = _("cannot parse value of managerid parameter"); + goto error; + } + + if (val > 0xff) { + *errmsg = _("value of managerid out of range"); + goto error; + } + + (*virtPort)->u.virtPort8021Qbg.managerID = (uint8_t)val; + + if (virStrToLong_ui(virtPortTypeID, NULL, 0, &val)) { + *errmsg = _("cannot parse value of typeid parameter"); + goto error; + } + + if (val > 0xffffff) { + *errmsg = _("value for typeid out of range"); + goto error; + } + + (*virtPort)->u.virtPort8021Qbg.typeID = (uint32_t)val; + + if (virStrToLong_ui(virtPortTypeIDVersion, NULL, 0, &val)) { + *errmsg = _("cannot parse value of typeidversion parameter"); + goto error; + } + + if (val > 0xff) { + *errmsg = _("value of typeidversion out of range"); + goto error; + } + + (*virtPort)->u.virtPort8021Qbg.typeIDVersion = (uint8_t)val; + + if (virtPortInstanceID != NULL) { + if (virUUIDParse(virtPortInstanceID, + (*virtPort)->u.virtPort8021Qbg.instanceID)) { + *errmsg = _("cannot parse instanceid parameter as a uuid"); + goto error; + } + } else { + if (virUUIDGenerate((*virtPort)->u.virtPort8021Qbg.instanceID)) { + *errmsg = _("cannot generate a random uuid for instanceid"); + goto error; + } + } + + (*virtPort)->virtPortType = VIR_VIRTUALPORT_8021QBG; + ret = 0; + } else { + *errmsg = _("a parameter is missing for 802.1Qbg description"); + goto error; + } + break; + + case VIR_VIRTUALPORT_8021QBH: + if (virtPortProfileID != NULL) { + if (virStrcpyStatic((*virtPort)->u.virtPort8021Qbh.profileID, + virtPortProfileID) != NULL) { + (*virtPort)->virtPortType = VIR_VIRTUALPORT_8021QBH; + ret = 0; + } else { + *errmsg = _("profileid parameter too long"); + goto error; + } + } else { + *errmsg = _("profileid parameter is missing for 802.1Qbh descripion"); + goto error; + } + break; + + + default: + case VIR_VIRTUALPORT_NONE: + case VIR_VIRTUALPORT_TYPE_LAST: + *errmsg = _("unknown virtualport type"); + goto error; + break; + } + +error: + if (ret < 0) + VIR_FREE(*virtPort); + VIR_FREE(virtPortManagerID); + VIR_FREE(virtPortTypeID); + VIR_FREE(virtPortTypeIDVersion); + VIR_FREE(virtPortInstanceID); + VIR_FREE(virtPortProfileID); + VIR_FREE(virtPortType); + + return ret; +} + +void +virVirtualPortProfileFormat(virBufferPtr buf, + virVirtualPortProfileParamsPtr virtPort, + const char *indent) +{ + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + if (!virtPort || virtPort->virtPortType == VIR_VIRTUALPORT_NONE) + return; + + virBufferAsprintf(buf, "%s<virtualport type='%s'>\n", + indent, + virVirtualPortTypeToString(virtPort->virtPortType)); + + switch (virtPort->virtPortType) { + case VIR_VIRTUALPORT_NONE: + case VIR_VIRTUALPORT_TYPE_LAST: + break; + + case VIR_VIRTUALPORT_8021QBG: + virUUIDFormat(virtPort->u.virtPort8021Qbg.instanceID, + uuidstr); + virBufferAsprintf(buf, + "%s <parameters managerid='%d' typeid='%d' " + "typeidversion='%d' instanceid='%s'/>\n", + indent, + virtPort->u.virtPort8021Qbg.managerID, + virtPort->u.virtPort8021Qbg.typeID, + virtPort->u.virtPort8021Qbg.typeIDVersion, + uuidstr); + break; + + case VIR_VIRTUALPORT_8021QBH: + virBufferAsprintf(buf, + "%s <parameters profileid='%s'/>\n", + indent, + virtPort->u.virtPort8021Qbh.profileID); + break; + } + + virBufferAsprintf(buf, "%s</virtualport>\n", indent); +} diff --git a/src/util/network.h b/src/util/network.h index ed0b78c..ab50178 100644 --- a/src/util/network.h +++ b/src/util/network.h @@ -12,6 +12,8 @@ # define __VIR_NETWORK_H__ # include "internal.h" +# include "buf.h" +# include "util.h" # include <sys/types.h> # include <sys/socket.h> @@ -20,6 +22,7 @@ # endif # include <netdb.h> # include <netinet/in.h> +# include <xml.h> typedef struct { union { @@ -90,4 +93,48 @@ int virSocketAddrPrefixToNetmask(unsigned int prefix, virSocketAddrPtr netmask, int family); +/* virtualPortProfile utilities */ +# ifdef IFLA_VF_PORT_PROFILE_MAX +# define LIBVIRT_IFLA_VF_PORT_PROFILE_MAX IFLA_VF_PORT_PROFILE_MAX +# else +# define LIBVIRT_IFLA_VF_PORT_PROFILE_MAX 40 +# endif + +enum virVirtualPortType { + VIR_VIRTUALPORT_NONE, + VIR_VIRTUALPORT_8021QBG, + VIR_VIRTUALPORT_8021QBH, + + VIR_VIRTUALPORT_TYPE_LAST, +}; + +VIR_ENUM_DECL(virVirtualPort) + +/* profile data for macvtap (VEPA) */ +typedef struct _virVirtualPortProfileParams virVirtualPortProfileParams; +typedef virVirtualPortProfileParams *virVirtualPortProfileParamsPtr; +struct _virVirtualPortProfileParams { + enum virVirtualPortType virtPortType; + union { + struct { + uint8_t managerID; + uint32_t typeID; /* 24 bit valid */ + uint8_t typeIDVersion; + unsigned char instanceID[VIR_UUID_BUFLEN]; + } virtPort8021Qbg; + struct { + char profileID[LIBVIRT_IFLA_VF_PORT_PROFILE_MAX]; + } virtPort8021Qbh; + } u; +}; + +int +virVirtualPortProfileParamsParseXML(xmlNodePtr node, + virVirtualPortProfileParamsPtr *virtPort, + const char **errmsg); +void +virVirtualPortProfileFormat(virBufferPtr buf, + virVirtualPortProfileParamsPtr virtPort, + const char *indent); + #endif /* __VIR_NETWORK_H__ */ -- 1.7.3.4

On 07/05/2011 01:45 AM, Laine Stump wrote:
virtPortProfiles are currently only used in the domain XML, but will soon also be used in the network XML. To prepare for that change, this patch moves the structure definition into util/network.h and the parse and format functions into util/network.c (I decided that this was a better choice than macvtap.h/c for something that needed to always be available on all platforms).
Additionally, the virtPortProfile in the domain interface struct is now a separately allocated object rather *pointed to by* (rather than
grammar nit - you can delete the first "rather"
contained in) the main virDomainNetDef object. This is done to make is
s/is/it/
easier to figure out when a virtualPortProfile has/hasn't been specified in a particular config. --- src/conf/domain_conf.c | 208 +++------------------------------------------ src/conf/domain_conf.h | 2 +- src/libvirt_private.syms | 2 + src/qemu/qemu_command.c | 4 +- src/qemu/qemu_hotplug.c | 2 +- src/qemu/qemu_migration.c | 4 +- src/qemu/qemu_process.c | 2 +- src/util/macvtap.c | 6 +- src/util/macvtap.h | 36 +-------- src/util/network.c | 196 ++++++++++++++++++++++++++++++++++++++++++ src/util/network.h | 47 ++++++++++ 11 files changed, 271 insertions(+), 238 deletions(-)
ACK. Looks like pretty clean code motion, plus fallout due to a slight parameter change and alteration of who is responsible for actually reporting error messages. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On Tue, Jul 05, 2011 at 03:45:53AM -0400, Laine Stump wrote:
virtPortProfiles are currently only used in the domain XML, but will soon also be used in the network XML. To prepare for that change, this patch moves the structure definition into util/network.h and the parse and format functions into util/network.c (I decided that this was a better choice than macvtap.h/c for something that needed to always be available on all platforms).
Additionally, the virtPortProfile in the domain interface struct is now a separately allocated object rather *pointed to by* (rather than contained in) the main virDomainNetDef object. This is done to make is easier to figure out when a virtualPortProfile has/hasn't been specified in a particular config. --- src/conf/domain_conf.c | 208 +++------------------------------------------ src/conf/domain_conf.h | 2 +- src/libvirt_private.syms | 2 + src/qemu/qemu_command.c | 4 +- src/qemu/qemu_hotplug.c | 2 +- src/qemu/qemu_migration.c | 4 +- src/qemu/qemu_process.c | 2 +- src/util/macvtap.c | 6 +- src/util/macvtap.h | 36 +-------- src/util/network.c | 196 ++++++++++++++++++++++++++++++++++++++++++ src/util/network.h | 47 ++++++++++ 11 files changed, 271 insertions(+), 238 deletions(-)
ACK, a little disappointing that we have to put this in src/util instead of a common file in src/conf/, but it looks like we have a compile time dep to macvtap forcing us to put it in util. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

This adds <virtualportprofile> and <portgroup> (which itself can contain a <portgroup>) to <network>, adds several new options to forward mode, and adds an optional pool of interfaces to <forward>. Since virtualPortProfile is now used in both the domain and network RNG, its definition was moved into a separate file that is included by both. --- docs/schemas/Makefile.am | 1 + docs/schemas/domain.rng | 54 ++++++--------------------------------- docs/schemas/network.rng | 29 +++++++++++++++++++++ docs/schemas/networkcommon.rng | 50 +++++++++++++++++++++++++++++++++++++ libvirt.spec.in | 1 + 5 files changed, 90 insertions(+), 45 deletions(-) create mode 100644 docs/schemas/networkcommon.rng diff --git a/docs/schemas/Makefile.am b/docs/schemas/Makefile.am index 5ef7737..75a0e73 100644 --- a/docs/schemas/Makefile.am +++ b/docs/schemas/Makefile.am @@ -8,6 +8,7 @@ schema_DATA = \ domainsnapshot.rng \ interface.rng \ network.rng \ + networkcommon.rng \ nodedev.rng \ nwfilter.rng \ secret.rng \ diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng index 3c8414e..65572df 100644 --- a/docs/schemas/domain.rng +++ b/docs/schemas/domain.rng @@ -7,7 +7,7 @@ <include href='basictypes.rng'/> <include href='storageencryption.rng'/> - + <include href='networkcommon.rng'/> <!-- description element, maybe placed anywhere under the root --> @@ -1022,6 +1022,14 @@ </attribute> <empty/> </element> + <optional> + <attribute name="portgroup"> + <ref name="deviceName"/> + </attribute> + </optional> + <optional> + <ref name="virtualPortProfile"/> + </optional> <ref name="interface-options"/> </interleave> </group> @@ -1167,45 +1175,6 @@ </optional> </interleave> </define> - <define name="virtualPortProfile"> - <choice> - <group> - <element name="virtualport"> - <attribute name="type"> - <value>802.1Qbg</value> - </attribute> - <element name="parameters"> - <attribute name="managerid"> - <ref name="uint8range"/> - </attribute> - <attribute name="typeid"> - <ref name="uint24range"/> - </attribute> - <attribute name="typeidversion"> - <ref name="uint8range"/> - </attribute> - <optional> - <attribute name="instanceid"> - <ref name="UUID"/> - </attribute> - </optional> - </element> - </element> - </group> - <group> - <element name="virtualport"> - <attribute name="type"> - <value>802.1Qbh</value> - </attribute> - <element name="parameters"> - <attribute name="profileid"> - <ref name="virtualPortProfileID"/> - </attribute> - </element> - </element> - </group> - </choice> - </define> <!-- An emulator description is just a path to the binary used for the task --> @@ -2483,9 +2452,4 @@ <param name="pattern">[a-zA-Z0-9_\.:]+</param> </data> </define> - <define name="virtualPortProfileID"> - <data type="string"> - <param name="maxLength">39</param> - </data> - </define> </grammar> diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng index 6d9f06b..9e667e6 100644 --- a/docs/schemas/network.rng +++ b/docs/schemas/network.rng @@ -7,6 +7,7 @@ </start> <include href='basictypes.rng'/> + <include href='networkcommon.rng'/> <define name="network"> @@ -77,9 +78,37 @@ <choice> <value>nat</value> <value>route</value> + <value>bridge</value> + <value>passthrough</value> + <value>private</value> + <value>vepa</value> </choice> </attribute> </optional> + <zeroOrMore> + <element name='interface'> + <attribute name='dev'> + <ref name='deviceName'/> + </attribute> + </element> + </zeroOrMore> + </element> + </optional> + + <!-- <virtualport> element --> + <optional> + <ref name="virtualPortProfile"/> + </optional> + + <!-- <portgroup> elements --> + <optional> + <element name='portgroup'> + <attribute name='engineering'> + <ref name='deviceName'/> + </attribute> + <optional> + <ref name="virtualPortProfile"/> + </optional> </element> </optional> diff --git a/docs/schemas/networkcommon.rng b/docs/schemas/networkcommon.rng new file mode 100644 index 0000000..0251813 --- /dev/null +++ b/docs/schemas/networkcommon.rng @@ -0,0 +1,50 @@ +<?xml version="1.0"?> +<!-- network-related definitions used in multiple grammars --> +<grammar xmlns="http://relaxng.org/ns/structure/1.0" datatypeLibrary="http://www.w3.org/2001/XMLSchema-datatypes"> + + <define name="virtualPortProfileID"> + <data type="string"> + <param name="maxLength">39</param> + </data> + </define> + + <define name="virtualPortProfile"> + <choice> + <group> + <element name="virtualport"> + <attribute name="type"> + <value>802.1Qbg</value> + </attribute> + <element name="parameters"> + <attribute name="managerid"> + <ref name="uint8range"/> + </attribute> + <attribute name="typeid"> + <ref name="uint24range"/> + </attribute> + <attribute name="typeidversion"> + <ref name="uint8range"/> + </attribute> + <optional> + <attribute name="instanceid"> + <ref name="UUID"/> + </attribute> + </optional> + </element> + </element> + </group> + <group> + <element name="virtualport"> + <attribute name="type"> + <value>802.1Qbh</value> + </attribute> + <element name="parameters"> + <attribute name="profileid"> + <ref name="virtualPortProfileID"/> + </attribute> + </element> + </element> + </group> + </choice> + </define> +</grammar> diff --git a/libvirt.spec.in b/libvirt.spec.in index bf220f3..8c059f8 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -1066,6 +1066,7 @@ fi %{_datadir}/libvirt/schemas/storageencryption.rng %{_datadir}/libvirt/schemas/nwfilter.rng %{_datadir}/libvirt/schemas/basictypes.rng +%{_datadir}/libvirt/schemas/networkcommon.rng %{_datadir}/libvirt/cpu_map.xml -- 1.7.3.4

On Tue, Jul 05, 2011 at 03:45:54AM -0400, Laine Stump wrote:
This adds <virtualportprofile> and <portgroup> (which itself can contain a <portgroup>) to <network>, adds several new options to forward mode, and adds an optional pool of interfaces to <forward>.
Since virtualPortProfile is now used in both the domain and network RNG, its definition was moved into a separate file that is included by both. --- docs/schemas/Makefile.am | 1 + docs/schemas/domain.rng | 54 ++++++--------------------------------- docs/schemas/network.rng | 29 +++++++++++++++++++++ docs/schemas/networkcommon.rng | 50 +++++++++++++++++++++++++++++++++++++ libvirt.spec.in | 1 + 5 files changed, 90 insertions(+), 45 deletions(-) create mode 100644 docs/schemas/networkcommon.rng
ACK, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

This implements the changes to <network> and domain <interface> XML that were earlier specified in the RNG. Each virDomainNetDef now also potentially has a virDomainActualNetDef which is a private object (never exported/imported via the public API, and not defined in the RNG) that is used to maintain information about the physical device that was actually used for a NetDef that was of type VIR_DOMAIN_NET_TYPE_NETWORK. The virDomainActualNetDef will only be parsed/formatted if the parse/format function is called with the VIR_DOMAIN_XML_ACTUAL_NET flags set (which is only needed when saving/loading a running domain's state info to the stateDir). To prevent this flag bit from accidentally being used in the public API, a "RESERVED" placeholder was put into the public flags enum (at the same time, I noticed there was another private flag that hadn't been reserved, so I added that one, making both of these flags #defined from the public RESERVED flags, and since it was also only used in domain_conf.c, I unpolluted domain_conf.h, putting both #defines in domain_conf.c. A small change is also made to two functions in bridge_driver.c, to prevent a bridge device name and mac address from being automatically added to a network definition when it is of one of the new forward types (none of which use bridge devices managed by libvirt). --- include/libvirt/libvirt.h.in | 2 + src/conf/domain_conf.c | 276 +++++++++++++++++++++++++++++++++++- src/conf/domain_conf.h | 46 ++++++- src/conf/network_conf.c | 321 +++++++++++++++++++++++++++++++++++++----- src/conf/network_conf.h | 34 ++++- src/libvirt_private.syms | 8 +- src/network/bridge_driver.c | 28 +++- 7 files changed, 657 insertions(+), 58 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 8e20f75..b88c96e 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -1112,6 +1112,8 @@ typedef enum { VIR_DOMAIN_XML_SECURE = (1 << 0), /* dump security sensitive information too */ VIR_DOMAIN_XML_INACTIVE = (1 << 1), /* dump inactive domain information */ VIR_DOMAIN_XML_UPDATE_CPU = (1 << 2), /* update guest CPU requirements according to host CPU */ + VIR_DOMAIN_XML_RESERVED1 = (1 << 30), /* reserved for internal used */ + VIR_DOMAIN_XML_RESERVED2 = (1 << 31), /* reserved for internal used */ } virDomainXMLFlags; char * virDomainGetXMLDesc (virDomainPtr domain, diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 66a7c59..d5a3387 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -489,6 +489,11 @@ VIR_ENUM_IMPL(virDomainNumatuneMemMode, VIR_DOMAIN_NUMATUNE_MEM_LAST, #define VIR_DOMAIN_XML_WRITE_FLAGS VIR_DOMAIN_XML_SECURE #define VIR_DOMAIN_XML_READ_FLAGS VIR_DOMAIN_XML_INACTIVE +/* these flags are only used internally */ +/* dump internal domain status information */ +#define VIR_DOMAIN_XML_INTERNAL_STATUS VIR_DOMAIN_XML_RESERVED1 +/* dump virDomainActualNetDef contents */ +#define VIR_DOMAIN_XML_ACTUAL_NET VIR_DOMAIN_XML_RESERVED2 static void virDomainObjListDataFree(void *payload, const void *name ATTRIBUTE_UNUSED) @@ -714,6 +719,25 @@ void virDomainFSDefFree(virDomainFSDefPtr def) VIR_FREE(def); } +void +virDomainActualNetDefFree(virDomainActualNetDefPtr def) +{ + if (!def) + return; + + switch (def->type) { + case VIR_DOMAIN_NET_TYPE_BRIDGE: + VIR_FREE(def->data.bridge.brname); + break; + case VIR_DOMAIN_NET_TYPE_DIRECT: + VIR_FREE(def->data.direct.linkdev); + VIR_FREE(def->data.direct.virtPortProfile); + break; + default: + break; + } +} + void virDomainNetDefFree(virDomainNetDefPtr def) { if (!def) @@ -736,6 +760,9 @@ void virDomainNetDefFree(virDomainNetDefPtr def) case VIR_DOMAIN_NET_TYPE_NETWORK: VIR_FREE(def->data.network.name); + VIR_FREE(def->data.network.portgroup); + VIR_FREE(def->data.network.virtPortProfile); + virDomainActualNetDefFree(def->data.network.actual); break; case VIR_DOMAIN_NET_TYPE_BRIDGE: @@ -2566,6 +2593,85 @@ cleanup: goto cleanup; } +static int +virDomainActualNetDefParseXML(xmlNodePtr node, + xmlXPathContextPtr ctxt, + virDomainActualNetDefPtr *actual) +{ + int ret = -1; + xmlNodePtr save_ctxt = ctxt->node; + char *type = NULL; + char *mode = NULL; + + if (VIR_ALLOC(*actual) < 0) { + virReportOOMError(); + return -1; + } + + ctxt->node = node; + + type = virXMLPropString(node, "type"); + if (!type) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing type attribute in interface's <actual> element")); + goto error; + } + if ((int)((*actual)->type = virDomainNetTypeFromString(type)) < 0) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown type '%s' in interface's <actual> element"), type); + goto error; + } + if ((*actual)->type != VIR_DOMAIN_NET_TYPE_BRIDGE && + (*actual)->type != VIR_DOMAIN_NET_TYPE_DIRECT) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + _("unsupported type '%s' in interface's <actual> element"), + type); + goto error; + } + + if ((*actual)->type == VIR_DOMAIN_NET_TYPE_BRIDGE) { + + (*actual)->data.bridge.brname = virXPathString("string(./source[1]/@bridge)", + ctxt); + + } else if ((*actual)->type == VIR_DOMAIN_NET_TYPE_DIRECT) { + xmlNodePtr virtPortNode; + const char *errmsg; + + (*actual)->data.direct.linkdev = virXPathString("string(./source[1]/@dev)", ctxt); + + mode = virXPathString("string(./source[1]/@mode)", ctxt); + if (mode) { + int m; + if ((m = virMacvtapModeTypeFromString(mode)) < 0) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + _("Unkown mode '%s' in interface <actual> element"), + mode); + goto error; + } + (*actual)->data.direct.mode = m; + } + + virtPortNode = virXPathNode("./virtualport", ctxt); + if (virtPortNode && + virVirtualPortProfileParamsParseXML(virtPortNode, + &(*actual)->data.direct.virtPortProfile, + &errmsg) < 0) { + if (errmsg) + virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s in interface <actual>", + errmsg); + goto error; + } + } + + ret = 0; +error: + VIR_FREE(type); + VIR_FREE(mode); + + ctxt->node = save_ctxt; + return ret; +} /* Parse the XML definition for a network interface * @param node XML nodeset to parse for net definition @@ -2583,6 +2689,7 @@ virDomainNetDefParseXML(virCapsPtr caps, char *macaddr = NULL; char *type = NULL; char *network = NULL; + char *portgroup = NULL; char *bridge = NULL; char *dev = NULL; char *ifname = NULL; @@ -2599,6 +2706,7 @@ virDomainNetDefParseXML(virCapsPtr caps, char *mode = NULL; virNWFilterHashTablePtr filterparams = NULL; virVirtualPortProfileParamsPtr virtPort = NULL; + virDomainActualNetDefPtr actual = NULL; xmlNodePtr oldnode = ctxt->node; int ret; @@ -2630,6 +2738,7 @@ virDomainNetDefParseXML(virCapsPtr caps, (def->type == VIR_DOMAIN_NET_TYPE_NETWORK) && (xmlStrEqual(cur->name, BAD_CAST "source"))) { network = virXMLPropString(cur, "network"); + portgroup = virXMLPropString(cur, "portgroup"); } else if ((internal == NULL) && (def->type == VIR_DOMAIN_NET_TYPE_INTERNAL) && (xmlStrEqual(cur->name, BAD_CAST "source"))) { @@ -2645,7 +2754,8 @@ virDomainNetDefParseXML(virCapsPtr caps, dev = virXMLPropString(cur, "dev"); mode = virXMLPropString(cur, "mode"); } else if ((virtPort == NULL) && - (def->type == VIR_DOMAIN_NET_TYPE_DIRECT) && + ((def->type == VIR_DOMAIN_NET_TYPE_DIRECT) || + (def->type == VIR_DOMAIN_NET_TYPE_NETWORK)) && xmlStrEqual(cur->name, BAD_CAST "virtualport")) { const char *errmsg; if (virVirtualPortProfileParamsParseXML(cur, &virtPort, &errmsg) < 0) { @@ -2697,6 +2807,12 @@ virDomainNetDefParseXML(virCapsPtr caps, if (virDomainDeviceBootParseXML(cur, &def->bootIndex, bootMap)) goto error; + } else if ((actual == NULL) && + (flags & VIR_DOMAIN_XML_ACTUAL_NET) && + (def->type == VIR_DOMAIN_NET_TYPE_NETWORK) && + xmlStrEqual(cur->name, BAD_CAST "actual")) { + if (virDomainActualNetDefParseXML(cur, ctxt, &actual) < 0) + goto error; } } cur = cur->next; @@ -2745,6 +2861,12 @@ virDomainNetDefParseXML(virCapsPtr caps, } def->data.network.name = network; network = NULL; + def->data.network.portgroup = portgroup; + portgroup = NULL; + def->data.network.virtPortProfile = virtPort; + virtPort = NULL; + def->data.network.actual = actual; + actual = NULL; break; case VIR_DOMAIN_NET_TYPE_ETHERNET: @@ -2836,10 +2958,8 @@ virDomainNetDefParseXML(virCapsPtr caps, } else def->data.direct.mode = VIR_MACVTAP_MODE_VEPA; - if (virtPort) { - def->data.direct.virtPortProfile = virtPort; - virtPort = NULL; - } + def->data.direct.virtPortProfile = virtPort; + virtPort = NULL; def->data.direct.linkdev = dev; dev = NULL; @@ -2943,11 +3063,13 @@ cleanup: ctxt->node = oldnode; VIR_FREE(macaddr); VIR_FREE(network); + VIR_FREE(portgroup); VIR_FREE(address); VIR_FREE(port); VIR_FREE(ifname); VIR_FREE(dev); VIR_FREE(virtPort); + virDomainActualNetDefFree(actual); VIR_FREE(script); VIR_FREE(bridge); VIR_FREE(model); @@ -8421,6 +8543,67 @@ virDomainFSDefFormat(virBufferPtr buf, } static int +virDomainActualNetDefFormat(virBufferPtr buf, + virDomainActualNetDefPtr def, + int flags) { + int ret = -1; + const char *type; + const char *mode; + + if (!(def && (flags & VIR_DOMAIN_XML_INTERNAL_STATUS))) + return 0; + + type = virDomainNetTypeToString(def->type); + if (!type) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected net type %d"), def->type); + return ret; + } + + if (def->type != VIR_DOMAIN_NET_TYPE_BRIDGE && + def->type != VIR_DOMAIN_NET_TYPE_DIRECT) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected net type %s"), type); + goto error; + } + virBufferAsprintf(buf, " <actual type='%s'>\n", type); + + switch (def->type) { + case VIR_DOMAIN_NET_TYPE_BRIDGE: + if (def->data.bridge.brname) { + virBufferEscapeString(buf, " <source bridge='%s'/>\n", + def->data.bridge.brname); + } + break; + + case VIR_DOMAIN_NET_TYPE_DIRECT: + virBufferAddLit(buf, " <source"); + if (def->data.direct.linkdev) + virBufferEscapeString(buf, " dev='%s'", + def->data.direct.linkdev); + + mode = virMacvtapModeTypeToString(def->data.direct.mode); + if (!mode) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected source mode %d"), + def->data.direct.mode); + return ret; + } + virBufferAsprintf(buf, " mode='%s'/>\n", mode); + virVirtualPortProfileFormat(buf, def->data.direct.virtPortProfile, + " "); + break; + default: + break; + } + virBufferAddLit(buf, " </actual>\n"); + + ret = 0; +error: + return ret; +} + +static int virDomainNetDefFormat(virBufferPtr buf, virDomainNetDefPtr def, int flags) @@ -8443,8 +8626,17 @@ virDomainNetDefFormat(virBufferPtr buf, switch (def->type) { case VIR_DOMAIN_NET_TYPE_NETWORK: - virBufferEscapeString(buf, " <source network='%s'/>\n", + virBufferEscapeString(buf, " <source network='%s'", def->data.network.name); + if (def->data.network.portgroup) { + virBufferEscapeString(buf, " portgroup='%s'", + def->data.network.portgroup); + } + virBufferAddLit(buf, "/>\n"); + virVirtualPortProfileFormat(buf, def->data.network.virtPortProfile, + " "); + if (virDomainActualNetDefFormat(buf, def->data.network.actual, flags) < 0) + return -1; break; case VIR_DOMAIN_NET_TYPE_ETHERNET: @@ -9871,7 +10063,9 @@ int virDomainSaveStatus(virCapsPtr caps, const char *statusDir, virDomainObjPtr obj) { - int flags = VIR_DOMAIN_XML_SECURE|VIR_DOMAIN_XML_INTERNAL_STATUS; + int flags = VIR_DOMAIN_XML_SECURE | + VIR_DOMAIN_XML_INTERNAL_STATUS | + VIR_DOMAIN_XML_ACTUAL_NET; int ret = -1; char *xml; @@ -9960,7 +10154,8 @@ static virDomainObjPtr virDomainLoadStatus(virCapsPtr caps, goto error; if (!(obj = virDomainObjParseFile(caps, statusFile, - VIR_DOMAIN_XML_INTERNAL_STATUS))) + VIR_DOMAIN_XML_INTERNAL_STATUS | + VIR_DOMAIN_XML_ACTUAL_NET))) goto error; virUUIDFormat(obj->def->uuid, uuidstr); @@ -10946,3 +11141,68 @@ virDomainStateReasonFromString(virDomainState state, const char *reason) return -1; } + + +/* some access functions to gloss over the difference between NetDef + * (<interface>) and ActualNetDef (<actual>). If the NetDef has an + * ActualNetDef, return the requested value from the ActualNetDef, + * otherwise return the value from the NetDef. + */ + +int +virDomainNetGetActualType(virDomainNetDefPtr iface) +{ + if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK) + return iface->type; + if (!iface->data.network.actual) + return iface->type; + return iface->data.network.actual->type; +} + +char * +virDomainNetGetActualBridgeName(virDomainNetDefPtr iface) +{ + if (iface->type == VIR_DOMAIN_NET_TYPE_BRIDGE) + return iface->data.bridge.brname; + if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK) + return NULL; + if (!iface->data.network.actual) + return NULL; + return iface->data.network.actual->data.bridge.brname; +} + +char * +virDomainNetGetActualDirectDev(virDomainNetDefPtr iface) +{ + if (iface->type == VIR_DOMAIN_NET_TYPE_DIRECT) + return iface->data.direct.linkdev; + if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK) + return NULL; + if (!iface->data.network.actual) + return NULL; + return iface->data.network.actual->data.direct.linkdev; +} + +int +virDomainNetGetActualDirectMode(virDomainNetDefPtr iface) +{ + if (iface->type == VIR_DOMAIN_NET_TYPE_DIRECT) + return iface->data.direct.mode; + if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK) + return 0; + if (!iface->data.network.actual) + return 0; + return iface->data.network.actual->data.direct.mode; +} + +virVirtualPortProfileParamsPtr +virDomainNetGetActualDirectVirtPortProfile(virDomainNetDefPtr iface) +{ + if (iface->type == VIR_DOMAIN_NET_TYPE_DIRECT) + return iface->data.direct.virtPortProfile; + if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK) + return NULL; + if (!iface->data.network.actual) + return NULL; + return iface->data.network.actual->data.direct.virtPortProfile; +} diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index f8771a9..5807e77 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1,3 +1,4 @@ + /* * domain_conf.h: domain XML processing * @@ -41,11 +42,6 @@ # include "macvtap.h" # include "sysinfo.h" -/* Private component of virDomainXMLFlags */ -typedef enum { - VIR_DOMAIN_XML_INTERNAL_STATUS = (1<<16), /* dump internal domain status information */ -} virDomainXMLInternalFlags; - /* Different types of hypervisor */ /* NB: Keep in sync with virDomainVirtTypeToString impl */ enum virDomainVirtType { @@ -348,6 +344,27 @@ enum virDomainNetVirtioTxModeType { VIR_DOMAIN_NET_VIRTIO_TX_MODE_LAST, }; +/* Config that was actually used to bring up interface, after + * resolving network reference. This is private data, only used within + * libvirt, but still must maintain backward compatibility, because + * different versions of libvirt may read the same data file. + */ +typedef struct _virDomainActualNetDef virDomainActualNetDef; +typedef virDomainActualNetDef *virDomainActualNetDefPtr; +struct _virDomainActualNetDef { + enum virDomainNetType type; + union { + struct { + char *brname; + } bridge; + struct { + char *linkdev; + int mode; /* enum virMacvtapMode from util/macvtap.h */ + virVirtualPortProfileParamsPtr virtPortProfile; + } direct; + } data; +}; + /* Stores the virtual network interface configuration */ typedef struct _virDomainNetDef virDomainNetDef; typedef virDomainNetDef *virDomainNetDefPtr; @@ -374,6 +391,17 @@ struct _virDomainNetDef { } socket; /* any of NET_CLIENT or NET_SERVER or NET_MCAST */ struct { char *name; + char *portgroup; + virVirtualPortProfileParamsPtr virtPortProfile; + /* actual has info about the currently used physical + * device (if the network is of type + * bridge/private/vepa/passthrough). This is saved in the + * domain state, but never written to persistent config, + * since it needs to be re-allocated whenever the domain + * is restarted. It is also never shown to the user, and + * the user cannot specify it in XML documents. + */ + virDomainActualNetDefPtr actual; } network; struct { char *brname; @@ -1319,6 +1347,7 @@ void virDomainDiskDefFree(virDomainDiskDefPtr def); void virDomainDiskHostDefFree(virDomainDiskHostDefPtr def); void virDomainControllerDefFree(virDomainControllerDefPtr def); void virDomainFSDefFree(virDomainFSDefPtr def); +void virDomainActualNetDefFree(virDomainActualNetDefPtr def); void virDomainNetDefFree(virDomainNetDefPtr def); void virDomainSmartcardDefFree(virDomainSmartcardDefPtr def); void virDomainChrDefFree(virDomainChrDefPtr def); @@ -1430,6 +1459,13 @@ int virDomainNetIndexByMac(virDomainDefPtr def, const unsigned char *mac); int virDomainNetInsert(virDomainDefPtr def, virDomainNetDefPtr net); int virDomainNetRemoveByMac(virDomainDefPtr def, const unsigned char *mac); +int virDomainNetGetActualType(virDomainNetDefPtr iface); +char *virDomainNetGetActualBridgeName(virDomainNetDefPtr iface); +char *virDomainNetGetActualDirectDev(virDomainNetDefPtr iface); +int virDomainNetGetActualDirectMode(virDomainNetDefPtr iface); +virVirtualPortProfileParamsPtr +virDomainNetGetActualDirectVirtPortProfile(virDomainNetDefPtr iface); + int virDomainControllerInsert(virDomainDefPtr def, virDomainControllerDefPtr controller); void virDomainControllerInsertPreAlloced(virDomainDefPtr def, diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 45ddee2..3898945 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -50,7 +50,7 @@ VIR_ENUM_DECL(virNetworkForward) VIR_ENUM_IMPL(virNetworkForward, VIR_NETWORK_FORWARD_LAST, - "none", "nat", "route" ) + "none", "nat", "route", "bridge", "private", "vepa", "passthrough" ) #define virNetworkReportError(code, ...) \ virReportErrorHelper(VIR_FROM_NETWORK, code, __FILE__, \ @@ -87,6 +87,19 @@ virNetworkObjPtr virNetworkFindByName(const virNetworkObjListPtr nets, } +static void +virPortGroupDefClear(virPortGroupDefPtr def) +{ + VIR_FREE(def->name); + VIR_FREE(def->virtPortProfile); +} + +static void +virNetworkForwardIfDefClear(virNetworkForwardIfDefPtr def) +{ + VIR_FREE(def->dev); +} + static void virNetworkIpDefClear(virNetworkIpDefPtr def) { int ii; @@ -138,11 +151,21 @@ void virNetworkDefFree(virNetworkDefPtr def) VIR_FREE(def->forwardDev); VIR_FREE(def->domain); + for (ii = 0 ; ii < def->nForwardIfs && def->forwardIfs ; ii++) { + virNetworkForwardIfDefClear(&def->forwardIfs[ii]); + } + VIR_FREE(def->forwardIfs); + for (ii = 0 ; ii < def->nips && def->ips ; ii++) { virNetworkIpDefClear(&def->ips[ii]); } VIR_FREE(def->ips); + for (ii = 0; ii < def->nPortGroups && def->portGroups; ii++) { + virPortGroupDefClear(&def->portGroups[ii]); + } + VIR_FREE(def->portGroups); + virNetworkDNSDefFree(def->dns); VIR_FREE(def); @@ -735,14 +758,69 @@ error: return result; } +static int +virNetworkPortGroupParseXML(const char *networkName, + virPortGroupDefPtr def, + xmlNodePtr node, + xmlXPathContextPtr ctxt) +{ + /* + * virPortGroupDef object is already allocated as part of an array. + * On failure clear it out, but don't free it. + */ + + xmlNodePtr save; + xmlNodePtr virtPortNode; + char *isDefault = NULL; + + int result = -1; + + save = ctxt->node; + ctxt->node = node; + + /* grab raw data from XML */ + def->name = virXPathString("string(./@name)", ctxt); + isDefault = virXPathString("string(./@default)", ctxt); + def->isDefault = isDefault && STRCASEEQ(isDefault, "yes"); + + virtPortNode = virXPathNode("./virtualport", ctxt); + if (virtPortNode) { + const char *errmsg; + if (virVirtualPortProfileParamsParseXML(virtPortNode, + &def->virtPortProfile, + &errmsg) < 0) { + if (errmsg) + virNetworkReportError(VIR_ERR_XML_ERROR, "%s (in network %s)", + errmsg, networkName); + goto error; + } + } + + result = 0; +error: + if (result < 0) { + virPortGroupDefClear(def); + } + VIR_FREE(isDefault); + + ctxt->node = save; + return result; +} + static virNetworkDefPtr virNetworkDefParseXML(xmlXPathContextPtr ctxt) { virNetworkDefPtr def; char *tmp; + char *stp = NULL; xmlNodePtr *ipNodes = NULL; + xmlNodePtr *portGroupNodes = NULL; + xmlNodePtr *forwardIfNodes = NULL; xmlNodePtr dnsNode = NULL; - int nIps; + xmlNodePtr virtPortNode = NULL; + xmlNodePtr forwardNode = NULL; + int nIps, nPortGroups, nForwardIfs; + xmlNodePtr save = ctxt->node; if (VIR_ALLOC(def) < 0) { virReportOOMError(); @@ -779,9 +857,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) /* Parse bridge information */ def->bridge = virXPathString("string(./bridge[1]/@name)", ctxt); - tmp = virXPathString("string(./bridge[1]/@stp)", ctxt); - def->stp = (tmp && STREQ(tmp, "off")) ? 0 : 1; - VIR_FREE(tmp); + stp = virXPathString("string(./bridge[1]/@stp)", ctxt); if (virXPathULong("string(./bridge[1]/@delay)", ctxt, &def->delay) < 0) def->delay = 0; @@ -805,6 +881,42 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) goto error; } + virtPortNode = virXPathNode("./virtualport", ctxt); + if (virtPortNode) { + const char *errmsg; + if (virVirtualPortProfileParamsParseXML(virtPortNode, + &def->virtPortProfile, + &errmsg) < 0) { + if (errmsg) + virNetworkReportError(VIR_ERR_XML_ERROR, "%s", errmsg); + goto error; + } + } + + nPortGroups = virXPathNodeSet("./portgroup", ctxt, &portGroupNodes); + if (nPortGroups < 0) + goto error; + + if (nPortGroups > 0) { + int ii; + + /* allocate array to hold all the portgroups */ + if (VIR_ALLOC_N(def->portGroups, nPortGroups) < 0) { + virReportOOMError(); + goto error; + } + /* parse each portgroup */ + for (ii = 0; ii < nPortGroups; ii++) { + int ret = virNetworkPortGroupParseXML(def->name, + &def->portGroups[ii], + portGroupNodes[ii], ctxt); + if (ret < 0) + goto error; + def->nPortGroups++; + } + } + VIR_FREE(portGroupNodes); + nIps = virXPathNodeSet("./ip", ctxt, &ipNodes); if (nIps < 0) goto error; @@ -828,17 +940,16 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) } VIR_FREE(ipNodes); - /* IPv4 forwarding setup */ - if (virXPathBoolean("count(./forward) > 0", ctxt)) { - if (def->nips == 0) { - virNetworkReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Forwarding requested, but no IP address provided")); - goto error; - } - tmp = virXPathString("string(./forward[1]/@mode)", ctxt); + forwardNode = virXPathNode("./forward", ctxt); + if (!forwardNode) { + def->forwardType = VIR_NETWORK_FORWARD_NONE; + def->stp = (stp && STREQ(stp, "off")) ? 0 : 1; + } else { + ctxt->node = forwardNode; + tmp = virXPathString("string(./@mode)", ctxt); if (tmp) { if ((def->forwardType = virNetworkForwardTypeFromString(tmp)) < 0) { - virNetworkReportError(VIR_ERR_INTERNAL_ERROR, + virNetworkReportError(VIR_ERR_XML_ERROR, _("unknown forwarding type '%s'"), tmp); VIR_FREE(tmp); goto error; @@ -848,17 +959,85 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) def->forwardType = VIR_NETWORK_FORWARD_NAT; } + def->forwardDev = virXPathString("string(./@dev)", ctxt); - def->forwardDev = virXPathString("string(./forward[1]/@dev)", ctxt); - } else { - def->forwardType = VIR_NETWORK_FORWARD_NONE; - } + switch (def->forwardType) { + case VIR_NETWORK_FORWARD_ROUTE: + case VIR_NETWORK_FORWARD_NAT: + /* It's pointless to specify L3 forwarding without specifying + * the network we're on. + */ + if (def->nips == 0) { + virNetworkReportError(VIR_ERR_XML_ERROR, + _("%s forwarding requested, but no IP address provided for network '%s'"), + virNetworkForwardTypeToString(def->forwardType), + def->name); + goto error; + } + def->stp = (stp && STREQ(stp, "off")) ? 0 : 1; + break; + case VIR_NETWORK_FORWARD_PRIVATE: + case VIR_NETWORK_FORWARD_VEPA: + case VIR_NETWORK_FORWARD_PASSTHROUGH: + if (def->bridge) { + virNetworkReportError(VIR_ERR_XML_ERROR, + _("bridge name not allowed in %s mode (network '%s'"), + virNetworkForwardTypeToString(def->forwardType), + def->name); + goto error; + } + /* Fall through to next case */ + case VIR_NETWORK_FORWARD_BRIDGE: + if (def->delay || stp) { + virNetworkReportError(VIR_ERR_XML_ERROR, + _("bridge delay/stp options only allowed in route, nat, and isolated mode, not in %s (network '%s')"), + virNetworkForwardTypeToString(def->forwardType), + def->name); + goto error; + } + /* all of these modes can use a pool of physical interfaces */ + nForwardIfs = virXPathNodeSet("./interface", ctxt, &forwardIfNodes); + if (nForwardIfs < 0) + goto error; + + if (nForwardIfs > 0) { + int ii; + /* allocate array to hold all the portgroups */ + if (VIR_ALLOC_N(def->forwardIfs, nForwardIfs) < 0) { + virReportOOMError(); + goto error; + } + + /* parse each forwardIf */ + for (ii = 0; ii < nForwardIfs; ii++) { + def->forwardIfs[ii].usageCount = 0; + def->forwardIfs[ii].dev = virXMLPropString(forwardIfNodes[ii], + "dev"); + if (!def->forwardIfs[ii].dev) { + virNetworkReportError(VIR_ERR_XML_ERROR, + _("Missing required dev attribute in network '%s' forward interface"), + def->name); + goto error; + } + def->nForwardIfs++; + } + } + VIR_FREE(forwardIfNodes); + break; + } + } + VIR_FREE(stp); + ctxt->node = save; return def; error: + VIR_FREE(stp); virNetworkDefFree(def); VIR_FREE(ipNodes); + VIR_FREE(portGroupNodes); + VIR_FREE(forwardIfNodes); + ctxt->node = save; return NULL; } @@ -1043,6 +1222,19 @@ error: return result; } +static void +virPortGroupDefFormat(virBufferPtr buf, + const virPortGroupDefPtr def) +{ + virBufferAsprintf(buf, " <portgroup name='%s'", def->name); + if (def->isDefault) { + virBufferAddLit(buf, " default='yes'"); + } + virBufferAddLit(buf, ">\n"); + virVirtualPortProfileFormat(buf, def->virtPortProfile, " "); + virBufferAddLit(buf, " </portgroup>\n"); +} + char *virNetworkDefFormat(const virNetworkDefPtr def) { virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -1058,24 +1250,55 @@ char *virNetworkDefFormat(const virNetworkDefPtr def) virBufferAsprintf(&buf, " <uuid>%s</uuid>\n", uuidstr); if (def->forwardType != VIR_NETWORK_FORWARD_NONE) { + char *dev = def->forwardDev; const char *mode = virNetworkForwardTypeToString(def->forwardType); - if (mode) { - if (def->forwardDev) { - virBufferEscapeString(&buf, " <forward dev='%s'", - def->forwardDev); - } else { - virBufferAddLit(&buf, " <forward"); + if (!mode) { + virNetworkReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown forward type %d in network '%s'"), + def->forwardType, def->name); + goto error; + } + virBufferAddLit(&buf, " <forward"); + + /* Duplicate the first interface from the pool into <forward + * dev=xxx for convenience. + */ + if (!dev && def->nForwardIfs && + def->forwardIfs && def->forwardIfs[0].dev) { + dev = def->forwardIfs[0].dev; + } + if (dev) + virBufferEscapeString(&buf, " dev='%s'", dev); + virBufferAsprintf(&buf, " mode='%s'%s>\n", mode, + def->nForwardIfs ? "" : "/"); + + if (def->nForwardIfs) { + for (ii = 0; ii < def->nForwardIfs; ii++) { + if (def->forwardIfs[ii].dev) { + virBufferEscapeString(&buf, " <interface dev='%s'/>\n", + def->forwardIfs[ii].dev); + } } - virBufferAsprintf(&buf, " mode='%s'/>\n", mode); + virBufferAddLit(&buf, " </forward>\n"); } } - virBufferAddLit(&buf, " <bridge"); - if (def->bridge) - virBufferEscapeString(&buf, " name='%s'", def->bridge); - virBufferAsprintf(&buf, " stp='%s' delay='%ld' />\n", - def->stp ? "on" : "off", - def->delay); + if (def->forwardType == VIR_NETWORK_FORWARD_NONE || + def->forwardType == VIR_NETWORK_FORWARD_NAT || + def->forwardType == VIR_NETWORK_FORWARD_ROUTE) { + + virBufferAddLit(&buf, " <bridge"); + if (def->bridge) + virBufferEscapeString(&buf, " name='%s'", def->bridge); + virBufferAsprintf(&buf, " stp='%s' delay='%ld' />\n", + def->stp ? "on" : "off", + def->delay); + } else if (def->forwardType == VIR_NETWORK_FORWARD_BRIDGE && + def->bridge) { + virBufferEscapeString(&buf, " <bridge name='%s' />\n", def->bridge); + } + + if (def->mac_specified) { char macaddr[VIR_MAC_STRING_BUFLEN]; virFormatMacAddr(def->mac, macaddr); @@ -1093,6 +1316,11 @@ char *virNetworkDefFormat(const virNetworkDefPtr def) goto error; } + virVirtualPortProfileFormat(&buf, def->virtPortProfile, " "); + + for (ii = 0; ii < def->nPortGroups; ii++) + virPortGroupDefFormat(&buf, &def->portGroups[ii]); + virBufferAddLit(&buf, "</network>\n"); if (virBufferError(&buf)) @@ -1107,6 +1335,22 @@ char *virNetworkDefFormat(const virNetworkDefPtr def) return NULL; } +virPortGroupDefPtr virPortGroupFindByName(virNetworkDefPtr net, + const char *portgroup) +{ + int ii; + for (ii = 0; ii < net->nPortGroups; ii++) { + if (portgroup) { + if (STREQ(portgroup, net->portGroups[ii].name)) + return &net->portGroups[ii]; + } else { + if (net->portGroups[ii].isDefault) + return &net->portGroups[ii]; + } + } + return NULL; +} + int virNetworkSaveXML(const char *configDir, virNetworkDefPtr def, const char *xml) @@ -1210,11 +1454,16 @@ virNetworkObjPtr virNetworkLoadConfig(virNetworkObjListPtr nets, goto error; } - /* Generate a bridge if none is specified, but don't check for collisions - * if a bridge is hardcoded, so the network is at least defined - */ - if (virNetworkSetBridgeName(nets, def, 0)) - goto error; + if (def->forwardType == VIR_NETWORK_FORWARD_NONE || + def->forwardType == VIR_NETWORK_FORWARD_NAT || + def->forwardType == VIR_NETWORK_FORWARD_ROUTE) { + + /* Generate a bridge if none is specified, but don't check for collisions + * if a bridge is hardcoded, so the network is at least defined. + */ + if (virNetworkSetBridgeName(nets, def, 0)) + goto error; + } if (!(net = virNetworkAssignDef(nets, def))) goto error; diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index d7d2951..5df6724 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -33,11 +33,14 @@ # include "network.h" # include "util.h" -/* 2 possible types of forwarding */ enum virNetworkForwardType { VIR_NETWORK_FORWARD_NONE = 0, VIR_NETWORK_FORWARD_NAT, VIR_NETWORK_FORWARD_ROUTE, + VIR_NETWORK_FORWARD_BRIDGE, + VIR_NETWORK_FORWARD_PRIVATE, + VIR_NETWORK_FORWARD_VEPA, + VIR_NETWORK_FORWARD_PASSTHROUGH, VIR_NETWORK_FORWARD_LAST, }; @@ -107,6 +110,21 @@ struct _virNetworkIpDef { virSocketAddr bootserver; }; +typedef struct _virNetworkForwardIfDef virNetworkForwardIfDef; +typedef virNetworkForwardIfDef *virNetworkForwardIfDefPtr; +struct _virNetworkForwardIfDef { + char *dev; /* name of device */ + int usageCount; /* how many guest interfaces are bound to this device? */ +}; + +typedef struct _virPortGroupDef virPortGroupDef; +typedef virPortGroupDef *virPortGroupDefPtr; +struct _virPortGroupDef { + char *name; + bool isDefault; + virVirtualPortProfileParamsPtr virtPortProfile; +}; + typedef struct _virNetworkDef virNetworkDef; typedef virNetworkDef *virNetworkDefPtr; struct _virNetworkDef { @@ -121,12 +139,22 @@ struct _virNetworkDef { bool mac_specified; int forwardType; /* One of virNetworkForwardType constants */ - char *forwardDev; /* Destination device for forwarding */ + char *forwardDev; /* Destination device for forwarding (if just one) */ + + /* If there are multiple forward devices (i.e. a pool of + * interfaces), they will be listed here. + */ + size_t nForwardIfs; + virNetworkForwardIfDefPtr forwardIfs; size_t nips; virNetworkIpDefPtr ips; /* ptr to array of IP addresses on this network */ virNetworkDNSDefPtr dns; /* ptr to dns related configuration */ + virVirtualPortProfileParamsPtr virtPortProfile; + + size_t nPortGroups; + virPortGroupDefPtr portGroups; }; typedef struct _virNetworkObj virNetworkObj; @@ -151,6 +179,8 @@ struct _virNetworkObjList { virNetworkObjPtr *objs; }; +virPortGroupDefPtr virPortGroupFindByName(virNetworkDefPtr net, + const char *portgroup); static inline int virNetworkObjIsActive(const virNetworkObjPtr net) { diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a7540d2..3b8214a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -205,6 +205,7 @@ dnsmasqSave; # domain_conf.h virDiskNameToBusDeviceIndex; virDiskNameToIndex; +virDomainActualNetDefFree; virDomainAssignDef; virDomainChrConsoleTargetTypeFromString; virDomainChrConsoleTargetTypeToString; @@ -309,6 +310,11 @@ virDomainLoadAllConfigs; virDomainMemballoonModelTypeFromString; virDomainMemballoonModelTypeToString; virDomainNetDefFree; +virDomainNetGetActualBridgeName; +virDomainNetGetActualDirectDev; +virDomainNetGetActualDirectMode; +virDomainNetGetActualType; +virDomainNetGetActualDirectVirtPortProfile; virDomainNetIndexByMac; virDomainNetInsert; virDomainNetRemoveByMac; @@ -722,7 +728,7 @@ virNetworkRemoveInactive; virNetworkSaveConfig; virNetworkSetBridgeMacAddr; virNetworkSetBridgeName; - +virPortGroupFindByName; # node_device_conf.h virNodeDevCapTypeToString; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 660dd65..cb49356 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2169,10 +2169,18 @@ static virNetworkPtr networkCreate(virConnectPtr conn, const char *xml) { if (virNetworkObjIsDuplicate(&driver->networks, def, 1) < 0) goto cleanup; - if (virNetworkSetBridgeName(&driver->networks, def, 1)) - goto cleanup; + /* Only the three L3 network types that are configured by libvirt + * need to have a bridge device name / mac address provided + */ + if (def->forwardType == VIR_NETWORK_FORWARD_NONE || + def->forwardType == VIR_NETWORK_FORWARD_NAT || + def->forwardType == VIR_NETWORK_FORWARD_ROUTE) { - virNetworkSetBridgeMacAddr(def); + if (virNetworkSetBridgeName(&driver->networks, def, 1)) + goto cleanup; + + virNetworkSetBridgeMacAddr(def); + } if (!(network = virNetworkAssignDef(&driver->networks, def))) @@ -2214,10 +2222,18 @@ static virNetworkPtr networkDefine(virConnectPtr conn, const char *xml) { if (virNetworkObjIsDuplicate(&driver->networks, def, 0) < 0) goto cleanup; - if (virNetworkSetBridgeName(&driver->networks, def, 1)) - goto cleanup; + /* Only the three L3 network types that are configured by libvirt + * need to have a bridge device name / mac address provided + */ + if (def->forwardType == VIR_NETWORK_FORWARD_NONE || + def->forwardType == VIR_NETWORK_FORWARD_NAT || + def->forwardType == VIR_NETWORK_FORWARD_ROUTE) { - virNetworkSetBridgeMacAddr(def); + if (virNetworkSetBridgeName(&driver->networks, def, 1)) + goto cleanup; + + virNetworkSetBridgeMacAddr(def); + } if (!(network = virNetworkAssignDef(&driver->networks, def))) -- 1.7.3.4

On Tue, Jul 05, 2011 at 03:45:55AM -0400, Laine Stump wrote:
This implements the changes to <network> and domain <interface> XML that were earlier specified in the RNG.
Each virDomainNetDef now also potentially has a virDomainActualNetDef which is a private object (never exported/imported via the public API, and not defined in the RNG) that is used to maintain information about the physical device that was actually used for a NetDef that was of type VIR_DOMAIN_NET_TYPE_NETWORK.
The virDomainActualNetDef will only be parsed/formatted if the parse/format function is called with the VIR_DOMAIN_XML_ACTUAL_NET flags set (which is only needed when saving/loading a running domain's state info to the stateDir). To prevent this flag bit from accidentally being used in the public API, a "RESERVED" placeholder was put into the public flags enum (at the same time, I noticed there was another private flag that hadn't been reserved, so I added that one, making both of these flags #defined from the public RESERVED flags, and since it was also only used in domain_conf.c, I unpolluted domain_conf.h, putting both #defines in domain_conf.c.
A small change is also made to two functions in bridge_driver.c, to prevent a bridge device name and mac address from being automatically added to a network definition when it is of one of the new forward types (none of which use bridge devices managed by libvirt). --- include/libvirt/libvirt.h.in | 2 + src/conf/domain_conf.c | 276 +++++++++++++++++++++++++++++++++++- src/conf/domain_conf.h | 46 ++++++- src/conf/network_conf.c | 321 +++++++++++++++++++++++++++++++++++++----- src/conf/network_conf.h | 34 ++++- src/libvirt_private.syms | 8 +- src/network/bridge_driver.c | 28 +++- 7 files changed, 657 insertions(+), 58 deletions(-)
I think it would be worth doing a little change in patch split for this and the previous patch. Instead of splitting between schema and impl, split between domain & network. So I think we should have one patch which pulls the common domain.rng conf schema pieces out, and also modifies domain_conf.h/c at the same time. Then a second patch, which pulls the common vport bits into network.rng and also modifies network_conf.h/.c Also, each of those patches ought to have at least one new data file added to their corresponding XML test case at the same time, so that each patch contains the full self-contained modifications.
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 8e20f75..b88c96e 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -1112,6 +1112,8 @@ typedef enum { VIR_DOMAIN_XML_SECURE = (1 << 0), /* dump security sensitive information too */ VIR_DOMAIN_XML_INACTIVE = (1 << 1), /* dump inactive domain information */ VIR_DOMAIN_XML_UPDATE_CPU = (1 << 2), /* update guest CPU requirements according to host CPU */ + VIR_DOMAIN_XML_RESERVED1 = (1 << 30), /* reserved for internal used */ + VIR_DOMAIN_XML_RESERVED2 = (1 << 31), /* reserved for internal used */ } virDomainXMLFlags;
[snip]
+/* these flags are only used internally */ +/* dump internal domain status information */ +#define VIR_DOMAIN_XML_INTERNAL_STATUS VIR_DOMAIN_XML_RESERVED1 +/* dump virDomainActualNetDef contents */ +#define VIR_DOMAIN_XML_ACTUAL_NET VIR_DOMAIN_XML_RESERVED2
Ewww, I really don't like this idea. If we need to pass special internal only flags to the parser/formating methods, then I think that should be done as a separate parameter from the public flags parameter. Either a 'unsigned int internalFlags' or one or more 'bool someOption' parameters.
static void virDomainObjListDataFree(void *payload, const void *name ATTRIBUTE_UNUSED) @@ -714,6 +719,25 @@ void virDomainFSDefFree(virDomainFSDefPtr def) VIR_FREE(def); }
+void +virDomainActualNetDefFree(virDomainActualNetDefPtr def) +{ + if (!def) + return; + + switch (def->type) { + case VIR_DOMAIN_NET_TYPE_BRIDGE: + VIR_FREE(def->data.bridge.brname); + break; + case VIR_DOMAIN_NET_TYPE_DIRECT: + VIR_FREE(def->data.direct.linkdev); + VIR_FREE(def->data.direct.virtPortProfile); + break; + default: + break; + } +} + void virDomainNetDefFree(virDomainNetDefPtr def) { if (!def) @@ -736,6 +760,9 @@ void virDomainNetDefFree(virDomainNetDefPtr def)
case VIR_DOMAIN_NET_TYPE_NETWORK: VIR_FREE(def->data.network.name); + VIR_FREE(def->data.network.portgroup); + VIR_FREE(def->data.network.virtPortProfile); + virDomainActualNetDefFree(def->data.network.actual); break;
case VIR_DOMAIN_NET_TYPE_BRIDGE: @@ -2566,6 +2593,85 @@ cleanup: goto cleanup; }
+static int +virDomainActualNetDefParseXML(xmlNodePtr node, + xmlXPathContextPtr ctxt, + virDomainActualNetDefPtr *actual) +{ + int ret = -1; + xmlNodePtr save_ctxt = ctxt->node; + char *type = NULL; + char *mode = NULL; + + if (VIR_ALLOC(*actual) < 0) { + virReportOOMError(); + return -1; + } + + ctxt->node = node; + + type = virXMLPropString(node, "type"); + if (!type) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing type attribute in interface's <actual> element")); + goto error; + } + if ((int)((*actual)->type = virDomainNetTypeFromString(type)) < 0) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown type '%s' in interface's <actual> element"), type); + goto error; + } + if ((*actual)->type != VIR_DOMAIN_NET_TYPE_BRIDGE && + (*actual)->type != VIR_DOMAIN_NET_TYPE_DIRECT) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + _("unsupported type '%s' in interface's <actual> element"), + type); + goto error; + } + + if ((*actual)->type == VIR_DOMAIN_NET_TYPE_BRIDGE) { + + (*actual)->data.bridge.brname = virXPathString("string(./source[1]/@bridge)", + ctxt); + + } else if ((*actual)->type == VIR_DOMAIN_NET_TYPE_DIRECT) { + xmlNodePtr virtPortNode; + const char *errmsg; + + (*actual)->data.direct.linkdev = virXPathString("string(./source[1]/@dev)", ctxt); + + mode = virXPathString("string(./source[1]/@mode)", ctxt); + if (mode) { + int m; + if ((m = virMacvtapModeTypeFromString(mode)) < 0) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + _("Unkown mode '%s' in interface <actual> element"), + mode); + goto error; + } + (*actual)->data.direct.mode = m; + } + + virtPortNode = virXPathNode("./virtualport", ctxt); + if (virtPortNode && + virVirtualPortProfileParamsParseXML(virtPortNode, + &(*actual)->data.direct.virtPortProfile, + &errmsg) < 0) { + if (errmsg) + virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s in interface <actual>", + errmsg); + goto error; + } + } + + ret = 0; +error: + VIR_FREE(type); + VIR_FREE(mode); + + ctxt->node = save_ctxt; + return ret; +}
/* Parse the XML definition for a network interface * @param node XML nodeset to parse for net definition @@ -2583,6 +2689,7 @@ virDomainNetDefParseXML(virCapsPtr caps, char *macaddr = NULL; char *type = NULL; char *network = NULL; + char *portgroup = NULL; char *bridge = NULL; char *dev = NULL; char *ifname = NULL; @@ -2599,6 +2706,7 @@ virDomainNetDefParseXML(virCapsPtr caps, char *mode = NULL; virNWFilterHashTablePtr filterparams = NULL; virVirtualPortProfileParamsPtr virtPort = NULL; + virDomainActualNetDefPtr actual = NULL; xmlNodePtr oldnode = ctxt->node; int ret;
@@ -2630,6 +2738,7 @@ virDomainNetDefParseXML(virCapsPtr caps, (def->type == VIR_DOMAIN_NET_TYPE_NETWORK) && (xmlStrEqual(cur->name, BAD_CAST "source"))) { network = virXMLPropString(cur, "network"); + portgroup = virXMLPropString(cur, "portgroup"); } else if ((internal == NULL) && (def->type == VIR_DOMAIN_NET_TYPE_INTERNAL) && (xmlStrEqual(cur->name, BAD_CAST "source"))) { @@ -2645,7 +2754,8 @@ virDomainNetDefParseXML(virCapsPtr caps, dev = virXMLPropString(cur, "dev"); mode = virXMLPropString(cur, "mode"); } else if ((virtPort == NULL) && - (def->type == VIR_DOMAIN_NET_TYPE_DIRECT) && + ((def->type == VIR_DOMAIN_NET_TYPE_DIRECT) || + (def->type == VIR_DOMAIN_NET_TYPE_NETWORK)) && xmlStrEqual(cur->name, BAD_CAST "virtualport")) { const char *errmsg; if (virVirtualPortProfileParamsParseXML(cur, &virtPort, &errmsg) < 0) { @@ -2697,6 +2807,12 @@ virDomainNetDefParseXML(virCapsPtr caps, if (virDomainDeviceBootParseXML(cur, &def->bootIndex, bootMap)) goto error; + } else if ((actual == NULL) && + (flags & VIR_DOMAIN_XML_ACTUAL_NET) && + (def->type == VIR_DOMAIN_NET_TYPE_NETWORK) && + xmlStrEqual(cur->name, BAD_CAST "actual")) { + if (virDomainActualNetDefParseXML(cur, ctxt, &actual) < 0) + goto error; } } cur = cur->next; @@ -2745,6 +2861,12 @@ virDomainNetDefParseXML(virCapsPtr caps, } def->data.network.name = network; network = NULL; + def->data.network.portgroup = portgroup; + portgroup = NULL; + def->data.network.virtPortProfile = virtPort; + virtPort = NULL; + def->data.network.actual = actual; + actual = NULL; break;
case VIR_DOMAIN_NET_TYPE_ETHERNET: @@ -2836,10 +2958,8 @@ virDomainNetDefParseXML(virCapsPtr caps, } else def->data.direct.mode = VIR_MACVTAP_MODE_VEPA;
- if (virtPort) { - def->data.direct.virtPortProfile = virtPort; - virtPort = NULL; - } + def->data.direct.virtPortProfile = virtPort; + virtPort = NULL;
def->data.direct.linkdev = dev; dev = NULL; @@ -2943,11 +3063,13 @@ cleanup: ctxt->node = oldnode; VIR_FREE(macaddr); VIR_FREE(network); + VIR_FREE(portgroup); VIR_FREE(address); VIR_FREE(port); VIR_FREE(ifname); VIR_FREE(dev); VIR_FREE(virtPort); + virDomainActualNetDefFree(actual); VIR_FREE(script); VIR_FREE(bridge); VIR_FREE(model); @@ -8421,6 +8543,67 @@ virDomainFSDefFormat(virBufferPtr buf, }
static int +virDomainActualNetDefFormat(virBufferPtr buf, + virDomainActualNetDefPtr def, + int flags) { + int ret = -1; + const char *type; + const char *mode; + + if (!(def && (flags & VIR_DOMAIN_XML_INTERNAL_STATUS))) + return 0; + + type = virDomainNetTypeToString(def->type); + if (!type) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected net type %d"), def->type); + return ret; + } + + if (def->type != VIR_DOMAIN_NET_TYPE_BRIDGE && + def->type != VIR_DOMAIN_NET_TYPE_DIRECT) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected net type %s"), type); + goto error; + } + virBufferAsprintf(buf, " <actual type='%s'>\n", type); + + switch (def->type) { + case VIR_DOMAIN_NET_TYPE_BRIDGE: + if (def->data.bridge.brname) { + virBufferEscapeString(buf, " <source bridge='%s'/>\n", + def->data.bridge.brname); + } + break; + + case VIR_DOMAIN_NET_TYPE_DIRECT: + virBufferAddLit(buf, " <source"); + if (def->data.direct.linkdev) + virBufferEscapeString(buf, " dev='%s'", + def->data.direct.linkdev); + + mode = virMacvtapModeTypeToString(def->data.direct.mode); + if (!mode) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected source mode %d"), + def->data.direct.mode); + return ret; + } + virBufferAsprintf(buf, " mode='%s'/>\n", mode); + virVirtualPortProfileFormat(buf, def->data.direct.virtPortProfile, + " "); + break; + default: + break; + } + virBufferAddLit(buf, " </actual>\n"); + + ret = 0; +error: + return ret; +} + +static int virDomainNetDefFormat(virBufferPtr buf, virDomainNetDefPtr def, int flags) @@ -8443,8 +8626,17 @@ virDomainNetDefFormat(virBufferPtr buf,
switch (def->type) { case VIR_DOMAIN_NET_TYPE_NETWORK: - virBufferEscapeString(buf, " <source network='%s'/>\n", + virBufferEscapeString(buf, " <source network='%s'", def->data.network.name); + if (def->data.network.portgroup) { + virBufferEscapeString(buf, " portgroup='%s'", + def->data.network.portgroup); + } + virBufferAddLit(buf, "/>\n"); + virVirtualPortProfileFormat(buf, def->data.network.virtPortProfile, + " "); + if (virDomainActualNetDefFormat(buf, def->data.network.actual, flags) < 0) + return -1; break;
This makes the XML formatting a little more verbose than we normally aim for, in the common case where no portgrp/profile exists. eg we get an empty <source network='defualt'> </source>
+ virtPortNode = virXPathNode("./virtualport", ctxt); + if (virtPortNode) { + const char *errmsg; + if (virVirtualPortProfileParamsParseXML(virtPortNode, + &def->virtPortProfile, + &errmsg) < 0) { + if (errmsg) + virNetworkReportError(VIR_ERR_XML_ERROR, "%s", errmsg); + goto error; + } + }
Any reason why we don't just make virVirtualPortProfileParamsParseXML responsible for raising the error? Passing back the error message as a string is rather unusual for our code.
+ if (nPortGroups > 0) { + int ii;
Is the more common name 'i' not available ?
+ + /* allocate array to hold all the portgroups */ + if (VIR_ALLOC_N(def->portGroups, nPortGroups) < 0) { + virReportOOMError(); + goto error; + } + /* parse each portgroup */ + for (ii = 0; ii < nPortGroups; ii++) { + int ret = virNetworkPortGroupParseXML(def->name, + &def->portGroups[ii], + portGroupNodes[ii], ctxt); + if (ret < 0) + goto error; + def->nPortGroups++; + } + } + VIR_FREE(portGroupNodes); +
+ def->forwardDev = virXPathString("string(./@dev)", ctxt);
- def->forwardDev = virXPathString("string(./forward[1]/@dev)", ctxt); - } else { - def->forwardType = VIR_NETWORK_FORWARD_NONE; - } + switch (def->forwardType) { + case VIR_NETWORK_FORWARD_ROUTE: + case VIR_NETWORK_FORWARD_NAT: + /* It's pointless to specify L3 forwarding without specifying + * the network we're on. + */ + if (def->nips == 0) { + virNetworkReportError(VIR_ERR_XML_ERROR, + _("%s forwarding requested, but no IP address provided for network '%s'"), + virNetworkForwardTypeToString(def->forwardType), + def->name); + goto error; + } + def->stp = (stp && STREQ(stp, "off")) ? 0 : 1; + break; + case VIR_NETWORK_FORWARD_PRIVATE: + case VIR_NETWORK_FORWARD_VEPA: + case VIR_NETWORK_FORWARD_PASSTHROUGH: + if (def->bridge) { + virNetworkReportError(VIR_ERR_XML_ERROR, + _("bridge name not allowed in %s mode (network '%s'"), + virNetworkForwardTypeToString(def->forwardType), + def->name); + goto error; + } + /* Fall through to next case */ + case VIR_NETWORK_FORWARD_BRIDGE: + if (def->delay || stp) { + virNetworkReportError(VIR_ERR_XML_ERROR, + _("bridge delay/stp options only allowed in route, nat, and isolated mode, not in %s (network '%s')"), + virNetworkForwardTypeToString(def->forwardType), + def->name); + goto error; + } + /* all of these modes can use a pool of physical interfaces */ + nForwardIfs = virXPathNodeSet("./interface", ctxt, &forwardIfNodes); + if (nForwardIfs < 0) + goto error; + + if (nForwardIfs > 0) { + int ii;
+ /* allocate array to hold all the portgroups */ + if (VIR_ALLOC_N(def->forwardIfs, nForwardIfs) < 0) { + virReportOOMError(); + goto error; + } + + /* parse each forwardIf */ + for (ii = 0; ii < nForwardIfs; ii++) { + def->forwardIfs[ii].usageCount = 0; + def->forwardIfs[ii].dev = virXMLPropString(forwardIfNodes[ii], + "dev"); + if (!def->forwardIfs[ii].dev) { + virNetworkReportError(VIR_ERR_XML_ERROR, + _("Missing required dev attribute in network '%s' forward interface"), + def->name); + goto error; + } + def->nForwardIfs++; + } + } + VIR_FREE(forwardIfNodes); + break; + } + }
The handling of the interface device names does not seem right to me. The following should be identical: <forward mode='nat' dev='eth0'/> <forward mode='nat'> <interface dev='eth0'/> </forward> <forward mode='nat' dev='eth0'> <interface dev='eth0'/> </forward> And so should <forward mode='vepa' dev='eth0'/> <forward mode='vepa'> <interface dev='eth0'/> </forward> <forward mode='vepa' dev='eth0'> <interface dev='eth0'/> </forward> <forward mode='vepa' dev='eth0'> <interface dev='eth0'/> <interface dev='eth1'/> </forward> The following should be illegal <forward mode='vepa' dev='eth0'> <interface dev='eth2'/> </forward> <forward mode='vepa' dev='eth0'> <interface dev='eth2'/> <interface dev='eth0'/> </forward> ie, @dev must be identical to /interface[0]/@dev if present, and both syntaxes should be allowed regardless of the 'mode' attribute value.
typedef struct _virNetworkDef virNetworkDef; typedef virNetworkDef *virNetworkDefPtr; struct _virNetworkDef { @@ -121,12 +139,22 @@ struct _virNetworkDef { bool mac_specified;
int forwardType; /* One of virNetworkForwardType constants */ - char *forwardDev; /* Destination device for forwarding */ + char *forwardDev; /* Destination device for forwarding (if just one) */ + + /* If there are multiple forward devices (i.e. a pool of + * interfaces), they will be listed here. + */ + size_t nForwardIfs; + virNetworkForwardIfDefPtr forwardIfs;
Keeping 'forwardDev' is wrong here. We should only end up with the array of interfaces, and forwardDev should be folded into that at parse time, and pulled back out at format time. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 07/08/2011 09:17 AM, Daniel P. Berrange wrote:
On Tue, Jul 05, 2011 at 03:45:55AM -0400, Laine Stump wrote:
This implements the changes to<network> and domain<interface> XML that were earlier specified in the RNG.
Each virDomainNetDef now also potentially has a virDomainActualNetDef which is a private object (never exported/imported via the public API, and not defined in the RNG) that is used to maintain information about the physical device that was actually used for a NetDef that was of type VIR_DOMAIN_NET_TYPE_NETWORK.
The virDomainActualNetDef will only be parsed/formatted if the parse/format function is called with the VIR_DOMAIN_XML_ACTUAL_NET flags set (which is only needed when saving/loading a running domain's state info to the stateDir). To prevent this flag bit from accidentally being used in the public API, a "RESERVED" placeholder was put into the public flags enum (at the same time, I noticed there was another private flag that hadn't been reserved, so I added that one, making both of these flags #defined from the public RESERVED flags, and since it was also only used in domain_conf.c, I unpolluted domain_conf.h, putting both #defines in domain_conf.c.
A small change is also made to two functions in bridge_driver.c, to prevent a bridge device name and mac address from being automatically added to a network definition when it is of one of the new forward types (none of which use bridge devices managed by libvirt). --- include/libvirt/libvirt.h.in | 2 + src/conf/domain_conf.c | 276 +++++++++++++++++++++++++++++++++++- src/conf/domain_conf.h | 46 ++++++- src/conf/network_conf.c | 321 +++++++++++++++++++++++++++++++++++++----- src/conf/network_conf.h | 34 ++++- src/libvirt_private.syms | 8 +- src/network/bridge_driver.c | 28 +++- 7 files changed, 657 insertions(+), 58 deletions(-)
I think it would be worth doing a little change in patch split for this and the previous patch. Instead of splitting between schema and impl, split between domain& network.
So I think we should have one patch which pulls the common domain.rng conf schema pieces out, and also modifies domain_conf.h/c at the same time.
Then a second patch, which pulls the common vport bits into network.rng and also modifies network_conf.h/.c
Also, each of those patches ought to have at least one new data file added to their corresponding XML test case at the same time, so that each patch contains the full self-contained modifications.
Okay, I'm redid it that way. (Actually, I made two separate "pre-patches", one that makes the virtportprofile stuff common, and another that changes the virtportprofile in the domain from being contained in the object to being pointed to by the object. *Then* I made a patch to add the new stuff to domain xml from top to bottom, and finally one that adds the new stuff to the network xml.)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 8e20f75..b88c96e 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -1112,6 +1112,8 @@ typedef enum { VIR_DOMAIN_XML_SECURE = (1<< 0), /* dump security sensitive information too */ VIR_DOMAIN_XML_INACTIVE = (1<< 1), /* dump inactive domain information */ VIR_DOMAIN_XML_UPDATE_CPU = (1<< 2), /* update guest CPU requirements according to host CPU */ + VIR_DOMAIN_XML_RESERVED1 = (1<< 30), /* reserved for internal used */ + VIR_DOMAIN_XML_RESERVED2 = (1<< 31), /* reserved for internal used */ } virDomainXMLFlags; [snip]
+/* these flags are only used internally */ +/* dump internal domain status information */ +#define VIR_DOMAIN_XML_INTERNAL_STATUS VIR_DOMAIN_XML_RESERVED1 +/* dump virDomainActualNetDef contents */ +#define VIR_DOMAIN_XML_ACTUAL_NET VIR_DOMAIN_XML_RESERVED2 Ewww, I really don't like this idea. If we need to pass special internal only flags to the parser/formating methods, then I think that should be done as a separate parameter from the public flags parameter. Either a 'unsigned int internalFlags' or one or more 'bool someOption' parameters.
After a lot of back and forth on this, what was decided on was to leave VIR_DOMAIN_XML_INTERNAL_STATUS in the regular flags (and also put VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET there), but put in a compile-time protection against re-using those bits for the public API, and a runtime check to make sure nobody calls the public API with those bits on. Eric took care of this in a separate patch, which was pushed yesterday.
+static int virDomainNetDefFormat(virBufferPtr buf, virDomainNetDefPtr def, int flags) @@ -8443,8 +8626,17 @@ virDomainNetDefFormat(virBufferPtr buf,
switch (def->type) { case VIR_DOMAIN_NET_TYPE_NETWORK: - virBufferEscapeString(buf, "<source network='%s'/>\n", + virBufferEscapeString(buf, "<source network='%s'", def->data.network.name); + if (def->data.network.portgroup) { + virBufferEscapeString(buf, " portgroup='%s'", + def->data.network.portgroup); + } + virBufferAddLit(buf, "/>\n"); + virVirtualPortProfileFormat(buf, def->data.network.virtPortProfile, + " "); + if (virDomainActualNetDefFormat(buf, def->data.network.actual, flags)< 0) + return -1; break; This makes the XML formatting a little more verbose than we normally aim for, in the common case where no portgrp/profile exists. eg we get an empty
<source network='defualt'> </source>
I think you've misread the code. The portgroup is part of <source>, but it's an attribute and is on the same line. the virtportprofile is a separate element, and is at the top level, not within <source>. So <source network='....." will always end on the same line, just as it did in the past. As proof, I can say that "make check" passes :-)
+ virtPortNode = virXPathNode("./virtualport", ctxt); + if (virtPortNode) { + const char *errmsg; + if (virVirtualPortProfileParamsParseXML(virtPortNode, +&def->virtPortProfile, +&errmsg)< 0) { + if (errmsg) + virNetworkReportError(VIR_ERR_XML_ERROR, "%s", errmsg); + goto error; + } + } Any reason why we don't just make virVirtualPortProfileParamsParseXML responsible for raising the error? Passing back the error message as a string is rather unusual for our code.
I wanted to call virNetworkReportError() rather than virSocketError(), and give a bit more context about where this virtportprofile was located. Since you and Eric both disliked this, I changed it to report the error directly in virVirtualPortProfilePramsParseXML
+ if (nPortGroups> 0) { + int ii; Is the more common name 'i' not available ?
I prefer ii because it's easier to search for - if you search for "i" by itself, you'll get a lot of false hits, but ii hardly ever occurs naturally. (I've actually been using this name for array indexes for a long time; I'm surprised you hadn't noticed it in earlier patches).
The handling of the interface device names does not seem right to me. The following should be identical:
<forward mode='nat' dev='eth0'/> <forward mode='nat'> <interface dev='eth0'/> </forward> <forward mode='nat' dev='eth0'> <interface dev='eth0'/> </forward>
And so should
<forward mode='vepa' dev='eth0'/> <forward mode='vepa'> <interface dev='eth0'/> </forward> <forward mode='vepa' dev='eth0'> <interface dev='eth0'/> </forward> <forward mode='vepa' dev='eth0'> <interface dev='eth0'/> <interface dev='eth1'/> </forward>
The following should be illegal
<forward mode='vepa' dev='eth0'> <interface dev='eth2'/> </forward> <forward mode='vepa' dev='eth0'> <interface dev='eth2'/> <interface dev='eth0'/> </forward>
ie, @dev must be identical to /interface[0]/@dev if present, and both syntaxes should be allowed regardless of the 'mode' attribute value.
Okay. I had understood the first item, but was a bit lazy in my code. I hadn't figured on the second (since nat/route mode can only use a single interface). I've changed it to work as you suggest, and to remove forwardDev from the struct.
typedef struct _virNetworkDef virNetworkDef; typedef virNetworkDef *virNetworkDefPtr; struct _virNetworkDef { @@ -121,12 +139,22 @@ struct _virNetworkDef { bool mac_specified;
int forwardType; /* One of virNetworkForwardType constants */ - char *forwardDev; /* Destination device for forwarding */ + char *forwardDev; /* Destination device for forwarding (if just one) */ + + /* If there are multiple forward devices (i.e. a pool of + * interfaces), they will be listed here. + */ + size_t nForwardIfs; + virNetworkForwardIfDefPtr forwardIfs; Keeping 'forwardDev' is wrong here. We should only end up with the array of interfaces, and forwardDev should be folded into that at parse time, and pulled back out at format time.
Done.

On 07/05/2011 01:45 AM, Laine Stump wrote:
This implements the changes to <network> and domain <interface> XML that were earlier specified in the RNG.
+++ b/include/libvirt/libvirt.h.in @@ -1112,6 +1112,8 @@ typedef enum { VIR_DOMAIN_XML_SECURE = (1 << 0), /* dump security sensitive information too */ VIR_DOMAIN_XML_INACTIVE = (1 << 1), /* dump inactive domain information */ VIR_DOMAIN_XML_UPDATE_CPU = (1 << 2), /* update guest CPU requirements according to host CPU */ + VIR_DOMAIN_XML_RESERVED1 = (1 << 30), /* reserved for internal used */ + VIR_DOMAIN_XML_RESERVED2 = (1 << 31), /* reserved for internal used */
If we keep this, then s/used/use/ However, I agree with Dan that we don't want to list this in the public header. And even if you can convince me that we need to consume bits from the public flags, I don't see any code in either daemon/remote.c or src/libvirt.c that explicitly rejects any attempts to invoke an API with one of these reserved bits set (that is, if you do merge the internal flags onto a public field, then you had better make sure that no one externally can abuse those flags; whereas if you use a second internalFlags or bool arguments, you have isolated internal flags to be completely independent of the public API).
+ + type = virXMLPropString(node, "type"); + if (!type) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing type attribute in interface's <actual> element")); + goto error; + } + if ((int)((*actual)->type = virDomainNetTypeFromString(type)) < 0) {
Why is this cast needed? Oh, because struct _virDomainNetDef used 'enum virDomainNetType type;' instead of the more typical 'int type; /* enum virDomainNetType */'
+ virDomainReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown type '%s' in interface's <actual> element"), type); + goto error; + } + if ((*actual)->type != VIR_DOMAIN_NET_TYPE_BRIDGE && + (*actual)->type != VIR_DOMAIN_NET_TYPE_DIRECT) {
This is a lot of dereferencing through *actual. It might be easier to delay the dereferencing to the very end, by writing: static int virDomainActualNetDefParseXML(xmlNodePtr node, xmlXPathContextPtr ctxt, virDomainActualNetDefPtr *actual) { virDomainActualNetDefPtr def = NULL; if (VIR_ALLOC(def) < 0) { virReportOOMError(); goto cleanup; } ... if (def->type != VIR_DOMAIN_NET_TYPE_BRIDGE && .... cleanup: ... if (ret < 0) { virDomainActualNetDefPtrFree(def); def = NULL; } *actual = def; return ret; }
+ + if ((*actual)->type == VIR_DOMAIN_NET_TYPE_BRIDGE) { + + (*actual)->data.bridge.brname = virXPathString("string(./source[1]/@bridge)", + ctxt); + + } else if ((*actual)->type == VIR_DOMAIN_NET_TYPE_DIRECT) {
Nit: those two blank lines don't quite match style elsewhere in the file.
@@ -9871,7 +10063,9 @@ int virDomainSaveStatus(virCapsPtr caps, const char *statusDir, virDomainObjPtr obj) { - int flags = VIR_DOMAIN_XML_SECURE|VIR_DOMAIN_XML_INTERNAL_STATUS; + int flags = VIR_DOMAIN_XML_SECURE | + VIR_DOMAIN_XML_INTERNAL_STATUS | + VIR_DOMAIN_XML_ACTUAL_NET;
This indentation looks unusual (7 spaces, but no prior hanging '(' to be flush with). I tend to write expressions like this as: int flags = (VIR_DOMAIN_XML_SECURE | VIR_DOMAIN_XML_INTERNAL_STATUS | VIR_DOMAIN_XML_ACTUAL_NET); Also, there may be some merge conflicts if my patch series for cleaning up flags usage goes in first.
+++ b/src/conf/domain_conf.h @@ -1,3 +1,4 @@ + /*
Why the spurious addition of a blank leading line?
+ case VIR_NETWORK_FORWARD_PASSTHROUGH: + if (def->bridge) { + virNetworkReportError(VIR_ERR_XML_ERROR, + _("bridge name not allowed in %s mode (network '%s'"), + virNetworkForwardTypeToString(def->forwardType), + def->name); + goto error; + } + /* Fall through to next case */
I'm not sure whether Coverity will recognize that spelling, so here's hoping. A quick 'git grep -iC2 "fall.\\?thr"' found existing spellings that Coverity appears to honor: /* fallthrough */ /* fall through */ /* Fallthrough */ but I'd have to actually check Coverity source to see what the canonical list is.
+ /* Duplicate the first interface from the pool into <forward + * dev=xxx for convenience.
Missing the paired > in the comment. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On 07/08/2011 04:02 PM, Eric Blake wrote:
On 07/05/2011 01:45 AM, Laine Stump wrote:
+ virDomainReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown type '%s' in interface's<actual> element"), type); + goto error; + } + if ((*actual)->type != VIR_DOMAIN_NET_TYPE_BRIDGE&& + (*actual)->type != VIR_DOMAIN_NET_TYPE_DIRECT) { This is a lot of dereferencing through *actual. It might be easier to delay the dereferencing to the very end,
Yeah, at the time it seemed like it was going to be easier to cleanup this way (the caller is already going to free the ActualNetDef anyway), but in practice it didn't work out. I changed it to the more standard way.
+ + if ((*actual)->type == VIR_DOMAIN_NET_TYPE_BRIDGE) { + + (*actual)->data.bridge.brname = virXPathString("string(./source[1]/@bridge)", + ctxt); + + } else if ((*actual)->type == VIR_DOMAIN_NET_TYPE_DIRECT) { Nit: those two blank lines don't quite match style elsewhere in the file.
Fixed.
@@ -9871,7 +10063,9 @@ int virDomainSaveStatus(virCapsPtr caps, const char *statusDir, virDomainObjPtr obj) { - int flags = VIR_DOMAIN_XML_SECURE|VIR_DOMAIN_XML_INTERNAL_STATUS; + int flags = VIR_DOMAIN_XML_SECURE | + VIR_DOMAIN_XML_INTERNAL_STATUS | + VIR_DOMAIN_XML_ACTUAL_NET; This indentation looks unusual (7 spaces, but no prior hanging '(' to be flush with). I tend to write expressions like this as:
int flags = (VIR_DOMAIN_XML_SECURE | VIR_DOMAIN_XML_INTERNAL_STATUS | VIR_DOMAIN_XML_ACTUAL_NET);
Also, there may be some merge conflicts if my patch series for cleaning up flags usage goes in first.
Indentation fixed. (And thanks for all the flags work, btw :-)
+++ b/src/conf/domain_conf.h @@ -1,3 +1,4 @@ + /* Why the spurious addition of a blank leading line?
I likely hit the enter key by accident just after opening the file, and didn't notice. I removed it.
+ case VIR_NETWORK_FORWARD_PASSTHROUGH: + if (def->bridge) { + virNetworkReportError(VIR_ERR_XML_ERROR, + _("bridge name not allowed in %s mode (network '%s'"), + virNetworkForwardTypeToString(def->forwardType), + def->name); + goto error; + } + /* Fall through to next case */ I'm not sure whether Coverity will recognize that spelling, so here's hoping. A quick 'git grep -iC2 "fall.\\?thr"' found existing spellings that Coverity appears to honor:
/* fallthrough */ /* fall through */ /* Fallthrough */
but I'd have to actually check Coverity source to see what the canonical list is.
Yeesh. "Fallthrough"? That sounds like a noun! I changed it to "fall through" to be sure.
+ /* Duplicate the first interface from the pool into<forward + * dev=xxx for convenience. Missing the paired> in the comment.
Fixed.

On 07/05/2011 01:45 AM, Laine Stump wrote:
The virDomainActualNetDef will only be parsed/formatted if the parse/format function is called with the VIR_DOMAIN_XML_ACTUAL_NET flags set (which is only needed when saving/loading a running domain's state info to the stateDir). To prevent this flag bit from accidentally being used in the public API, a "RESERVED" placeholder was put into the public flags enum (at the same time, I noticed there was another private flag that hadn't been reserved, so I added that one, making both of these flags #defined from the public RESERVED flags, and since it was also only used in domain_conf.c, I unpolluted domain_conf.h, putting both #defines in domain_conf.c.
It turns out that we've used internal-use flags before. See how libvirt.c filters out flags in both virDomainGetXMLDesc and virSecretGetValue if the flags are larger than 0xffff, so that it can start internal flags at 1<<16. Regarding the networking code and our discussions on whether we should split out a second internalFlags argument rather than cramming internal and external flags into a single parameter, I think we should be consistent. That is, either the existing uses of VIR_SECRET_GET_VALUE_INTERNAL_CALL and VIR_DOMAIN_XML_INTERNAL_STATUS should be factored into internalFlags arguments, or your new patches for virtual switches should define VIR_DOMAIN_XML_ACTUAL_NET to be placed alongside VIR_DOMAIN_XML_INTERNAL_STATUS
+++ b/include/libvirt/libvirt.h.in @@ -1112,6 +1112,8 @@ typedef enum { VIR_DOMAIN_XML_SECURE = (1 << 0), /* dump security sensitive information too */ VIR_DOMAIN_XML_INACTIVE = (1 << 1), /* dump inactive domain information */ VIR_DOMAIN_XML_UPDATE_CPU = (1 << 2), /* update guest CPU requirements according to host CPU */ + VIR_DOMAIN_XML_RESERVED1 = (1 << 30), /* reserved for internal used */ + VIR_DOMAIN_XML_RESERVED2 = (1 << 31), /* reserved for internal used */ } virDomainXMLFlags;
-/* Private component of virDomainXMLFlags */ -typedef enum { - VIR_DOMAIN_XML_INTERNAL_STATUS = (1<<16), /* dump internal domain status information */ -} virDomainXMLInternalFlags; -
Meanwhile, I've got a patch to libvirt.c; I think virDomainGetXMLDesc should reject an attempt to pass 1<<16, rather than silently ignore it. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On Wed, Jul 13, 2011 at 03:40:45PM -0600, Eric Blake wrote:
On 07/05/2011 01:45 AM, Laine Stump wrote:
The virDomainActualNetDef will only be parsed/formatted if the parse/format function is called with the VIR_DOMAIN_XML_ACTUAL_NET flags set (which is only needed when saving/loading a running domain's state info to the stateDir). To prevent this flag bit from accidentally being used in the public API, a "RESERVED" placeholder was put into the public flags enum (at the same time, I noticed there was another private flag that hadn't been reserved, so I added that one, making both of these flags #defined from the public RESERVED flags, and since it was also only used in domain_conf.c, I unpolluted domain_conf.h, putting both #defines in domain_conf.c.
It turns out that we've used internal-use flags before. See how libvirt.c filters out flags in both virDomainGetXMLDesc and virSecretGetValue if the flags are larger than 0xffff, so that it can start internal flags at 1<<16. Regarding the networking code and our discussions on whether we should split out a second internalFlags argument rather than cramming internal and external flags into a single parameter, I think we should be consistent.
The difference is that this was an internal impl detail not exposed in the public headers, so could be removed at any time.
Meanwhile, I've got a patch to libvirt.c; I think virDomainGetXMLDesc should reject an attempt to pass 1<<16, rather than silently ignore it.
No, that would not be a good idea, because libvirt.c is in the code path for the remote client, as well as the server side. We don't want todo any flag filtering on the remote client side at all, since we can't expect the client & server to be on the same versions. Flag filtering can only be done after dispatch via the internal driver table, eg in QEMU, LXC, etc directly. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 07/14/2011 03:42 AM, Daniel P. Berrange wrote:
It turns out that we've used internal-use flags before. See how libvirt.c filters out flags in both virDomainGetXMLDesc and virSecretGetValue if the flags are larger than 0xffff, so that it can start internal flags at 1<<16. Regarding the networking code and our discussions on whether we should split out a second internalFlags argument rather than cramming internal and external flags into a single parameter, I think we should be consistent.
The difference is that this was an internal impl detail not exposed in the public headers, so could be removed at any time.
Meanwhile, I've got a patch to libvirt.c; I think virDomainGetXMLDesc should reject an attempt to pass 1<<16, rather than silently ignore it.
No, that would not be a good idea, because libvirt.c is in the code path for the remote client, as well as the server side.
But we don't pass internal flags over the wire. That is, the only time we use internal flags is from within the same executable, where we don't go through daemon/remote.c, and therefore don't re-feed the request through src/libvirt.c, therefore are not impacted by libvirt.c rejecting internal flags.
We don't want todo any flag filtering on the remote client side at all, since we can't expect the client & server to be on the same versions. Flag filtering can only be done after dispatch via the internal driver table, eg in QEMU, LXC, etc directly.
That would be a valid concern if we passed internal flags over the wire, but we don't. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On Thu, Jul 14, 2011 at 07:32:56AM -0600, Eric Blake wrote:
On 07/14/2011 03:42 AM, Daniel P. Berrange wrote:
It turns out that we've used internal-use flags before. See how libvirt.c filters out flags in both virDomainGetXMLDesc and virSecretGetValue if the flags are larger than 0xffff, so that it can start internal flags at 1<<16. Regarding the networking code and our discussions on whether we should split out a second internalFlags argument rather than cramming internal and external flags into a single parameter, I think we should be consistent.
The difference is that this was an internal impl detail not exposed in the public headers, so could be removed at any time.
Meanwhile, I've got a patch to libvirt.c; I think virDomainGetXMLDesc should reject an attempt to pass 1<<16, rather than silently ignore it.
No, that would not be a good idea, because libvirt.c is in the code path for the remote client, as well as the server side.
But we don't pass internal flags over the wire.
That is, the only time we use internal flags is from within the same executable, where we don't go through daemon/remote.c, and therefore don't re-feed the request through src/libvirt.c, therefore are not impacted by libvirt.c rejecting internal flags.
We don't want todo any flag filtering on the remote client side at all, since we can't expect the client & server to be on the same versions. Flag filtering can only be done after dispatch via the internal driver table, eg in QEMU, LXC, etc directly.
That would be a valid concern if we passed internal flags over the wire, but we don't.
I'm thinking about the case where a value we currently use for an internal flag, gets used for a public flag in the future. In that case, any client side checks for curent internal flags would be bogus. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 07/14/2011 08:08 AM, Daniel P. Berrange wrote:
That would be a valid concern if we passed internal flags over the wire, but we don't.
I'm thinking about the case where a value we currently use for an internal flag, gets used for a public flag in the future. In that case, any client side checks for curent internal flags would be bogus.
Then we already have the bogus situation, where libvirt.c is silently filtering out internal flags. Either we must not use the public flags argument for internal flags (and thus fix driver.h to give the callback an internalFlags argument), or we are already publicly limited to at most 16 bits for virDomainGetXMLDesc due to older clients silently ignoring the upper bits. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On Thu, Jul 14, 2011 at 08:11:20AM -0600, Eric Blake wrote:
On 07/14/2011 08:08 AM, Daniel P. Berrange wrote:
That would be a valid concern if we passed internal flags over the wire, but we don't.
I'm thinking about the case where a value we currently use for an internal flag, gets used for a public flag in the future. In that case, any client side checks for curent internal flags would be bogus.
Then we already have the bogus situation, where libvirt.c is silently filtering out internal flags.
I wasn't aware of any existing filtering - we must remove any such cases. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 07/14/2011 08:14 AM, Daniel P. Berrange wrote:
On Thu, Jul 14, 2011 at 08:11:20AM -0600, Eric Blake wrote:
On 07/14/2011 08:08 AM, Daniel P. Berrange wrote:
That would be a valid concern if we passed internal flags over the wire, but we don't.
I'm thinking about the case where a value we currently use for an internal flag, gets used for a public flag in the future. In that case, any client side checks for curent internal flags would be bogus.
Then we already have the bogus situation, where libvirt.c is silently filtering out internal flags.
I wasn't aware of any existing filtering - we must remove any such cases.
Will do - I already had a local patch started for that effort, but I can improve it along these lines. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On 07/14/2011 10:26 AM, Eric Blake wrote:
On 07/14/2011 08:14 AM, Daniel P. Berrange wrote:
On Thu, Jul 14, 2011 at 08:11:20AM -0600, Eric Blake wrote:
On 07/14/2011 08:08 AM, Daniel P. Berrange wrote:
That would be a valid concern if we passed internal flags over the wire, but we don't. I'm thinking about the case where a value we currently use for an internal flag, gets used for a public flag in the future. In that case, any client side checks for curent internal flags would be bogus.
Right. That concern is what triggered my code; I fixed that, just didn't fix it in the best way.
Then we already have the bogus situation, where libvirt.c is silently filtering out internal flags. I wasn't aware of any existing filtering - we must remove any such cases.
Yeah, neither was I. I was only aware that we had an internal flags enum and a public flags enum, that they shared the same arg, and that there was nothing in the code to protect against conflicting bits in the two (runtime protection doesn't count :-P)
Will do - I already had a local patch started for that effort, but I can improve it along these lines.
Note that the new version of my patches adds an internalFlags argument only to the necessary functions within domain_conf.c, and puts my flag there. I purposefully am not doing anything about the existing flag, VIR_DOMAIN_XML_INTERNAL_STATUS. I spent several hours attempting a "pre-patch patch" to fix INTERNAL_STATUS and add an internalFlags argument to *every* function in domain_conf.c, but it became very huge very quickly, and was destined to lead to a regression somewhere due to my limited time. BTW, I've noticed that although VIR_DOMAIN_XML_INTERNAL_STATUS is defined in domain_conf.h, it isn't actually used outside of domain_conf.c, so it doesn't need to be exposed in the .h file.

Previously all networks were composed of bridge devices created and managed by libvirt, and the same operations needed to be done for all of them when they were started and stopped (create and start the bridge device, configure its MAC address and IP address, add iptables rules). The new network types are (for now at least) managed outside of libvirt, and the network object is used only to contain information about the network, which is then used as each individual guest connects itself. This means that when starting/stopping one of these new networks, we really want to do nothing, aside from marking the network as active/inactive. This has been setup as toplevel Start/Shutdown functions that do the small bit of common stuff, then have a switch statement to execute network type-specific start/shutdown code, then do a bit more common code. The type-specific functions called for the new host bridge and macvtap based types are currently empty. In the future these functions may actually do something, and we will surely add more functions that are similarly patterned. Once everything has settled, we can make a table of "sub-driver" function pointers for each network type, and store a pointer to that table in the network object, then we can replace the switch statements with calls to functions in the table. The final step in this will be to add a new table (and corresponding new functions) for new network types as they are added. --- src/network/bridge_driver.c | 188 +++++++++++++++++++++++++++++++------------ 1 files changed, 138 insertions(+), 50 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index cb49356..69d4c35 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -97,10 +97,22 @@ static void networkDriverUnlock(struct network_driver *driver) static int networkShutdown(void); -static int networkStartNetworkDaemon(struct network_driver *driver, +static int networkStartNetwork(struct network_driver *driver, + virNetworkObjPtr network); + +static int networkShutdownNetwork(struct network_driver *driver, + virNetworkObjPtr network); + +static int networkStartNetworkVirtual(struct network_driver *driver, virNetworkObjPtr network); -static int networkShutdownNetworkDaemon(struct network_driver *driver, +static int networkShutdownNetworkVirtual(struct network_driver *driver, + virNetworkObjPtr network); + +static int networkStartNetworkExternal(struct network_driver *driver, + virNetworkObjPtr network); + +static int networkShutdownNetworkExternal(struct network_driver *driver, virNetworkObjPtr network); static void networkReloadIptablesRules(struct network_driver *driver); @@ -252,9 +264,10 @@ networkAutostartConfigs(struct network_driver *driver) { for (i = 0 ; i < driver->networks.count ; i++) { virNetworkObjLock(driver->networks.objs[i]); if (driver->networks.objs[i]->autostart && - !virNetworkObjIsActive(driver->networks.objs[i]) && - networkStartNetworkDaemon(driver, driver->networks.objs[i]) < 0) { + !virNetworkObjIsActive(driver->networks.objs[i])) { + if (networkStartNetwork(driver, driver->networks.objs[i]) < 0) { /* failed to start but already logged */ + } } virNetworkObjUnlock(driver->networks.objs[i]); } @@ -1689,7 +1702,7 @@ networkAddAddrToBridge(struct network_driver *driver, } static int -networkStartNetworkDaemon(struct network_driver *driver, +networkStartNetworkVirtual(struct network_driver *driver, virNetworkObjPtr network) { int ii, err; @@ -1698,12 +1711,6 @@ networkStartNetworkDaemon(struct network_driver *driver, virNetworkIpDefPtr ipdef; char *macTapIfName = NULL; - if (virNetworkObjIsActive(network)) { - networkReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("network is already active")); - return -1; - } - /* Check to see if any network IP collides with an existing route */ if (networkCheckRouteCollision(network) < 0) return -1; @@ -1805,26 +1812,10 @@ networkStartNetworkDaemon(struct network_driver *driver, if (v6present && networkStartRadvd(network) < 0) goto err4; - /* Persist the live configuration now we have bridge info */ - if (virNetworkSaveConfig(NETWORK_STATE_DIR, network->def) < 0) { - goto err5; - } - VIR_FREE(macTapIfName); - VIR_INFO("Starting up network '%s'", network->def->name); - network->active = 1; return 0; - err5: - if (!save_err) - save_err = virSaveLastError(); - - if (network->radvdPid > 0) { - kill(network->radvdPid, SIGTERM); - network->radvdPid = -1; - } - err4: if (!save_err) save_err = virSaveLastError(); @@ -1876,25 +1867,11 @@ networkStartNetworkDaemon(struct network_driver *driver, return -1; } - -static int networkShutdownNetworkDaemon(struct network_driver *driver, +static int networkShutdownNetworkVirtual(struct network_driver *driver, virNetworkObjPtr network) { int err; - char *stateFile; - char *macTapIfName; - - VIR_INFO("Shutting down network '%s'", network->def->name); - - if (!virNetworkObjIsActive(network)) - return 0; - - stateFile = virNetworkConfigFile(NETWORK_STATE_DIR, network->def->name); - if (!stateFile) - return -1; - - unlink(stateFile); - VIR_FREE(stateFile); + char ebuf[1024]; if (network->radvdPid > 0) { char *radvdpidbase; @@ -1912,10 +1889,8 @@ static int networkShutdownNetworkDaemon(struct network_driver *driver, if (network->dnsmasqPid > 0) kill(network->dnsmasqPid, SIGTERM); - char ebuf[1024]; - if (network->def->mac_specified) { - macTapIfName = networkBridgeDummyNicName(network->def->bridge); + char *macTapIfName = networkBridgeDummyNicName(network->def->bridge); if (!macTapIfName) { virReportOOMError(); } else { @@ -1951,6 +1926,119 @@ static int networkShutdownNetworkDaemon(struct network_driver *driver, kill(network->radvdPid, SIGKILL); network->radvdPid = -1; + return 0; +} + +static int +networkStartNetworkExternal(struct network_driver *driver ATTRIBUTE_UNUSED, + virNetworkObjPtr network ATTRIBUTE_UNUSED) +{ + /* put anything here that needs to be done each time a network of + * type BRIDGE, PRIVATE, VEPA, or PASSTHROUGH is started. On + * failure, undo anything you've done, and return -1. On success + * return 0. + */ + return 0; +} + +static int networkShutdownNetworkExternal(struct network_driver *driver ATTRIBUTE_UNUSED, + virNetworkObjPtr network ATTRIBUTE_UNUSED) +{ + /* put anything here that needs to be done each time a network of + * type BRIDGE, PRIVATE, VEPA, or PASSTHROUGH is shutdown. On + * failure, undo anything you've done, and return -1. On success + * return 0. + */ + return 0; +} + +static int +networkStartNetwork(struct network_driver *driver, + virNetworkObjPtr network) +{ + int ret = 0; + + if (virNetworkObjIsActive(network)) { + networkReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("network is already active")); + return -1; + } + + switch (network->def->forwardType) { + + case VIR_NETWORK_FORWARD_NONE: + case VIR_NETWORK_FORWARD_NAT: + case VIR_NETWORK_FORWARD_ROUTE: + ret = networkStartNetworkVirtual(driver, network); + break; + + case VIR_NETWORK_FORWARD_BRIDGE: + case VIR_NETWORK_FORWARD_PRIVATE: + case VIR_NETWORK_FORWARD_VEPA: + case VIR_NETWORK_FORWARD_PASSTHROUGH: + ret = networkStartNetworkExternal(driver, network); + break; + } + + if (ret < 0) + return ret; + + /* Persist the live configuration now that anything autogenerated + * is setup. + */ + if ((ret = virNetworkSaveConfig(NETWORK_STATE_DIR, network->def)) < 0) { + goto error; + } + + VIR_INFO("Starting up network '%s'", network->def->name); + network->active = 1; + +error: + if (ret < 0) { + virErrorPtr save_err = virSaveLastError(); + int save_errno = errno; + networkShutdownNetwork(driver, network); + virSetError(save_err); + virFreeError(save_err); + errno = save_errno; + } + return ret; +} + +static int networkShutdownNetwork(struct network_driver *driver, + virNetworkObjPtr network) +{ + int ret = 0; + char *stateFile; + + VIR_INFO("Shutting down network '%s'", network->def->name); + + if (!virNetworkObjIsActive(network)) + return 0; + + stateFile = virNetworkConfigFile(NETWORK_STATE_DIR, network->def->name); + if (!stateFile) + return -1; + + unlink(stateFile); + VIR_FREE(stateFile); + + switch (network->def->forwardType) { + + case VIR_NETWORK_FORWARD_NONE: + case VIR_NETWORK_FORWARD_NAT: + case VIR_NETWORK_FORWARD_ROUTE: + ret = networkShutdownNetworkVirtual(driver, network); + break; + + case VIR_NETWORK_FORWARD_BRIDGE: + case VIR_NETWORK_FORWARD_PRIVATE: + case VIR_NETWORK_FORWARD_VEPA: + case VIR_NETWORK_FORWARD_PASSTHROUGH: + ret = networkShutdownNetworkExternal(driver, network); + break; + } + network->active = 0; if (network->newDef) { @@ -1959,7 +2047,7 @@ static int networkShutdownNetworkDaemon(struct network_driver *driver, network->newDef = NULL; } - return 0; + return ret; } @@ -2187,7 +2275,7 @@ static virNetworkPtr networkCreate(virConnectPtr conn, const char *xml) { goto cleanup; def = NULL; - if (networkStartNetworkDaemon(driver, network) < 0) { + if (networkStartNetwork(driver, network) < 0) { virNetworkRemoveInactive(&driver->networks, network); network = NULL; @@ -2389,7 +2477,7 @@ static int networkStart(virNetworkPtr net) { goto cleanup; } - ret = networkStartNetworkDaemon(driver, network); + ret = networkStartNetwork(driver, network); cleanup: if (network) @@ -2418,7 +2506,7 @@ static int networkDestroy(virNetworkPtr net) { goto cleanup; } - ret = networkShutdownNetworkDaemon(driver, network); + ret = networkShutdownNetwork(driver, network); if (!network->persistent) { virNetworkRemoveInactive(&driver->networks, network); -- 1.7.3.4

On Tue, Jul 05, 2011 at 03:45:56AM -0400, Laine Stump wrote:
Previously all networks were composed of bridge devices created and managed by libvirt, and the same operations needed to be done for all of them when they were started and stopped (create and start the bridge device, configure its MAC address and IP address, add iptables rules). The new network types are (for now at least) managed outside of libvirt, and the network object is used only to contain information about the network, which is then used as each individual guest connects itself.
This means that when starting/stopping one of these new networks, we really want to do nothing, aside from marking the network as active/inactive.
This has been setup as toplevel Start/Shutdown functions that do the small bit of common stuff, then have a switch statement to execute network type-specific start/shutdown code, then do a bit more common code. The type-specific functions called for the new host bridge and macvtap based types are currently empty.
In the future these functions may actually do something, and we will surely add more functions that are similarly patterned. Once everything has settled, we can make a table of "sub-driver" function pointers for each network type, and store a pointer to that table in the network object, then we can replace the switch statements with calls to functions in the table.
The final step in this will be to add a new table (and corresponding new functions) for new network types as they are added. --- src/network/bridge_driver.c | 188 +++++++++++++++++++++++++++++++------------ 1 files changed, 138 insertions(+), 50 deletions(-)
ACK, there is pretty much no functional change for existing network types.
+ + VIR_INFO("Starting up network '%s'", network->def->name); + network->active = 1;
...
network->active = 0;
We could take this opportunity to change to using a bool and true/false Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 07/08/2011 09:32 AM, Daniel P. Berrange wrote:
On Tue, Jul 05, 2011 at 03:45:56AM -0400, Laine Stump wrote:
Previously all networks were composed of bridge devices created and managed by libvirt, and the same operations needed to be done for all of them when they were started and stopped (create and start the bridge device, configure its MAC address and IP address, add iptables rules). The new network types are (for now at least) managed outside of libvirt, and the network object is used only to contain information about the network, which is then used as each individual guest connects itself.
This means that when starting/stopping one of these new networks, we really want to do nothing, aside from marking the network as active/inactive.
This has been setup as toplevel Start/Shutdown functions that do the small bit of common stuff, then have a switch statement to execute network type-specific start/shutdown code, then do a bit more common code. The type-specific functions called for the new host bridge and macvtap based types are currently empty.
In the future these functions may actually do something, and we will surely add more functions that are similarly patterned. Once everything has settled, we can make a table of "sub-driver" function pointers for each network type, and store a pointer to that table in the network object, then we can replace the switch statements with calls to functions in the table.
The final step in this will be to add a new table (and corresponding new functions) for new network types as they are added. --- src/network/bridge_driver.c | 188 +++++++++++++++++++++++++++++++------------ 1 files changed, 138 insertions(+), 50 deletions(-) ACK, there is pretty much no functional change for existing network types.
+ + VIR_INFO("Starting up network '%s'", network->def->name); + network->active = 1; ...
network->active = 0;
We could take this opportunity to change to using a bool and true/false
I think I'd rather do that in a separate patch - there are several other "ints used as booleans" in network_conf, domain_conf, and nwfilter_conf (at least) and it would probably be good to make them all consistent.

The qemu driver accesses fields in the virDomainNetDef directly, but with the advent of the virDomainActualNetDef, some pieces of information may be found in a different place (the ActualNetDef) if the network connection is of type='network' and that network is of forward type='bridge|private|vepa|passthrough'. The previous patch added functions to mask this difference from callers - they hide the decision making process and just pick the value from the proper place. This patch uses those functions in the qemu driver as a first step in making qemu work with the new network types. At this point, it's assumed that the virDomainActualNetDef is already properly initialized (it isn't yet). --- src/qemu/qemu_command.c | 44 ++++++++++++++++++++++++++------------------ src/qemu/qemu_driver.c | 30 ++++++++++++++++++++++++++++-- src/qemu/qemu_hotplug.c | 16 +++++++++------- src/qemu/qemu_migration.c | 12 ++++++------ src/qemu/qemu_process.c | 10 ++++++---- 5 files changed, 75 insertions(+), 37 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index bd47eae..d1ecaf4 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -125,9 +125,12 @@ qemuPhysIfaceConnect(virDomainDefPtr def, net->model && STREQ(net->model, "virtio")) vnet_hdr = 1; - rc = openMacvtapTap(net->ifname, net->mac, net->data.direct.linkdev, - net->data.direct.mode, vnet_hdr, def->uuid, - net->data.direct.virtPortProfile, &res_ifname, + rc = openMacvtapTap(net->ifname, net->mac, + virDomainNetGetActualDirectDev(net), + virDomainNetGetActualDirectMode(net), + vnet_hdr, def->uuid, + virDomainNetGetActualDirectVirtPortProfile(net), + &res_ifname, vmop, driver->stateDir); if (rc >= 0) { qemuAuditNetDevice(def, net, res_ifname, true); @@ -148,9 +151,10 @@ qemuPhysIfaceConnect(virDomainDefPtr def, err = virDomainConfNWFilterInstantiate(conn, net); if (err) { VIR_FORCE_CLOSE(rc); - delMacvtap(net->ifname, net->mac, net->data.direct.linkdev, - net->data.direct.mode, - net->data.direct.virtPortProfile, + delMacvtap(net->ifname, net->mac, + virDomainNetGetActualDirectDev(net), + virDomainNetGetActualDirectMode(net), + virDomainNetGetActualDirectVirtPortProfile(net), driver->stateDir); VIR_FREE(net->ifname); } @@ -184,8 +188,9 @@ qemuNetworkIfaceConnect(virDomainDefPtr def, int vnet_hdr = 0; int template_ifname = 0; unsigned char tapmac[VIR_MAC_BUFLEN]; + int actualType = virDomainNetGetActualType(net); - if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { + if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK) { int active, fail = 0; virErrorPtr errobj; virNetworkPtr network = virNetworkLookupByName(conn, @@ -218,14 +223,15 @@ qemuNetworkIfaceConnect(virDomainDefPtr def, if (fail) return -1; - } else if (net->type == VIR_DOMAIN_NET_TYPE_BRIDGE) { - if (!(brname = strdup(net->data.bridge.brname))) { + } else if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) { + if (!(brname = strdup(virDomainNetGetActualBridgeName(net)))) { virReportOOMError(); return -1; } } else { qemuReportError(VIR_ERR_INTERNAL_ERROR, - _("Network type %d is not supported"), net->type); + _("Network type %d is not supported"), + virDomainNetGetActualType(net)); return -1; } @@ -1829,7 +1835,7 @@ qemuBuildHostNetStr(virDomainNetDefPtr net, bool is_tap = false; virBuffer buf = VIR_BUFFER_INITIALIZER; - switch (net->type) { + switch (virDomainNetGetActualType(net)) { case VIR_DOMAIN_NET_TYPE_NETWORK: case VIR_DOMAIN_NET_TYPE_BRIDGE: case VIR_DOMAIN_NET_TYPE_DIRECT: @@ -1857,7 +1863,7 @@ qemuBuildHostNetStr(virDomainNetDefPtr net, case VIR_DOMAIN_NET_TYPE_SERVER: case VIR_DOMAIN_NET_TYPE_MCAST: virBufferAddLit(&buf, "socket"); - switch (net->type) { + switch (virDomainNetGetActualType(net)) { case VIR_DOMAIN_NET_TYPE_CLIENT: virBufferAsprintf(&buf, "%cconnect=%s:%d", type_sep, @@ -3658,6 +3664,7 @@ qemuBuildCommandLine(virConnectPtr conn, char vhostfd_name[50] = ""; int vlan; int bootindex = bootNet; + int actualType; bootNet = 0; if (!bootindex) @@ -3670,8 +3677,9 @@ qemuBuildCommandLine(virConnectPtr conn, else vlan = i; - if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK || - net->type == VIR_DOMAIN_NET_TYPE_BRIDGE) { + actualType = virDomainNetGetActualType(net); + if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK || + actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) { int tapfd = qemuNetworkIfaceConnect(def, conn, driver, net, qemuCaps); if (tapfd < 0) @@ -3683,7 +3691,7 @@ qemuBuildCommandLine(virConnectPtr conn, if (snprintf(tapfd_name, sizeof(tapfd_name), "%d", tapfd) >= sizeof(tapfd_name)) goto no_memory; - } else if (net->type == VIR_DOMAIN_NET_TYPE_DIRECT) { + } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { int tapfd = qemuPhysIfaceConnect(def, conn, driver, net, qemuCaps, vmop); if (tapfd < 0) @@ -3697,9 +3705,9 @@ qemuBuildCommandLine(virConnectPtr conn, goto no_memory; } - if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK || - net->type == VIR_DOMAIN_NET_TYPE_BRIDGE || - net->type == VIR_DOMAIN_NET_TYPE_DIRECT) { + if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK || + actualType == VIR_DOMAIN_NET_TYPE_BRIDGE || + actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { /* Attempt to use vhost-net mode for these types of network device */ int vhostfd; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8105910..e552cff 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3935,9 +3935,35 @@ static char *qemuDomainXMLToNative(virConnectPtr conn, for (i = 0 ; i < def->nnets ; i++) { virDomainNetDefPtr net = def->nets[i]; int bootIndex = net->bootIndex; - if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK || - net->type == VIR_DOMAIN_NET_TYPE_DIRECT) { + if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { + int actualType = virDomainNetGetActualType(net); VIR_FREE(net->data.network.name); + VIR_FREE(net->data.network.portgroup); + if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) { + char *brname = strdup(virDomainNetGetActualBridgeName(net)); + virDomainActualNetDefFree(net->data.network.actual); + + memset(net, 0, sizeof *net); + + net->type = VIR_DOMAIN_NET_TYPE_ETHERNET; + net->data.ethernet.dev = brname; + net->data.ethernet.script = NULL; + net->data.ethernet.ipaddr = NULL; + } else { + /* actualType is either NETWORK or DIRECT. In either + * case, the best we can do is NULL everything out. + */ + virDomainActualNetDefFree(net->data.network.actual); + memset(net, 0, sizeof *net); + + net->type = VIR_DOMAIN_NET_TYPE_ETHERNET; + net->data.ethernet.dev = NULL; + net->data.ethernet.script = NULL; + net->data.ethernet.ipaddr = NULL; + } + } else if (net->type == VIR_DOMAIN_NET_TYPE_DIRECT) { + VIR_FREE(net->data.direct.linkdev); + VIR_FREE(net->data.direct.virtPortProfile); memset(net, 0, sizeof *net); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 5049f6e..a5fcdc9 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -603,6 +603,7 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, virDomainDevicePCIAddress guestAddr; int vlan; bool releaseaddr = false; + int actualType = virDomainNetGetActualType(net); if (!qemuCapsGet(priv->qemuCaps, QEMU_CAPS_HOST_NET_ADD)) { qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -610,14 +611,14 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, return -1; } - if (net->type == VIR_DOMAIN_NET_TYPE_BRIDGE || - net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { + if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE || + actualType == VIR_DOMAIN_NET_TYPE_NETWORK) { if ((tapfd = qemuNetworkIfaceConnect(vm->def, conn, driver, net, priv->qemuCaps)) < 0) return -1; if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, &vhostfd) < 0) goto cleanup; - } else if (net->type == VIR_DOMAIN_NET_TYPE_DIRECT) { + } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { if ((tapfd = qemuPhysIfaceConnect(vm->def, conn, driver, net, priv->qemuCaps, VIR_VM_OP_CREATE)) < 0) @@ -1608,10 +1609,11 @@ int qemuDomainDetachNetDevice(struct qemud_driver *driver, virDomainConfNWFilterTeardown(detach); #if WITH_MACVTAP - if (detach->type == VIR_DOMAIN_NET_TYPE_DIRECT) { - delMacvtap(detach->ifname, detach->mac, detach->data.direct.linkdev, - detach->data.direct.mode, - detach->data.direct.virtPortProfile, + if (virDomainNetGetActualType(detach) == VIR_DOMAIN_NET_TYPE_DIRECT) { + delMacvtap(detach->ifname, detach->mac, + virDomainNetGetActualDirectDev(detach), + virDomainNetGetActualDirectMode(detach), + virDomainNetGetActualDirectVirtPortProfile(detach), driver->stateDir); VIR_FREE(detach->ifname); } diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 9622021..5d3ae32 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2394,11 +2394,11 @@ qemuMigrationVPAssociatePortProfiles(virDomainDefPtr def) { for (i = 0; i < def->nnets; i++) { net = def->nets[i]; - if (net->type == VIR_DOMAIN_NET_TYPE_DIRECT) { + if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_DIRECT) { if (vpAssociatePortProfileId(net->ifname, net->mac, - net->data.direct.linkdev, - net->data.direct.virtPortProfile, + virDomainNetGetActualDirectDev(net), + virDomainNetGetActualDirectVirtPortProfile(net), def->uuid, VIR_VM_OP_MIGRATE_IN_FINISH) != 0) goto err_exit; @@ -2411,11 +2411,11 @@ qemuMigrationVPAssociatePortProfiles(virDomainDefPtr def) { err_exit: for (i = 0; i < last_good_net; i++) { net = def->nets[i]; - if (net->type == VIR_DOMAIN_NET_TYPE_DIRECT) { + if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_DIRECT) { vpDisassociatePortProfileId(net->ifname, net->mac, - net->data.direct.linkdev, - net->data.direct.virtPortProfile, + virDomainNetGetActualDirectDev(net), + virDomainNetGetActualDirectVirtPortProfile(net), VIR_VM_OP_MIGRATE_IN_FINISH); } } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index fa7face..9fec746 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2902,10 +2902,12 @@ void qemuProcessStop(struct qemud_driver *driver, def = vm->def; for (i = 0; i < def->nnets; i++) { virDomainNetDefPtr net = def->nets[i]; - if (net->type == VIR_DOMAIN_NET_TYPE_DIRECT) { - delMacvtap(net->ifname, net->mac, net->data.direct.linkdev, - net->data.direct.mode, - net->data.direct.virtPortProfile, driver->stateDir); + if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_DIRECT) { + delMacvtap(net->ifname, net->mac, + virDomainNetGetActualDirectDev(net), + virDomainNetGetActualDirectMode(net), + virDomainNetGetActualDirectVirtPortProfile(net), + driver->stateDir); VIR_FREE(net->ifname); } } -- 1.7.3.4

On 07/05/2011 01:45 AM, Laine Stump wrote:
The qemu driver accesses fields in the virDomainNetDef directly, but with the advent of the virDomainActualNetDef, some pieces of information may be found in a different place (the ActualNetDef) if the network connection is of type='network' and that network is of forward type='bridge|private|vepa|passthrough'. The previous patch added functions to mask this difference from callers - they hide the decision making process and just pick the value from the proper place.
This patch uses those functions in the qemu driver as a first step in making qemu work with the new network types. At this point, it's assumed that the virDomainActualNetDef is already properly initialized (it isn't yet).
Is this going to bite anyone during bisection of this patch series? Hopefully not, so I'm not sure how much you want to rework this while rebasing, which means you can probably keep the code as-is. But my approach would have been: patch 1 - introduce wrapper functions that make no semantic change, while updating all callers to use wrapper functions. Something like: int virDomainNetGetActualType(virDomainNetDefPtr iface) { return iface->type; } and replace all uses of iface->type with virDomainNetGetActualType(). patch 2 - enhance wrapper functions to actually look into virDomainActualNetDef, preferably while guaranteeing that it is initialized correctly at this stage, you fix the body of virDomainNetGetActualType to: if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK || !iface->data.network.actual) return iface->type; return iface->data.network.actual->type;
+++ b/src/qemu/qemu_driver.c @@ -3935,9 +3935,35 @@ static char *qemuDomainXMLToNative(virConnectPtr conn, for (i = 0 ; i < def->nnets ; i++) { virDomainNetDefPtr net = def->nets[i]; int bootIndex = net->bootIndex; - if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK || - net->type == VIR_DOMAIN_NET_TYPE_DIRECT) { + if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { + int actualType = virDomainNetGetActualType(net); VIR_FREE(net->data.network.name); + VIR_FREE(net->data.network.portgroup); + if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) {
And here is an instance where you are refactoring existing code (converting net->type to virDomainNetGetActualType(net)) as well as adding new code (taking action if actualType is TYPE_BRIDGE). Separating the refactoring from the introduction of new features can make review a bit easier.
+ char *brname = strdup(virDomainNetGetActualBridgeName(net)); + virDomainActualNetDefFree(net->data.network.actual); + + memset(net, 0, sizeof *net); + + net->type = VIR_DOMAIN_NET_TYPE_ETHERNET; + net->data.ethernet.dev = brname;
Need to check for strdup failure, rather than setting dev to NULL. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On 07/08/2011 04:18 PM, Eric Blake wrote:
On 07/05/2011 01:45 AM, Laine Stump wrote:
The qemu driver accesses fields in the virDomainNetDef directly, but with the advent of the virDomainActualNetDef, some pieces of information may be found in a different place (the ActualNetDef) if the network connection is of type='network' and that network is of forward type='bridge|private|vepa|passthrough'. The previous patch added functions to mask this difference from callers - they hide the decision making process and just pick the value from the proper place.
This patch uses those functions in the qemu driver as a first step in making qemu work with the new network types. At this point, it's assumed that the virDomainActualNetDef is already properly initialized (it isn't yet). Is this going to bite anyone during bisection of this patch series?
No. I misused the word "initialized" there. I probably should have just said "set". The virDomainActualNetDef *is* "properly initialized" to NULL, and when it's null, all behavior with this patch in place is identical to what it would have been without the patch. What is being assumed here is that an ActualNetDef might really be present, but one never is, that's all.
Hopefully not, so I'm not sure how much you want to rework this while rebasing, which means you can probably keep the code as-is. But my approach would have been:
patch 1 - introduce wrapper functions that make no semantic change, while updating all callers to use wrapper functions. Something like:
int virDomainNetGetActualType(virDomainNetDefPtr iface) { return iface->type; }
and replace all uses of iface->type with virDomainNetGetActualType().
We can't replace *all* uses. There are times when we want to know the original type.
patch 2 - enhance wrapper functions to actually look into virDomainActualNetDef, preferably while guaranteeing that it is initialized correctly
at this stage, you fix the body of virDomainNetGetActualType to:
if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK || !iface->data.network.actual) return iface->type; return iface->data.network.actual->type;
I don't really see this two stage process (well, it was already 2, and this turns it into 3) as necessary, because 1) the code that added the ActualNetDef also ensured that it was always initialized to NULL, 2) there was no place in the code that changed the ActualNetDef, and 3) if the ActualNetDef is NULL, virDomainNetGetActualType will *always* return iface->type.
+++ b/src/qemu/qemu_driver.c @@ -3935,9 +3935,35 @@ static char *qemuDomainXMLToNative(virConnectPtr conn, for (i = 0 ; i< def->nnets ; i++) { virDomainNetDefPtr net = def->nets[i]; int bootIndex = net->bootIndex; - if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK || - net->type == VIR_DOMAIN_NET_TYPE_DIRECT) { + if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { + int actualType = virDomainNetGetActualType(net); VIR_FREE(net->data.network.name); + VIR_FREE(net->data.network.portgroup); + if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) { And here is an instance where you are refactoring existing code (converting net->type to virDomainNetGetActualType(net)) as well as adding new code (taking action if actualType is TYPE_BRIDGE). Separating the refactoring from the introduction of new features can make review a bit easier.
Okay, I can agree with that. I can put that in a different patch.
+ char *brname = strdup(virDomainNetGetActualBridgeName(net)); + virDomainActualNetDefFree(net->data.network.actual); + + memset(net, 0, sizeof *net); + + net->type = VIR_DOMAIN_NET_TYPE_ETHERNET; + net->data.ethernet.dev = brname; Need to check for strdup failure, rather than setting dev to NULL.
Yep. I missed that one.

The network driver needs to assign physical devices for use by modes that use macvtap, keeping track of which physical devices are in use (and how many instances, when the devices can be shared). Three calls are added: networkAllocateActualDevice - finds a physical device for use by the domain, and sets up the virDomainActualNetDef accordingly. networkNotifyActualDevice - assumes that the domain was already running, but libvirtd was restarted, and needs to be notified by each already-running domain about what interfaces they are using. networkReleaseActualDevice - decrements the usage count of the allocated physical device, and frees the virDomainActualNetDef to avoid later accidentally using the device. bridge_driver.[hc] - the new APIs qemu_(command|driver|hotplug|process).c - add calls to the above APIs in the appropriate places. tests/Makefile.am - need to include libvirt_driver_network.la whenever libvirt_driver_qemu.la is linked, to avoid unreferenced symbols (in functions that are never called by the test programs...) --- src/network/bridge_driver.c | 398 +++++++++++++++++++++++++++++++++++++++++++ src/network/bridge_driver.h | 6 + src/qemu/qemu_command.c | 11 ++ src/qemu/qemu_hotplug.c | 41 +++-- src/qemu/qemu_process.c | 26 +++- tests/Makefile.am | 12 +- 6 files changed, 476 insertions(+), 18 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 69d4c35..02d7b8b 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2702,3 +2702,401 @@ int networkRegister(void) { virRegisterStateDriver(&networkStateDriver); return 0; } + +/********************************************************/ + +/* Private API to deal with logical switch capabilities. + * These functions are exported so that other parts of libvirt can + * call them, but are not part of the public API and not in the + * driver's function table. If we ever have more than one network + * driver, we will need to present these functions via a second + * "backend" function table. + */ + +/* networkAllocateActualDevice: + * @iface: the original NetDef from the domain + * + * Looks up the network reference by iface, allocates a physical + * device from that network (if appropriate), and returns with the + * virDomainActualNetDef filled in accordingly. If there are no + * changes to be made in the netdef, then just leave the actualdef + * empty. + * + * Returns 0 on success, -1 on failure. + */ +int +networkAllocateActualDevice(virDomainNetDefPtr iface) +{ + struct network_driver *driver = driverState; + virNetworkObjPtr network; + virNetworkDefPtr netdef; + int ret = -1; + + if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK) + return 0; + + virDomainActualNetDefFree(iface->data.network.actual); + iface->data.network.actual = NULL; + + networkDriverLock(driver); + network = virNetworkFindByName(&driver->networks, iface->data.network.name); + networkDriverUnlock(driver); + if (!network) { + networkReportError(VIR_ERR_NO_NETWORK, + _("no network with matching name '%s'"), + iface->data.network.name); + goto cleanup; + } + + netdef = network->def; + if ((netdef->forwardType == VIR_NETWORK_FORWARD_BRIDGE) && + netdef->bridge) { + + /* <forward type='bridge'/> <bridge name='xxx'/> + * is VIR_DOMAIN_NET_TYPE_BRIDGE + */ + + if (VIR_ALLOC(iface->data.network.actual) < 0) { + virReportOOMError(); + goto cleanup; + } + + iface->data.network.actual->type = VIR_DOMAIN_NET_TYPE_BRIDGE; + iface->data.network.actual->data.bridge.brname = strdup(netdef->bridge); + if (!iface->data.network.actual->data.bridge.brname) { + virReportOOMError(); + goto cleanup; + } + + } else if ((netdef->forwardType == VIR_NETWORK_FORWARD_BRIDGE) || + (netdef->forwardType == VIR_NETWORK_FORWARD_PRIVATE) || + (netdef->forwardType == VIR_NETWORK_FORWARD_VEPA) || + (netdef->forwardType == VIR_NETWORK_FORWARD_PASSTHROUGH)) { + virVirtualPortProfileParamsPtr virtport = NULL; + + /* <forward type='bridge|private|vepa|passthrough'> are all + * VIR_DOMAIN_NET_TYPE_DIRECT. + */ + + if (VIR_ALLOC(iface->data.network.actual) < 0) { + virReportOOMError(); + goto cleanup; + } + + /* Set type=direct and appropriate <source mode='xxx'/> */ + iface->data.network.actual->type = VIR_DOMAIN_NET_TYPE_DIRECT; + switch (netdef->forwardType) { + case VIR_NETWORK_FORWARD_BRIDGE: + iface->data.network.actual->data.direct.mode = VIR_MACVTAP_MODE_BRIDGE; + break; + case VIR_NETWORK_FORWARD_PRIVATE: + iface->data.network.actual->data.direct.mode = VIR_MACVTAP_MODE_PRIVATE; + break; + case VIR_NETWORK_FORWARD_VEPA: + iface->data.network.actual->data.direct.mode = VIR_MACVTAP_MODE_VEPA; + break; + case VIR_NETWORK_FORWARD_PASSTHROUGH: + iface->data.network.actual->data.direct.mode = VIR_MACVTAP_MODE_PASSTHRU; + break; + } + + /* Find the most specific virtportprofile and copy it */ + if (iface->data.network.virtPortProfile) { + virtport = iface->data.network.virtPortProfile; + } else { + virPortGroupDefPtr portgroup + = virPortGroupFindByName(netdef, iface->data.network.portgroup); + if (portgroup) + virtport = portgroup->virtPortProfile; + else + virtport = netdef->virtPortProfile; + } + if (virtport) { + if (VIR_ALLOC(iface->data.network.actual->data.direct.virtPortProfile) < 0) { + virReportOOMError(); + goto cleanup; + } + /* There are no pointers in a virtualPortProfile, so a shallow copy + * is sufficient + */ + *iface->data.network.actual->data.direct.virtPortProfile = *virtport; + } + /* If there is only a single device, just return it (caller will detect + * any error if exclusive use is required but could be acquired). + */ + if (netdef->nForwardIfs == 0) { + + if (!netdef->forwardDev) { + networkReportError(VIR_ERR_INTERNAL_ERROR, + _("network '%s' uses a direct mode, but has no forward dev and no interface pool"), + netdef->name); + goto cleanup; + } + iface->data.network.actual->data.direct.linkdev + = strdup(netdef->forwardDev); + if (!iface->data.network.actual->data.direct.linkdev) { + virReportOOMError(); + goto cleanup; + } + } else { + int ii; + virNetworkForwardIfDefPtr dev = NULL; + + /* pick an interface from the pool */ + + /* PASSTHROUGH mode, and PRIVATE Mode + 802.1Qbh both require + * exclusive access to a device, so current usageCount must be + * 0. Other modes can share, so just search for the one with + * the lowest usageCount. + */ + if ((netdef->forwardType == VIR_NETWORK_FORWARD_PASSTHROUGH) || + ((netdef->forwardType == VIR_NETWORK_FORWARD_PRIVATE) && + iface->data.network.actual->data.direct.virtPortProfile && + (iface->data.network.actual->data.direct.virtPortProfile->virtPortType + == VIR_VIRTUALPORT_8021QBH))) { + /* pick first dev with 0 usageCount */ + + for (ii = 0; ii < netdef->nForwardIfs; ii++) { + if (netdef->forwardIfs[ii].usageCount == 0) { + dev = &netdef->forwardIfs[ii]; + break; + } + } + } else { + /* pick least used dev */ + dev = &netdef->forwardIfs[0]; + for (ii = 1; ii < netdef->nForwardIfs; ii++) { + if (netdef->forwardIfs[ii].usageCount < dev->usageCount) + dev = &netdef->forwardIfs[ii]; + } + } + /* dev points at the physical device we want to use */ + if (!dev) { + networkReportError(VIR_ERR_INTERNAL_ERROR, + _("network '%s' requires exclusive access to interfaces, but none are available"), + netdef->name); + goto cleanup; + } + iface->data.network.actual->data.direct.linkdev = strdup(dev->dev); + if (!iface->data.network.actual->data.direct.linkdev) { + virReportOOMError(); + goto cleanup; + } + /* we are now assured of success, so mark the allocation */ + dev->usageCount++; + VIR_DEBUG("Using physical device %s, usageCount %d", + dev->dev, dev->usageCount); + } + } + + ret = 0; +cleanup: + if (network) + virNetworkObjUnlock(network); + if (ret < 0) { + virDomainActualNetDefFree(iface->data.network.actual); + iface->data.network.actual = NULL; + } + return ret; +} + +/* networkNotifyActualDevice: + * @iface: the domain's NetDef with an "actual" device already filled in. + * + * Called to notify the network driver when libvirtd is restarted and + * finds an already running domain. If appropriate it will force an + * allocation of the actual->direct.linkdev to get everything back in + * order. + * + * Returns 0 on success, -1 on failure. + */ +int +networkNotifyActualDevice(virDomainNetDefPtr iface) +{ + struct network_driver *driver = driverState; + virNetworkObjPtr network; + virNetworkDefPtr netdef; + char *actualDev; + int ret = -1; + + if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK) + return 0; + + if (!iface->data.network.actual || + (virDomainNetGetActualType(iface) != VIR_DOMAIN_NET_TYPE_DIRECT)) { + VIR_DEBUG("Nothing to claim from network %s", iface->data.network.name); + return 0; + } + + networkDriverLock(driver); + network = virNetworkFindByName(&driver->networks, iface->data.network.name); + networkDriverUnlock(driver); + if (!network) { + networkReportError(VIR_ERR_NO_NETWORK, + _("no network with matching name '%s'"), + iface->data.network.name); + goto cleanup; + } + + actualDev = virDomainNetGetActualDirectDev(iface); + if (!actualDev) { + networkReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("the interface uses a direct mode, but has no source dev")); + goto cleanup; + } + + netdef = network->def; + if (netdef->nForwardIfs == 0) { + + if (!netdef->forwardDev) { + networkReportError(VIR_ERR_INTERNAL_ERROR, + _("network '%s' uses a direct mode, but has no forward dev and no interface pool"), + netdef->name); + goto cleanup; + } + + if (!STREQ(actualDev, netdef->forwardDev)) { + networkReportError(VIR_ERR_INTERNAL_ERROR, + _("network '%s' forward dev='%s' doesn't match domain's '%s'"), + netdef->name, netdef->forwardDev, actualDev); + goto cleanup; + } + } else { + int ii; + virNetworkForwardIfDefPtr dev = NULL; + + /* find the matching interface in the pool and increment its usageCount */ + + for (ii = 0; ii < netdef->nForwardIfs; ii++) { + if (STREQ(actualDev, netdef->forwardIfs[ii].dev)) { + dev = &netdef->forwardIfs[ii]; + break; + } + } + /* dev points at the physical device we want to use */ + if (!dev) { + networkReportError(VIR_ERR_INTERNAL_ERROR, + _("network '%s' doesn't have dev='%s' in use by domain"), + netdef->name, actualDev); + goto cleanup; + } + + /* PASSTHROUGH mode, and PRIVATE Mode + 802.1Qbh both require + * exclusive access to a device, so current usageCount must be + * 0 in those cases. + */ + if ((dev->usageCount > 0) && + ((netdef->forwardType == VIR_NETWORK_FORWARD_PASSTHROUGH) || + ((netdef->forwardType == VIR_NETWORK_FORWARD_PRIVATE) && + iface->data.network.actual->data.direct.virtPortProfile && + (iface->data.network.actual->data.direct.virtPortProfile->virtPortType + == VIR_VIRTUALPORT_8021QBH)))) { + networkReportError(VIR_ERR_INTERNAL_ERROR, + _("network '%s' claims dev='%s' is already in use by a different domain"), + netdef->name, actualDev); + goto cleanup; + } + /* we are now assured of success, so mark the allocation */ + dev->usageCount++; + VIR_DEBUG("Using physical device %s, usageCount %d", + dev->dev, dev->usageCount); + } + + ret = 0; +cleanup: + if (network) + virNetworkObjUnlock(network); + return ret; +} + + +/* networkReleaseActualDevice() - Given a domain <interface element + * that previously had its <actual> element filled in (and possibly a + * physical device allocated to it), free up the physical device for use + * by someone else, and free the virDomainActualNetDef. + * + * Returns 0 on success, -1 on failure. + */ +int +networkReleaseActualDevice(virDomainNetDefPtr iface) +{ + struct network_driver *driver = driverState; + virNetworkObjPtr network = NULL; + virNetworkDefPtr netdef; + char *actualDev; + int ret = -1; + + if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK) + return 0; + + if (!iface->data.network.actual || + (virDomainNetGetActualType(iface) != VIR_DOMAIN_NET_TYPE_DIRECT)) { + VIR_DEBUG("Nothing to release to network %s", iface->data.network.name); + ret = 0; + goto cleanup; + } + + networkDriverLock(driver); + network = virNetworkFindByName(&driver->networks, iface->data.network.name); + networkDriverUnlock(driver); + if (!network) { + networkReportError(VIR_ERR_NO_NETWORK, + _("no network with matching name '%s'"), + iface->data.network.name); + goto cleanup; + } + + actualDev = virDomainNetGetActualDirectDev(iface); + if (!actualDev) { + networkReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("the interface uses a direct mode, but has no source dev")); + goto cleanup; + } + + netdef = network->def; + if (netdef->nForwardIfs == 0) { + + if (!netdef->forwardDev) { + networkReportError(VIR_ERR_INTERNAL_ERROR, + _("network '%s' uses a direct mode, but has no forward dev and no interface pool"), + netdef->name); + goto cleanup; + } + + if (!STREQ(actualDev, netdef->forwardDev)) { + networkReportError(VIR_ERR_INTERNAL_ERROR, + _("network '%s' forward dev='%s' doesn't match domain's '%s'"), + netdef->name, netdef->forwardDev, actualDev); + goto cleanup; + } + } else { + int ii; + virNetworkForwardIfDefPtr dev = NULL; + + for (ii = 0; ii < netdef->nForwardIfs; ii++) { + if (STREQ(actualDev, netdef->forwardIfs[ii].dev)) { + dev = &netdef->forwardIfs[ii]; + break; + } + } + /* dev points at the physical device we've been using */ + if (!dev) { + networkReportError(VIR_ERR_INTERNAL_ERROR, + _("network '%s' doesn't have dev='%s' in use by domain"), + netdef->name, actualDev); + goto cleanup; + } + + dev->usageCount--; + VIR_DEBUG("Releasing physical device %s, usageCount %d", + dev->dev, dev->usageCount); + } + + ret = 0; +cleanup: + if (network) + virNetworkObjUnlock(network); + virDomainActualNetDefFree(iface->data.network.actual); + iface->data.network.actual = NULL; + return ret; +} diff --git a/src/network/bridge_driver.h b/src/network/bridge_driver.h index 2896c84..4f6a54d 100644 --- a/src/network/bridge_driver.h +++ b/src/network/bridge_driver.h @@ -29,10 +29,16 @@ # include "internal.h" # include "network_conf.h" +# include "domain_conf.h" # include "command.h" # include "dnsmasq.h" int networkRegister(void); + +int networkAllocateActualDevice(virDomainNetDefPtr iface); +int networkNotifyActualDevice(virDomainNetDefPtr iface); +int networkReleaseActualDevice(virDomainNetDefPtr iface); + int networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network, virCommandPtr *cmdout, char *pidfile, dnsmasqContext *dctx); diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d1ecaf4..d6a0c6d 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -37,6 +37,7 @@ #include "domain_nwfilter.h" #include "qemu_audit.h" #include "domain_conf.h" +#include "network/bridge_driver.h" #include <sys/utsname.h> #include <sys/stat.h> @@ -3677,6 +3678,13 @@ qemuBuildCommandLine(virConnectPtr conn, else vlan = i; + /* If appropriate, grab a physical device from the configured + * network's pool of devices, or resolve bridge device name + * to the one defined in the network definition. + */ + if (networkAllocateActualDevice(net) < 0) + goto error; + actualType = virDomainNetGetActualType(net); if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK || actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) { @@ -4673,6 +4681,9 @@ qemuBuildCommandLine(virConnectPtr conn, no_memory: virReportOOMError(); error: + /* free up any resources in the network driver */ + for (i = 0 ; i < def->nnets ; i++) + networkReleaseActualDevice(def->nets[i]); for (i = 0; i <= last_good_net; i++) virDomainConfNWFilterTeardown(def->nets[i]); virBufferFreeAndReset(&rbd_hosts); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index a5fcdc9..37cfbef 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -39,6 +39,7 @@ #include "files.h" #include "qemu_cgroup.h" #include "locking/domain_lock.h" +#include "network/bridge_driver.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -603,7 +604,8 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, virDomainDevicePCIAddress guestAddr; int vlan; bool releaseaddr = false; - int actualType = virDomainNetGetActualType(net); + bool iface_connected = false; + int actualType; if (!qemuCapsGet(priv->qemuCaps, QEMU_CAPS_HOST_NET_ADD)) { qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -611,18 +613,28 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, return -1; } + /* If appropriate, grab a physical device from the configured + * network's pool of devices, or resolve bridge device name + * to the one defined in the network definition. + */ + if (networkAllocateActualDevice(net) < 0) + goto cleanup; + + actualType = virDomainNetGetActualType(net); if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE || actualType == VIR_DOMAIN_NET_TYPE_NETWORK) { if ((tapfd = qemuNetworkIfaceConnect(vm->def, conn, driver, net, priv->qemuCaps)) < 0) - return -1; + goto cleanup; + iface_connected = true; if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, &vhostfd) < 0) goto cleanup; } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { if ((tapfd = qemuPhysIfaceConnect(vm->def, conn, driver, net, priv->qemuCaps, VIR_VM_OP_CREATE)) < 0) - return -1; + goto cleanup; + iface_connected = true; if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, &vhostfd) < 0) goto cleanup; } @@ -738,16 +750,19 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, vm->def->nets[vm->def->nnets++] = net; cleanup: - if ((ret != 0) && - qemuCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) && - (net->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) && - releaseaddr && - qemuDomainPCIAddressReleaseSlot(priv->pciaddrs, - net->info.addr.pci.slot) < 0) - VIR_WARN("Unable to release PCI address on NIC"); + if (ret < 0) { + if (qemuCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) && + (net->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) && + releaseaddr && + qemuDomainPCIAddressReleaseSlot(priv->pciaddrs, + net->info.addr.pci.slot) < 0) + VIR_WARN("Unable to release PCI address on NIC"); - if (ret != 0) - virDomainConfNWFilterTeardown(net); + if (iface_connected) + virDomainConfNWFilterTeardown(net); + + networkReleaseActualDevice(net); + } VIR_FREE(nicstr); VIR_FREE(netstr); @@ -1629,6 +1644,8 @@ int qemuDomainDetachNetDevice(struct qemud_driver *driver, } } + networkReleaseActualDevice(detach); + if (vm->def->nnets > 1) { memmove(vm->def->nets + i, vm->def->nets + i + 1, diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 9fec746..f4a57ff 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -56,6 +56,7 @@ #include "processinfo.h" #include "domain_nwfilter.h" #include "locking/domain_lock.h" +#include "network/bridge_driver.h" #include "uuid.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -2168,6 +2169,20 @@ int qemuProcessStopCPUs(struct qemud_driver *driver, virDomainObjPtr vm, static int +qemuProcessNotifyNets(virDomainDefPtr def) +{ + int ii; + + for (ii = 0 ; ii < def->nnets ; ii++) { + virDomainNetDefPtr net = def->nets[ii]; + if (networkNotifyActualDevice(net) < 0) + return -1; + } + return 0; +} + + +static int qemuProcessFiltersInstantiate(virConnectPtr conn, virDomainDefPtr def) { @@ -2279,6 +2294,9 @@ qemuProcessReconnect(void *payload, const void *name ATTRIBUTE_UNUSED, void *opa if (virSecurityManagerReserveLabel(driver->securityManager, obj) < 0) goto error; + if (qemuProcessNotifyNets(obj->def) < 0) + goto error; + if (qemuProcessFiltersInstantiate(conn, obj->def)) goto error; @@ -2898,10 +2916,10 @@ void qemuProcessStop(struct qemud_driver *driver, qemuDomainReAttachHostDevices(driver, vm->def); -#if WITH_MACVTAP def = vm->def; for (i = 0; i < def->nnets; i++) { virDomainNetDefPtr net = def->nets[i]; +#if WITH_MACVTAP if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_DIRECT) { delMacvtap(net->ifname, net->mac, virDomainNetGetActualDirectDev(net), @@ -2910,8 +2928,12 @@ void qemuProcessStop(struct qemud_driver *driver, driver->stateDir); VIR_FREE(net->ifname); } - } #endif + /* release the physical device (or any other resources used by + * this interface in the network driver + */ + networkReleaseActualDevice(net); + } retry: if ((ret = qemuRemoveCgroup(driver, vm, 0)) < 0) { diff --git a/tests/Makefile.am b/tests/Makefile.am index 30b2d95..cf01c23 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -314,23 +314,27 @@ EXTRA_DIST += xml2sexprtest.c sexpr2xmltest.c xmconfigtest.c \ endif if WITH_QEMU + +qemu_LDADDS = ../src/libvirt_driver_qemu.la \ + ../src/libvirt_driver_network.la + qemuxml2argvtest_SOURCES = \ qemuxml2argvtest.c testutilsqemu.c testutilsqemu.h \ testutils.c testutils.h -qemuxml2argvtest_LDADD = ../src/libvirt_driver_qemu.la $(LDADDS) +qemuxml2argvtest_LDADD = $(qemu_LDADDS) $(LDADDS) qemuxml2xmltest_SOURCES = \ qemuxml2xmltest.c testutilsqemu.c testutilsqemu.h \ testutils.c testutils.h -qemuxml2xmltest_LDADD = ../src/libvirt_driver_qemu.la $(LDADDS) +qemuxml2xmltest_LDADD = $(qemu_LDADDS) $(LDADDS) qemuargv2xmltest_SOURCES = \ qemuargv2xmltest.c testutilsqemu.c testutilsqemu.h \ testutils.c testutils.h -qemuargv2xmltest_LDADD = ../src/libvirt_driver_qemu.la $(LDADDS) +qemuargv2xmltest_LDADD = $(qemu_LDADDS) $(LDADDS) qemuhelptest_SOURCES = qemuhelptest.c testutils.c testutils.h -qemuhelptest_LDADD = ../src/libvirt_driver_qemu.la $(LDADDS) +qemuhelptest_LDADD = $(qemu_LDADDS) $(LDADDS) else EXTRA_DIST += qemuxml2argvtest.c qemuxml2xmltest.c qemuargv2xmltest.c qemuhelptest.c testutilsqemu.c testutilsqemu.h endif -- 1.7.3.4

I realized that PATCH 10/10 would cause the build to fail if someone did a build --with-qemu but --without-network. I'm squashing the following into the original patch to remedy that. I'm not really a fan of putting #if all over the place, but this is similar to what's done with WITH_MACVTAP, so at least there's precedence. (This is necessary because this new "backend API" to the network driver isn't called via a pointer table filled in at runtime, as is done with the public API). --- src/qemu/qemu_command.c | 5 ++++- src/qemu/qemu_hotplug.c | 8 ++++++-- src/qemu/qemu_process.c | 8 ++++++-- tests/Makefile.am | 7 +++++-- 4 files changed, 21 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d6a0c6d..0b957cb 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3678,13 +3678,14 @@ qemuBuildCommandLine(virConnectPtr conn, else vlan = i; +#if WITH_NETWORK /* If appropriate, grab a physical device from the configured * network's pool of devices, or resolve bridge device name * to the one defined in the network definition. */ if (networkAllocateActualDevice(net) < 0) goto error; - +#endif actualType = virDomainNetGetActualType(net); if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK || actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) { @@ -4682,8 +4683,10 @@ qemuBuildCommandLine(virConnectPtr conn, virReportOOMError(); error: /* free up any resources in the network driver */ +#if WITH_NETWORK for (i = 0 ; i < def->nnets ; i++) networkReleaseActualDevice(def->nets[i]); +#endif for (i = 0; i <= last_good_net; i++) virDomainConfNWFilterTeardown(def->nets[i]); virBufferFreeAndReset(&rbd_hosts); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 37cfbef..5f1a424 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -613,13 +613,14 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, return -1; } +#if WITH_NETWORK /* If appropriate, grab a physical device from the configured * network's pool of devices, or resolve bridge device name * to the one defined in the network definition. */ if (networkAllocateActualDevice(net) < 0) goto cleanup; - +#endif actualType = virDomainNetGetActualType(net); if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE || actualType == VIR_DOMAIN_NET_TYPE_NETWORK) { @@ -761,7 +762,9 @@ cleanup: if (iface_connected) virDomainConfNWFilterTeardown(net); +#if WITH_NETWORK networkReleaseActualDevice(net); +#endif } VIR_FREE(nicstr); @@ -1644,8 +1647,9 @@ int qemuDomainDetachNetDevice(struct qemud_driver *driver, } } +#if WITH_NETWORK networkReleaseActualDevice(detach); - +#endif if (vm->def->nnets > 1) { memmove(vm->def->nets + i, vm->def->nets + i + 1, diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f4a57ff..709f187 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2168,6 +2168,7 @@ int qemuProcessStopCPUs(struct qemud_driver *driver, virDomainObjPtr vm, +#if WITH_NETWORK static int qemuProcessNotifyNets(virDomainDefPtr def) { @@ -2180,7 +2181,7 @@ qemuProcessNotifyNets(virDomainDefPtr def) } return 0; } - +#endif static int qemuProcessFiltersInstantiate(virConnectPtr conn, @@ -2294,9 +2295,10 @@ qemuProcessReconnect(void *payload, const void *name ATTRIBUTE_UNUSED, void *opa if (virSecurityManagerReserveLabel(driver->securityManager, obj) < 0) goto error; +#if WITH_NETWORK if (qemuProcessNotifyNets(obj->def) < 0) goto error; - +#endif if (qemuProcessFiltersInstantiate(conn, obj->def)) goto error; @@ -2932,7 +2934,9 @@ void qemuProcessStop(struct qemud_driver *driver, /* release the physical device (or any other resources used by * this interface in the network driver */ +#if WITH_NETWORK networkReleaseActualDevice(net); +#endif } retry: diff --git a/tests/Makefile.am b/tests/Makefile.am index a9ad759..6afec28 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -319,8 +319,11 @@ endif if WITH_QEMU -qemu_LDADDS = ../src/libvirt_driver_qemu.la \ - ../src/libvirt_driver_network.la +qemu_LDADDS = ../src/libvirt_driver_qemu.la + +if WITH_NETWORK +qemu_LDADDS += ../src/libvirt_driver_network.la +endif qemuxml2argvtest_SOURCES = \ qemuxml2argvtest.c testutilsqemu.c testutilsqemu.h \ -- 1.7.3.4

On 07/05/2011 10:18 PM, Laine Stump wrote:
I realized that PATCH 10/10 would cause the build to fail if someone did a build --with-qemu but --without-network. I'm squashing the following into the original patch to remedy that.
I'm not really a fan of putting #if all over the place, but this is similar to what's done with WITH_MACVTAP, so at least there's precedence. (This is necessary because this new "backend API" to the network driver isn't called via a pointer table filled in at runtime, as is done with the public API).
Would it be any easier to instead guarantee that even when #if WITH_NETWORK is false, those same symbols are available as no-ops to allow compilation to proceed?
+#if WITH_NETWORK /* If appropriate, grab a physical device from the configured * network's pool of devices, or resolve bridge device name * to the one defined in the network definition. */ if (networkAllocateActualDevice(net) < 0) goto error; - +#endif
That is, make networkAllocateActualDevice() be a no-op that returns 0 if there is no network support compiled in, and therefore nothing to allocate.
+++ b/tests/Makefile.am @@ -319,8 +319,11 @@ endif
if WITH_QEMU
-qemu_LDADDS = ../src/libvirt_driver_qemu.la \ - ../src/libvirt_driver_network.la +qemu_LDADDS = ../src/libvirt_driver_qemu.la + +if WITH_NETWORK +qemu_LDADDS += ../src/libvirt_driver_network.la +endif
Then again, if you are compiling --without-network, you don't want to link against a library that won't be built. That would imply that any no-op stubs would have to be provided by static inline functions in the header in the no-network case. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On 07/08/2011 04:23 PM, Eric Blake wrote:
On 07/05/2011 10:18 PM, Laine Stump wrote:
I realized that PATCH 10/10 would cause the build to fail if someone did a build --with-qemu but --without-network. I'm squashing the following into the original patch to remedy that.
I'm not really a fan of putting #if all over the place, but this is similar to what's done with WITH_MACVTAP, so at least there's precedence. (This is necessary because this new "backend API" to the network driver isn't called via a pointer table filled in at runtime, as is done with the public API). Would it be any easier to instead guarantee that even when #if WITH_NETWORK is false, those same symbols are available as no-ops to allow compilation to proceed?
That would mean either defining the functions inside qemu, or creating a dummy network module replacement to be built when WITH_NETWORK is false. I like both of those options even less.
+#if WITH_NETWORK /* If appropriate, grab a physical device from the configured * network's pool of devices, or resolve bridge device name * to the one defined in the network definition. */ if (networkAllocateActualDevice(net)< 0) goto error; - +#endif That is, make networkAllocateActualDevice() be a no-op that returns 0 if there is no network support compiled in, and therefore nothing to allocate.
But where would that NOP function live? The natural place for a function called "networkSomething" is in a network driver, but there isn't one.
+++ b/tests/Makefile.am @@ -319,8 +319,11 @@ endif
if WITH_QEMU
-qemu_LDADDS = ../src/libvirt_driver_qemu.la \ - ../src/libvirt_driver_network.la +qemu_LDADDS = ../src/libvirt_driver_qemu.la + +if WITH_NETWORK +qemu_LDADDS += ../src/libvirt_driver_network.la +endif Then again, if you are compiling --without-network, you don't want to link against a library that won't be built. That would imply that any no-op stubs would have to be provided by static inline functions in the header in the no-network case.
Ah, you're doing this mail "On the Road" style (stream of consciousness with no going back to edit), so I'll respond that way too :-) Actually, I like this idea - in bridge_driver.h, I can put the function declrations inside #if WITH_NETWORK, and have a #else clause that contains inlines that return success but do nothing (exactly what is needed). That way the code in qemu won't need #if. I'll do it that way in the next version.

On 07/05/2011 01:45 AM, Laine Stump wrote:
The network driver needs to assign physical devices for use by modes that use macvtap, keeping track of which physical devices are in use (and how many instances, when the devices can be shared). Three calls are added:
networkAllocateActualDevice - finds a physical device for use by the domain, and sets up the virDomainActualNetDef accordingly.
networkNotifyActualDevice - assumes that the domain was already running, but libvirtd was restarted, and needs to be notified by each already-running domain about what interfaces they are using.
networkReleaseActualDevice - decrements the usage count of the allocated physical device, and frees the virDomainActualNetDef to avoid later accidentally using the device.
bridge_driver.[hc] - the new APIs
qemu_(command|driver|hotplug|process).c - add calls to the above APIs in the appropriate places.
tests/Makefile.am - need to include libvirt_driver_network.la whenever libvirt_driver_qemu.la is linked, to avoid unreferenced symbols (in functions that are never called by the test programs...) --- src/network/bridge_driver.c | 398 +++++++++++++++++++++++++++++++++++++++++++ src/network/bridge_driver.h | 6 + src/qemu/qemu_command.c | 11 ++ src/qemu/qemu_hotplug.c | 41 +++-- src/qemu/qemu_process.c | 26 +++- tests/Makefile.am | 12 +- 6 files changed, 476 insertions(+), 18 deletions(-)
+ + /* Find the most specific virtportprofile and copy it */ + if (iface->data.network.virtPortProfile) { + virtport = iface->data.network.virtPortProfile; + } else { + virPortGroupDefPtr portgroup + = virPortGroupFindByName(netdef, iface->data.network.portgroup); + if (portgroup) + virtport = portgroup->virtPortProfile;
Nit: Indentation looks interesting there - the line starting with = has one less space. Perhaps something to do with how your preferred editor splits assignments. My editor uses 4 spaces instead of 3 in that instance.
+ if (!netdef->forwardDev) { + networkReportError(VIR_ERR_INTERNAL_ERROR, + _("network '%s' uses a direct mode, but has no forward dev and no interface pool"), + netdef->name); + goto cleanup; + } + iface->data.network.actual->data.direct.linkdev + = strdup(netdef->forwardDev);
And here you used a multiple of four spaces.
+int +networkNotifyActualDevice(virDomainNetDefPtr iface) +{
+ + /* PASSTHROUGH mode, and PRIVATE Mode + 802.1Qbh both require + * exclusive access to a device, so current usageCount must be + * 0 in those cases. + */ + if ((dev->usageCount > 0) && + ((netdef->forwardType == VIR_NETWORK_FORWARD_PASSTHROUGH) || + ((netdef->forwardType == VIR_NETWORK_FORWARD_PRIVATE) && + iface->data.network.actual->data.direct.virtPortProfile && + (iface->data.network.actual->data.direct.virtPortProfile->virtPortType + == VIR_VIRTUALPORT_8021QBH)))) { + networkReportError(VIR_ERR_INTERNAL_ERROR, + _("network '%s' claims dev='%s' is already in use by a different domain"),
Do we have a data race here? Suppose that libvirtd is restarted while one VM is already using device 0. Is there any chance that if I'm fast enough, I can cause the creation of a second domain, and that second domain will go to pick from the interface pool before the notification pass of the libvirtd restart has completed, and mistakenly claim device 0? That is, I think we need to somehow guarantee (if we don't already) that no new domains can be created until after all existing domains have been reloaded on a libvirtd restart. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On 07/08/2011 04:37 PM, Eric Blake wrote:
On 07/05/2011 01:45 AM, Laine Stump wrote:
The network driver needs to assign physical devices for use by modes that use macvtap, keeping track of which physical devices are in use (and how many instances, when the devices can be shared). Three calls are added:
networkAllocateActualDevice - finds a physical device for use by the domain, and sets up the virDomainActualNetDef accordingly.
networkNotifyActualDevice - assumes that the domain was already running, but libvirtd was restarted, and needs to be notified by each already-running domain about what interfaces they are using.
networkReleaseActualDevice - decrements the usage count of the allocated physical device, and frees the virDomainActualNetDef to avoid later accidentally using the device.
bridge_driver.[hc] - the new APIs
qemu_(command|driver|hotplug|process).c - add calls to the above APIs in the appropriate places.
tests/Makefile.am - need to include libvirt_driver_network.la whenever libvirt_driver_qemu.la is linked, to avoid unreferenced symbols (in functions that are never called by the test programs...) --- src/network/bridge_driver.c | 398 +++++++++++++++++++++++++++++++++++++++++++ src/network/bridge_driver.h | 6 + src/qemu/qemu_command.c | 11 ++ src/qemu/qemu_hotplug.c | 41 +++-- src/qemu/qemu_process.c | 26 +++- tests/Makefile.am | 12 +- 6 files changed, 476 insertions(+), 18 deletions(-) + + /* Find the most specific virtportprofile and copy it */ + if (iface->data.network.virtPortProfile) { + virtport = iface->data.network.virtPortProfile; + } else { + virPortGroupDefPtr portgroup + = virPortGroupFindByName(netdef, iface->data.network.portgroup); + if (portgroup) + virtport = portgroup->virtPortProfile; Nit: Indentation looks interesting there - the line starting with = has one less space. Perhaps something to do with how your preferred editor splits assignments. My editor uses 4 spaces instead of 3 in that instance.
It defaults to a different style, and I sometimes forget to switch.
+int +networkNotifyActualDevice(virDomainNetDefPtr iface) +{ + + /* PASSTHROUGH mode, and PRIVATE Mode + 802.1Qbh both require + * exclusive access to a device, so current usageCount must be + * 0 in those cases. + */ + if ((dev->usageCount> 0)&& + ((netdef->forwardType == VIR_NETWORK_FORWARD_PASSTHROUGH) || + ((netdef->forwardType == VIR_NETWORK_FORWARD_PRIVATE)&& + iface->data.network.actual->data.direct.virtPortProfile&& + (iface->data.network.actual->data.direct.virtPortProfile->virtPortType + == VIR_VIRTUALPORT_8021QBH)))) { + networkReportError(VIR_ERR_INTERNAL_ERROR, + _("network '%s' claims dev='%s' is already in use by a different domain"), Do we have a data race here?
Suppose that libvirtd is restarted while one VM is already using device 0. Is there any chance that if I'm fast enough, I can cause the creation of a second domain, and that second domain will go to pick from the interface pool before the notification pass of the libvirtd restart has completed, and mistakenly claim device 0?
That is, I think we need to somehow guarantee (if we don't already) that no new domains can be created until after all existing domains have been reloaded on a libvirtd restart.
I was concerned about this at first too. Here's the call chain down to this function (it's only called from one place): networkNotifyActualDevice qemuProcessNotifyNets qemuProcessReconnect qemuProcessReconnectAll qemudStartup <- aka the "initialize" function of the qemu state driver. At the top of qemudStartup, the lock for the driver is initialized, then locked. After this, a ton of stuff is initialized, including calling qemuProcessReconnectAll(), then the worker thread pool is created, and finally the driver lock is released. So as far as I understand it, it's not possible for a new domain to be created until after this is all finished.
participants (3)
-
Daniel P. Berrange
-
Eric Blake
-
Laine Stump