This patch resolves:
https://bugzilla.redhat.com/show_bug.cgi?id=1035336
The basic problem is that during a network update, the required
iptables rules sometimes change, and this was being handled by simply
removing and re-adding the rules. However, the removal of the old
rules was done based on the *new* state of the network, which would
mean that some of the rules would not match those currently in the
system, so the old rules wouldn't be removed.
This patch removes the old rules prior to updating the network
definition then adds the new rules as soon as the definition is
updated. Note that this could lead to a stray packet or two during the
interim, but that was already a problem before (the period of limbo is
now just slightly longer).
While moving the location for the rules, I added a few more sections
that should result in the iptables rules being redone:
DHCP_RANGE and DHCP_HOST - these are needed because adding/removing a dhcp
host entry could lead to the dhcp service being started/stopped, which
would require that the mangle rule that fixes up dhcp response
checksums sould need to be added/removed, and this wasn't being done.
Most of our code base uses space after comma but not before;
fix the remaining uses before adding a syntax check.
* src/network/bridge_driver.c: Consistently use commas.
* src/node_device/node_device_hal.c: Likewise.
* src/node_device/node_device_udev.c: Likewise.
* src/storage/storage_backend_rbd.c: Likewise.
Signed-off-by: Eric Blake <eblake@redhat.com>
This resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1020135
If networkAllocateActualDevice() had failed due to a pool of hostdev
or direct devices being depleted, the calling function could still
call networkReleaseActualDevice() as part of its cleanup, and that
function would then unconditionally decrement the connections count
for the network, even though it hadn't been incremented (due to
failure of allocate). This *was* necessary because the .actual member
of the netdef was allocated with a "lazy" algorithm, only being
created if there was a need to store data there (e.g. if a device was
allocated from a pool, or bandwidth was allocated for the device), so
there was no simple way for networkReleaseActualDevice() to tell if
something really had been allocated (i.e. if "connections++" had been
executed).
This patch changes networkAllocateDevice() to *always* allocate an
actual device for any netdef of type='network', even if it isn't
needed for any other reason. This has no ill effects anywhere else in
the code (except for using a small amount of memory), and
networkReleaseActualDevice() can then determine if there was a
previous successful allocate by checking for .actual != NULL (if not,
it skips the "connections--").
Currently, we ignore whether dnsmasqCapsRefresh succeeds or fails. We
shouldn't do that as we may generate wrong dnsmasq command line (what
is done just a few lines below).
Signed-off-by: Hongwei Bi <hwbi2008@gmail.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Packets sent by guests on virbrN, *or* by dnsmasq on the same, to
- 255.255.255.255/32 (netmask-independent local network broadcast
address), or to
- 224.0.0.0/24 (local subnetwork multicast range)
are never forwarded, hence it is not necessary to masquerade them.
In fact we must not masquerade them: translating their source addresses or
source ports (where applicable) may confuse receivers on virbrN.
One example is the DHCP client in OVMF (= UEFI firmware for virtual
machines):
http://thread.gmane.org/gmane.comp.bios.tianocore.devel/1506/focus=2640
It expects DHCP replies to arrive from remote source port 67. Even though
dnsmasq conforms to that, the destination address (255.255.255.255) and
the source address (eg. 192.168.122.1) in the reply allow the UDP
masquerading rule to match, which rewrites the source port to or above
1024. This prevents the DHCP client in OVMF from accepting the packet.
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=709418
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Useful to set custom forwarders instead of using the contents of
/etc/resolv.conf. It helps me to setup dnsmasq as local nameserver to
resolve VM domain names from domain 0, when domain option is used.
Signed-off-by: Diego Woitasen <diego.woitasen@vhgroup.net>
Signed-off-by: Eric Blake <eblake@redhat.com>
Similarly to qemu_driver.c, we can join often repeating code of looking
up network into one function: networkObjFromNetwork.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
This resolves the issue that prompted the filing of
https://bugzilla.redhat.com/show_bug.cgi?id=928638
(although the request there is for something much larger and more
general than this patch).
commit f3868259ca disabled the
forwarding to upstream DNS servers of unresolved DNS requests for
names that had no domain, but were just simple host names (no "."
character anywhere in the name). While this behavior is frowned upon
by DNS root servers (that's why it was changed in libvirt), it is
convenient in some cases, and since dnsmasq can be configured to allow
it, it must not be strictly forbidden.
This patch restores the old behavior, but since it is usually
undesirable, restoring it requires specification of a new option in
the network config. Adding the attribute "forwardPlainNames='yes'" to
the <dns> elemnt does the trick - when that attribute is added to a
network config, any simple hostnames that can't be resolved by the
network's dnsmasq instance will be forwarded to the DNS servers listed
in the host's /etc/resolv.conf for an attempt at resolution (just as
any FQDN would be forwarded).
When that attribute *isn't* specified, unresolved simple names will
*not* be forwarded to the upstream DNS server - this is the default
behavior.
* Move platform specific things (e.g. firewalling and route
collision checks) into bridge_driver_platform
* Create two platform specific implementations:
- bridge_driver_linux: Linux implementation using iptables,
it's actually the code moved from bridge_driver.c
- bridge_driver_nop: dumb implementation that does nothing
Signed-off-by: Eric Blake <eblake@redhat.com>
This is another cleanup before extracting platform-specific
parts from bridge_driver.
Rename struct network_driver to _virNetworkDriverState and
add appropriate typedefs: virNetworkDriverState and
virNetworkDriverStatePtr.
This will help us to avoid potential problems when moving
this struct to the .h file.
Convert the type of loop iterators named 'i', 'j', k',
'ii', 'jj', 'kk', to be 'size_t' instead of 'int' or
'unsigned int', also santizing 'ii', 'jj', 'kk' to use
the normal 'i', 'j', 'k' naming
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
iptablesContext holds only 4 pairs of iptables
(table, chain) and there's no need to pass
it around.
This is a first step towards separating bridge_driver.c
in platform-specific parts.
If networkUnplugBandwidth is called on a network which has
no bandwidth defined, print a warning instead of crashing.
This can happen when destroying a domain with bandwidth if
bandwidth was removed from the network after the domain was
started.
https://bugzilla.redhat.com/show_bug.cgi?id=975359
Although SRIOV network cards support setting a vlan tag on their
virtual functions, and although setting this vlan tag via a <vlan>
element in a domain's <interface> works, setting a vlan tag for these
devices in a <network> definition, or in a network <portgroup>
definition is also supposed to work (and the comment that validates
<vlan> usage even says that!). However, the check to allow it only
checked for an openvswitch network, so attempts to add <vlan> to a
network of type='hostdev' would fail.
This fixes the problem reported in:
https://bugzilla.redhat.com/show_bug.cgi?id=972690
When checking for a collision of a new libvirt network's subnet with
any existing routes, we read all of /proc/net/route into memory, then
parse all the entries. The function that we use to read this file
requires a "maximum length" parameter, which had previously been set
to 64*1024. As each line in /proc/net/route is 128 bytes, this would
allow for a maximum of 512 entries in the routing table.
This patch increases that number to 128 * 100000, which allows for
100,000 routing table entries. This means that it's possible that 12MB
would be allocated, but that would only happen if there really were
100,000 route table entries on the system, it's only held for a very
short time.
Since there is no method of specifying and unlimited max (and that
would create a potential denial of service anyway) hopefully this
limit is large enough to accomodate everyone.
In order to learn libvirt multiqueue several things must be done:
1) The '/dev/net/tun' device needs to be opened multiple times with
IFF_MULTI_QUEUE flag passed to ioctl(fd, TUNSETIFF, &ifr);
2) Similarly, '/dev/vhost-net' must be opened as many times as in 1)
in order to keep 1:1 ratio recommended by qemu and kernel folks.
3) The command line construction code needs to switch from 'fd=X' to
'fds=X:Y:...:Z' and from 'vhostfd=X' to 'vhostfds=X:Y:...:Z'.
4) The monitor handling code needs to learn to pass multiple FDs.
network: static route support for <network>
This patch adds the <route> subelement of <network> to define a static
route. the address and prefix (or netmask) attribute identify the
destination network, and the gateway attribute specifies the next hop
address (which must be directly reachable from the containing
<network>) which is to receive the packets destined for
"address/(prefix|netmask)".
These attributes are translated into an "ip route add" command that is
executed when the network is started. The command used is of the
following form:
ip route add <address>/<prefix> via <gateway> \
dev <virbr-bridge> proto static metric <metric>
Tests are done to validate that the input data are correct. For
example, for a static route ip definition, the address must be a
network address and not a host address. Additional checks are added
to ensure that the specified gateway is directly reachable via this
network (i.e. that the gateway IP address is in the same subnet as one
of the IP's defined for the network).
prefix='0' is supported for both family='ipv4' address='0.0.0.0'
netmask='0.0.0.0' or prefix='0', and for family='ipv6' address='::',
prefix=0', although care should be taken to not override a desired
system default route.
Anytime an attempt is made to define a static route which *exactly*
duplicates an existing static route (for example, address=::,
prefix=0, metric=1), the following error message will be sent to
syslog:
RTNETLINK answers: File exists
This can be overridden by decreasing the metric value for the route
that should be preferred, or increasing the metric for the route that
shouldn't be preferred (and is thus in place only in anticipation that
the preferred route may be removed in the future). Caution should be
used when manipulating route metrics, especially for a default route.
Note: The use of the command-line interface should be replaced by
direct use of libnl so that error conditions can be handled better. But,
that is being left as an exercise for another day.
Signed-off-by: Gene Czarcinski <gene@czarc.net>
Signed-off-by: Laine Stump <laine@laine.org>
This should resolve https://bugzilla.redhat.com/show_bug.cgi?id=958907
Recent new addition of code to read/write active network state to the
NETWORK_STATE_DIR in the network driver broke startup for
qemu:///session. The network driver had several state file paths
hardcoded to /var, which could never possibly work in session mode.
This patch modifies *all* state files to use a variable string that is
set differently according to whether or not we're running
privileged. (It turns out that logDir was never used, so it's been
completely eliminated.)
There are very definitely other problems preventing dnsmasq and radvd
from running in non-privileged mode, but it's more consistent to have
the directories used by them be determined in the same fashion.
NB: I've noted before that the network driver is storing its state
(including dnsmasq and radvd state) in /var/lib, while qemu stores its
state in /var/run. It would probably have been better if the two
matched, but it's been this way for a long time, and changing it would
break running installations during an upgrade, so it's best to just
leave it as it is.
The call to virReportError conditionally switched between
two format strings, with different numbers of placeholders.
This meant the format string with no placeholders was not
protected by a "%s".
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
The source code base needs to be adapted as well. Some files
include virutil.h just for the string related functions (here,
the include is substituted to match the new file), some include
virutil.h without any need (here, the include is removed), and
some require both.
On the off-chance that creation of persistent configuration file would
fail when defining a network that is already started as transient, the
code would remove the transient data structure and thus the network.
This patch changes the code so that in such case, the network is again
marked as transient and left behind.
This isn't strictly speaking a bugfix, but I realized I'd gotten a bit
too verbose when I chose the names for
VIR_DOMAIN_HOSTDEV_PCI_BACKEND_TYPE_*. This shortens them all a bit.
I remembered to document this bit, but somehow forgot to implement it.
This adds <driver name='kvm|vfio'/> as a subelement to the <forward>
element of a network (this puts it parallel to the match between
mode='hostdev' attribute in a network and type='hostdev' in an
<interface>).
Since it's already documented, only the parser, formatter, backend
driver recognition (it just translates/moves the flag into the
<interface> at the appropriate time), and a test case were needed.
(I used a separate enum for the values both because the original is
defined in domain_conf.h, which is unavailable from network_conf.h,
and because in the future it's possible that we may want to support
other non-hostdev oriented driver names in the network parser; this
makes sure that one can be expanded without the other).
There will soon be other items related to pci hostdevs that need to be
in the same part of the hostdevsubsys union as the pci address (which
is currently a single member called "pci". This patch replaces the
single member named pci with a struct named pci that contains a single
member named "addr".
Ensure that all drivers implementing public APIs use a
naming convention for their implementation that matches
the public API name.
eg for the public API virDomainCreate make sure QEMU
uses qemuDomainCreate and not qemuDomainStart
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
It will simplify later work if the sub-drivers have dedicated
APIs / field names. ie virNetworkDriver should have
virDrvNetworkOpen and virDrvNetworkClose methods
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Ensure that the driver struct field names match the public
API names. For an API virXXXX we must have a driver struct
field xXXXX. ie strip the leading 'vir' and lowercase any
leading uppercase letters.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Until now tranisent networks weren't really useful as libvirtd wasn't
able to remember them across restarts. This patch adds support for
loading status files of transient networks (that already were generated)
so that the status isn't lost.
This patch chops up virNetworkObjUpdateParseFile and turns it into
virNetworkLoadState and a few friends that will help us to load status
XMLs and refactors the functions that are loading the configs to use
them.
Detected by a simple Shell script:
for i in $(git ls-files -- '*.[ch]'); do
awk 'BEGIN {
fail=0
}
/# *include.*\.h/{
match($0, /["<][^">]*[">]/)
arr[substr($0, RSTART+1, RLENGTH-2)]++
}
END {
for (key in arr) {
if (arr[key] > 1) {
fail=1
printf("%d %s\n", arr[key], key)
}
}
if (fail == 1)
exit 1
}' $i
if test $? != 0; then
echo "Duplicate header(s) in $i"
fi
done;
A later patch will add the syntax-check to avoid duplicate
headers.
By current implementation, network inbound is required in order
to use 'floor' for guaranteeing minimal throughput. This is so,
because we want user to tell us the maximal throughput of the
network instead of finding out ourselves (and detect bogus values
in case of virtual interfaces). However, we are nowadays
requiring this only on documentation level. So if user starts a
domain with 'floor' set on one its interfaces, we silently ignore
the setting. We should error out instead.
This reverts commit 383ebc4694.
We decided the xml for this feature needed more thought to make sure
we are doing it the best way, in particular wrt option values that
have multiple items.
Originally, only a host name was used to associate a
DHCPv6 request with a specific IPv6 address. Further testing
demonstrates that this is an unreliable method and, instead,
a client-id or DUID needs to be used. According to DHCPv6
standards, this id can be a duid-LLT, duid-LL, or duid-UUID
even though dnsmasq will accept almost any text string.
Although validity checking of a specified string makes sure it is
hexadecimal notation with bytes separated by colons, there is no
rigorous check to make sure it meets the standard.
Documentation and schemas have been updated.
Signed-off-by: Gene Czarcinski <gene@czarc.net>
Signed-off-by: Laine Stump <laine@laine.org>
This patch adds support for a new <option>-Tag in the <dhcp> block of
network configs, based on a subset of the fifth proposal by Laine
Stump in the mailing list discussion at
https://www.redhat.com/archives/libvir-list/2012-November/msg01054.html.
Any such defined option will result in a dhcp-option=<number>,"<value>"
statement in the generated dnsmasq configuration file.
Currently, DHCP options can be specified by number only and there is
no whitelisting or blacklisting of option numbers, which should
probably be added.
Signed-off-by: Pieter Hollants <pieter@hollants.com>
Signed-off-by: Laine Stump <laine@laine.org>
We pass over the address/port start/end values many times so we put
them in structs.
Signed-off-by: Natanael Copa <ncopa@alpinelinux.org>
Signed-off-by: Laine Stump <laine@laine.org>
Let users set the port range to be used for forward mode NAT:
...
<forward mode='nat'>
<nat>
<port start='1024' end='65535'/>
</nat>
</forward>
...
Signed-off-by: Natanael Copa <ncopa@alpinelinux.org>
Signed-off-by: Laine Stump <laine@laine.org>
Support setting which public ip to use for NAT via attribute
address in subelement <nat> in <forward>:
...
<forward mode='nat'>
<address start='1.2.3.4' end='1.2.3.10'/>
</forward>
...
This will construct an iptables line using:
'-j SNAT --to-source <start>-<end>'
instead of:
'-j MASQUERADE'
Signed-off-by: Natanael Copa <ncopa@alpinelinux.org>
Signed-off-by: Laine Stump <laine@laine.org>
The conditional setting of cmdout in networkBuildDhcpDaemonCommandLine()
caused Coverity to complain that 'cmd' could be leaked if !cmdout. Since
the function is local and only called with cmdout being passed those checks
have been removed.
The fetch of 'ipdef' in networkRefreshDhcpDaemon() when the loop to fill
in ipv4def fails to find an ipv4 address with dhcp defined. The filled in
ipdef value was not used. Code was made unnecessary with commit it 2d5cd1.
The bandwidth plug and unplug functions were assuming that an
interface's bandwidth setting was always specified directly in the
domain's <interface> definition, but that's not necessarily true - it
could have been obtained from a <portgroup> definition in the network
definition. This patch fixes those functions to use
virDomainNetGetActualBandwidth(), which gets the bandwidth pointer
from iface->data.network.actual if it exists, otherwise returns
iface->bandwidth.
Remove extraneous check for 'netdef' when dereferencing for vlan.nTags.
Prior code would already check if netdef was NULL.
Coverity complained about a path where the 'vlan' was potentially valid,
but a prior checks may not have allocated 'iface->data.network.actual',
so like other paths it needs to be allocated on the fly.
Move the copying of vlan up earlier in networkAllocateActualDevice, so
that actual.type gets properly set.
Since the first assignment to vlan is redundant except in the case of
jumping immediately to validate from the start of the function,
eliminate its initial setting at the top of the function in favor of
calling the helper function virDomainNetGetActualVlan() (which doesn't
depend on the local vlan pointer being initialized) down at validate:
Signed-off-by: Laine Stump <laine@redhat.com>
If addition of rules in networkAddIptablesRules() failed the real error
was masked by error reported when trying to clean up the remaining
rules.
With this patch the original error message is saved and set back after
the removal is complete.
Commit 0211fd6e04 introduced regression
where newly defined networks were not made persistent.
This patch makes the network persistent on each successful definition.
This is yet another refinement to the fix for CVE-2012-3411:
https://bugzilla.redhat.com/show_bug.cgi?id=833033
It turns out that it would be very intrusive to correctly backport the
entire --bind-dynamic option to older dnsmasq versions
(e.g. dnsmasq-2.48 that is used on RHEL6.x and CentOS 6.x), but very
simple to patch those versions to just use SO_BINDTODEVICE on all
their listening sockets (SO_BINDTODEVICE also has the desired effect
of permitting only traffic that was received on the interface(s) where
dnsmasq was set to listen.)
This patch modifies the dnsmasq capabilities detection to detect the
string:
--bind-interfaces with SO_BINDTODEVICE
in the output of "dnsmasq --version", and in that case realize that
using the old --bind-interfaces option is just as safe as
--bind-dynamic (and therefore *not* forbid creation of networks that
use public IP address ranges).
If -bind-dynamic is available, it is still preferred over
--bind-interfaces.
Note that this patch does no harm in upstream, or in any distro's
downstream if it happens to end up there, but builds for distros that
have a new enough dnsmasq to support --bind-dynamic do *NOT* need to
specifically backport this patch; it's only required for distro
releases that have dnsmasq too old to have --bind-dynamic (and those
distros will need to add the SO_BINDTODEVICE patch to dnsmasq,
*including the extra string in the --version output*, as well.
Somehow I managed to push the changes to this file with improper
indentation. This patch just re-indents, reformats the comment lines,
and re-groups a couple of multi-line strings so that they fit within
80 columns. The resulting binary should be identical.
A forgotten "!" in recently-modified code at the top of
networkRefreshDaemon() meant an improper early return, which led to 1)
dnsmasq config files not being updated from the newly modified config,
and 2) dnsmasq not being sent a SIGHUP so that it could learn about
the changes to the config.
virNetworkDefGetIpByIndex() returns NULL if there are no ip objects of
the requested type, and if there are no IP elements, then dnsmasq
shouldn't be running, so we can return early. Otherwise we should
rewrite the config files and send a SIGHUP.
This patch resolves the problem reported in:
https://bugzilla.redhat.com/show_bug.cgi?id=886663
The source of the problem was the fix for CVE 2011-3411:
https://bugzilla.redhat.com/show_bug.cgi?id=833033
which was originally committed upstream in commit
753ff83a50. That commit improperly
removed the "--except-interface lo" from dnsmasq commandlines when
--bind-dynamic was used (based on comments in the latter bug).
It turns out that the problem reported in the CVE could be eliminated
without removing "--except-interface lo", and removing it actually
caused each instance of dnsmasq to listen on localhost on port 53,
which created a new problem:
If another instance of dnsmasq using "bind-interfaces" (instead of
"bind-dynamic") had already been started (or if another instance
started later used "bind-dynamic"), this wouldn't have any immediately
visible ill effects, but if you tried to start another dnsmasq
instance using "bind-interfaces" *after* starting any libvirt
networks, the new dnsmasq would fail to start, because there was
already another process listening on port 53.
(Subsequent to the CVE fix, another patch changed the network driver
to put dnsmasq options in a conf file rather than directly on the
dnsmasq commandline, but preserved the same options.)
This patch changes the network driver to *always* add
"except-interface=lo" to dnsmasq conf files, regardless of whether we use
bind-dynamic or bind-interfaces. This way no libvirt dnsmasq instances
are listening on localhost (and the CVE is still fixed).
The actual code change is miniscule, but must be propogated through all
of the test files as well.
I noticed that /var/lib/libvirt/dnsmasq/*.conf used the wrong word;
it was intended to match the wording in src/util/xml.c.
* src/network/bridge_driver.c (networkDnsmasqConfContents): Fix typo.
* tests/networkxml2confdata/*.conf: Update accordingly.
Currently, we are only keeping a inactive XML configuration
in status dir. This is no longer enough as we need to keep
this class_id attribute so we don't overwrite old entries
when the daemon restarts. However, since there has already
been release which has just <network/> as root element,
and we want to keep things compatible, detect that loaded
status file is older one, and don't scream about it.
Network should be notified if we plug in or unplug an
interface, so it can perform some action, e.g. set/unset
network part of QoS. However, we are doing this in very
early stage, so iface->ifname isn't filled in yet. So
whenever we want to report an error, we must use a different
identifier, e.g. the MAC address.
These classes can borrow unused bandwidth. Basically,
only egress qdsics can have classes, therefore we can
do this kind of traffic shaping only on host's outgoing,
that is domain's incoming traffic.
This patch changes how parameters are passed to dnsmasq. Instead of
being on the command line, the parameters are put into a file (one
parameter per line) and a commandline --conf-file= specifies the
location of the file. The file is located in the same directory as
the leases file.
Putting the dnsmasq parameters into a configuration file
allows them to be examined and more easily understood than
examining the command lines displayed by "ps ax". This is
especially true when a number of networks have been started.
When the use of dnsmasq was originally done, the required command line
was simple, but it has gotten more complicated over time and will
likely become even more complicated in the future.
Note: The test conf files have all been renamed .conf instead of
.argv, and tests/networkxml2xmlargvdata was moved to
tests/networkxml2xmlconfdata.
The DHCPv6 support includes IPV6 dhcp-range and dhcp-host for one
IPv6 subnetwork on one interface. This support will only work
if dnsmasq version >= 2.64; otherwise an error occurs if
dhcp-range or dhcp-host is specified for an IPv6 address.
Essentially, this change provides the same DHCP support for IPv6
that has been available for IPv4.
With dnsmasq >= 2.64, support for the RA service is also now provided
by dnsmasq (radvd is no longer used/started). (Although at least one
version of dnsmasq prior to 2.64 "supported" IPv6 Router
Advertisement, there were bugs (fixed in 2.64) that rendered it
unusable.)
Documentation and the network schema has been updated
to reflect the new support.
The attributes of a <network> element's <forward> element were
previously stored directly in the virNetworkDef object, but
virNetworkUpdateForward() needs to operate on a <forward> in
isolation, so this patchs pulls out all those attributes into a
separate virNetworkForwardDef struct (and shortens their names
appropriately). This new object is contained in the virNetworkDef, not
pointed to by it, so there is no extra memory management.
This patch makes no functional changes, it only changes, e.g.,
"nForwardIfs" to "forward.nifs".
Since there is only a single virNetworkDNSDef for any virNetworkDef,
and it's trivial to determine whether or not it contains any real
data, it's much simpler (and fits more uniformly with the parse
function calling sequence of the parsers for many other objects that
are subordinates of virNetworkDef) if virNetworkDef *contains* an
virNetworkDNSDef rather than pointing to one.
Since it is now just a part of another object rather than its own
object, it no longer makes sense to have a *Free() function, so that
is changed to a *Clear() function.
More importantly though, ParseXML and Clear functions are needed for
the individual items contained in a virNetworkDNSDef (srv, txt, and
host records), but none of them have a *Clear(), and only two of the
three had *ParseXML() functions (both of which used a non-uniform
arglist). Those problems are cleared up by this patch - it splits the
higher-level Clear function into separate functions for each of the
three, creates a parse for txt records, and cleans up the srv and host
parsers, so we now have all the utility functions necessary to
implement virNetworkDefUpdateDNS(Host|Srv|Txt).
This shortens the name of the structs for srv and txt, and their
instances in virNetworkDNSDef, to be more compact and uniform with the
naming of the dns host array. It also changes the type of ntxts, etc
from unsigned int to size_t, so that they can be used directly as args
to VIR_*_ELEMENT.
This resolves: https://bugzilla.redhat.com/show_bug.cgi?id=767057
It was possible to define a network with <forward mode='bridge'> that
had both a bridge device and a forward device defined. These two are
mutually exclusive by definition (if you are using a bridge device,
then this is a host bridge, and if you have a forward dev defined,
this is using macvtap). It was also possible to put <ip>, <dns>, and
<domain> elements in this definition, although those aren't supported
by the current driver (although it's conceivable that some other
driver might support that).
The items that are invalid by definition, are now checked in the XML
parser (since they will definitely *always* be wrong), and the others
are checked in networkValidate() in the network driver (since, as
mentioned, it's possible that some other network driver, or even this
one, could some day support setting those).
This patch adds the capability for virtual guests to do IPv6
communication via a virtual network interface with no IPv6 (gateway)
addresses specified. This capability has always been enabled by
default for IPv4, but disabled for IPv6 for security concerns, and
because it requires the ip6tables command to be operational (which
isn't the case on a system with the ipv6 module completely disabled).
This patch adds a new attribute "ipv6" at the toplevel of a <network>
object. If ipv6='yes', the extra ip6tables rules required to permite
inter-guest communications are added when the network is started. If
it is 'no', or not present, those rules will not be added; thus the
default behavior doesn't change, so there should be no compatibility
issues with any existing installations.
Note that virtual guests cannot communication with the virtualization
host via this interface, because the following kernel tunable has
been set:
net.ipv6.conf.<bridge_interface_name>.disable_ipv6 = 1
This assures that the bridge interface will not have an IPv6
link-local (fe80::) address.
To control this behavior so that it is not enabled by default, the parameter
ipv6='yes' on the <network> statement has been added.
Documentation related to this patch has been updated.
The network schema has also been updated.
Currently to deal with auto-shutdown libvirtd must periodically
poll all stateful drivers. Thus sucks because it requires
acquiring both the driver lock and locks on every single virtual
machine. Instead pass in a "inhibit" callback to virStateInitialize
which drivers can invoke whenever they want to inhibit shutdown
due to existance of active VMs.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
The only important state that should prevent libvirtd shutdown
is from running VMs. Networks, host devices, network filters
and storage pools are all long lived resources that have no
significant in-memory state. They should not block shutdown.
This bug resolves CVE-2012-3411, which is described in the following
bugzilla report:
https://bugzilla.redhat.com/show_bug.cgi?id=833033
The following report is specifically for libvirt on Fedora:
https://bugzilla.redhat.com/show_bug.cgi?id=874702
In short, a dnsmasq instance run with the intention of listening for
DHCP/DNS requests only on a libvirt virtual network (which is
constructed using a Linux host bridge) would also answer queries sent
from outside the virtualization host.
This patch takes advantage of a new dnsmasq option "--bind-dynamic",
which will cause the listening socket to be setup such that it will
only receive those requests that actually come in via the bridge
interface. In order for this behavior to actually occur, not only must
"--bind-interfaces" be replaced with "--bind-dynamic", but also all
"--listen-address" options must be replaced with a single
"--interface" option. Fully:
--bind-interfaces --except-interface lo --listen-address x.x.x.x ...
(with --listen-address possibly repeated) is replaced with:
--bind-dynamic --interface virbrX
Of course libvirt can't use this new option if the host's dnsmasq
doesn't have it, but we still want libvirt to function (because the
great majority of libvirt installations, which only have mode='nat'
networks using RFC1918 private address ranges (e.g. 192.168.122.0/24),
are immune to this vulnerability from anywhere beyond the local subnet
of the host), so we use the new dnsmasqCaps API to check if dnsmasq
supports the new option and, if not, we use the "old" option style
instead. In order to assure that this permissiveness doesn't lead to a
vulnerable system, we do check for non-private addresses in this case,
and refuse to start the network if both a) we are using the old-style
options, and b) the network has a publicly routable IP
address. Hopefully this will provide the proper balance of not being
disruptive to those not practically affected, and making sure that
those who *are* affected get their dnsmasq upgraded.
(--bind-dynamic was added to dnsmasq in upstream commit
54dd393f3938fc0c19088fbd319b95e37d81a2b0, which was included in
dnsmasq-2.63)
In order to optionally take advantage of new features in dnsmasq when
the host's version of dnsmasq supports them, but still be able to run
on hosts that don't support the new features, we need to be able to
detect the version of dnsmasq running on the host, and possibly
determine from the help output what options are in this dnsmasq.
This patch implements a greatly simplified version of the capabilities
code we already have for qemu. A dnsmasqCaps device can be created and
populated either from running a program on disk, reading a file with
the concatenated output of "dnsmasq --version; dnsmasq --help", or
examining a buffer in memory that contains the concatenated output of
those two commands. Simple functions to retrieve capabilities flags,
the version number, and the path of the binary are also included.
bridge_driver.c creates a single dnsmasqCaps object at driver startup,
and disposes of it at driver shutdown. Any time it must be used, the
dnsmasqCapsRefresh method is called - it checks the mtime of the
binary, and re-runs the checks if the binary has changed.
networkxml2argvtest.c creates 2 "artificial" dnsmasqCaps objects at
startup - one "restricted" (doesn't support --bind-dynamic) and one
"full" (does support --bind-dynamic). Some of the test cases use one
and some the other, to make sure both code pathes are tested.
The virStateInitialize method and several cgroups methods were
using an 'int privileged' parameter or similar for dual-state
values. These are better represented with the bool type.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
bridge_driver.h: silence gcc warnings:
statement with no effect [-Wunused-value]
unused variable 'net' [-Wunused-variable]
virdrivermoduletest.c: don't require network driver module
if it hasn't been built.
The libvirt coding standard is to use 'function(...args...)'
instead of 'function (...args...)'. A non-trivial number of
places did not follow this rule and are fixed in this patch.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
When assigning the new persistent definition for a transient network
(thus making it persistent) the network needs to be marked persistent
before actually atempting to assign the definition.
Until now, the network undefine API was able to undefine only inactive
networks. The restriction doesn't make sense any more so this patch
implements changing networks to transient.
When a transient network was created some of the checks weren't run on
the definition allowing to start invalid networks.
This patch splits out code to the network validation function and
re-uses that code when creating transient networks.
The network driver didn't care about config files when a network was
destroyed, just when it was undefined leaving behind files for transient
networks.
This patch splits out the cleanup code to a helper function that handles
the cleanup if the inactive network object is being removed and re-uses
this code when getting rid of inactive networks.
The hosts file was created in the network definition function. This
patch moves the place the file is being created to the point where
dnsmasq is being started.
Three FORWARD chain rules are added and two INPUT chain rules
are added when a network is started but only the FORWARD chain
rules are removed when the network is destroyed.
This was found during testing of the fix for:
https://bugzilla.redhat.com/show_bug.cgi?id=868483
networkValidate was supposed to check for the existence of multiple
portgroups and report an error if this was encountered. It did, but
there were two problems:
1) even though it logged an error, it still returned success, allowing
the operation to continue.
2) It could exit the portgroup checking loop early (or possibly not
even do it once) if a vlan tag was supplied in the base network config
or one of the portgroups.
This patch fixes networkValidate to return failure in addition to
logging the error, and also changes it to not exit the portgroup
checking loop early. The logic was a bit off in the checking for vlan
anyway, and it's intertwined with fixing the early loop exit, so I
fixed that as well. Now it correctly checks for combinations where a
<virtualport> is specified in the base network def and <vlan> is given
in a portgroup, as well as the opposite (<vlan> in base network def
and <virtualport> in portgroup), and ignores the case of a disallowed
vlan when using *no* portgroup if there is a default portgroup (since
in that case there is no way to not use any portgroup).
This resolves: https://bugzilla.redhat.com/show_bug.cgi?id=868483
virNetworkUpdate, virNetworkDefine, and virNetworkCreate all three
allow network definitions to contain multiple <portgroup> elements
with default='yes'. Only a single default portgroup should be allowed
for each network.
This patch updates networkValidate() (called by both
virNetworkCreate() and virNetworkDefine()) and
virNetworkDefUpdatePortGroup (called by virNetworkUpdate() to not
allow multiple default portgroups.
This fixes the problem reported in:
https://bugzilla.redhat.com/show_bug.cgi?id=868389
Previously, the dnsmasq hosts file (used for static dhcp entries, and
addnhosts file (used for additional dns host entries) were only
created/referenced on the dnsmasq commandline if there was something
to put in them at the time the network was started. Once we can update
a network definition while it's active (which is now possible with
virNetworkUpdate), this is no longer a valid strategy - if there were
0 dhcp static hosts (resulting in no reference to the hosts file on the
commandline), then one was later added, the commandline wouldn't have
linked dnsmasq up to the file, so even though we create it, dnsmasq
doesn't pay any attention.
The solution is to just always create these files and reference them
on the dnsmasq commandline (almost always, anyway). That way dnsmasq
can notice when a new entry is added at runtime (a SIGHUP is sent to
dnsmasq by virNetworkUdpate whenever a host entry is added or removed)
The exception to this is that the dhcp static hosts file isn't created
if there are no lease ranges *and* no static hosts. This is because in
this case dnsmasq won't be setup to listen for dhcp requests anyway -
in that case, if the count of dhcp hosts goes from 0 to 1, dnsmasq
will need to be restarted anyway (to get it listening on the dhcp
port). Likewise, if the dhcp hosts count goes from 1 to 0 (and there
are no dhcp ranges) we need to restart dnsmasq so that it will stop
listening on port 67. These special situations are handled in the
bridge driver's networkUpdate() by checking for ((bool)
nranges||nhosts) both before and after the update, and triggering a
dnsmasq restart if the before and after don't match.
https://bugzilla.redhat.com/show_bug.cgi?id=866364
pointed out a crash due to virNetworkObjAssignDef free'ing
network->newDef without NULLing it afterward. A fix for this is in
upstream commit b7e9202401. While the
NULLing of newDef was a legitimate fix, newDef should have already
been empty (NULL) anyway (as indicated in the comment that was deleted
by that commit).
The reason that newDef had a non-NULL value (i.e. the root cause) was
that networkStartNetwork() had failed after populating
network->newDef, but then neglected to free/NULL newDef in the
cleanup.
(A bit of background here: network->newDef should contain the
persistent config of a network when a network is active (and of course
only when it is persisten), and NULL at all other times. There is also
a network->def which should contain the persistent definition of the
network when it is inactive, and the current live state at all other
times. The idea is that you can make changes to network->newDef which
will take effect the next time the network is restarted, but won't
mess with the current state of the network (virDomainObj has a similar
pair of virDomainDefs that behave in the same fashion). Personally I
think there should be a network->live and network->config, and the
location of the persistent config should *always* be in
network->config, but that's for a later cleanup).
Since I love things to be symmetric, I created a new function called
virNetworkObjUnsetDefTransient(), which reverses the effects of
virNetworkObjSetDefTransient(). I don't really like the name of the
new function, but then I also didn't really like the name of the old
one either (it's just named that way to match a similar function in
the domain conf code).
This function really should have been taking virDevicePCIAddress*
instead of the inefficient virDevicePCIAddress (results in copying two
entire structs onto the stack rather than just two pointers), and
returning a bool true/false (not matching is not necessarily a
"failure", as a -1 return would imply, and also using "if
(!virDevicePCIAddressEqual(x, y))" to mean "if x == y" is just a bit
counterintuitive).
I hit this problem recently when trying to create a bridge with an IPv6
address on a 3.2 kernel: dnsmasq (and, further, radvd) would not bind to
the given address, waiting 20s and then giving up with -EADDRNOTAVAIL
(resp. exiting immediately with "error parsing or activating the config
file", without libvirt noticing it, BTW). This can be reproduced with (I
think) any kernel >= 2.6.39 and the following XML (to be used with
"virsh net-create"):
<network>
<name>test-bridge</name>
<bridge name='testbr0' />
<ip family='ipv6' address='fd00::1' prefix='64'>
</ip>
</network>
(it happens even when you have an IPv4, too)
The problem is that since commit [1] (which, ironically, was made to
“help IPv6 autoconfiguration”) the linux bridge code makes bridges
behave like “real” devices regarding carrier detection. This makes the
bridges created by libvirt, which are started without any up devices,
stay with the NO-CARRIER flag set, and thus prevents DAD (Duplicate
address detection) from happening, thus letting the IPv6 address flagged
as “tentative”. Such addresses cannot be bound to (see RFC 2462), so
dnsmasq fails binding to it (for radvd, it detects that "interface XXX
is not RUNNING", thus that "interface XXX does not exist, ignoring the
interface" (sic)). It seems that this behavior was enhanced somehow with
commit [2] by avoiding setting NO-CARRIER on empty bridges, but I
couldn't reproduce this behavior on my kernel. Anyway, with the “dummy
tap to set MAC address” trick, this wouldn't work.
To fix this, the idea is to get the bridge's attached device to be up so
that DAD can happen (deactivating DAD altogether is not a good idea, I
think). Currently, libvirt creates a dummy TAP device to set the MAC
address of the bridge, keeping it down. But even if we set this device
up, it is not RUNNING as soon as the tap file descriptor attached to it
is closed, thus still preventing DAD. So, we must modify the API a bit,
so that we can get the fd, keep the tap device persistent, run the
daemons, and close it after DAD has taken place. After that, the bridge
will be flagged NO-CARRIER again, but the daemons will be running, even
if not happy about the device's state (but we don't really care about
the bridge's daemons doing anything when no up interface is connected to
it).
Other solutions that I envisioned were:
* Keeping the *-nic interface up: this would waste an fd for each
bridge during all its life. May be acceptable, I don't really
know.
* Stop using the dummy tap trick, and set the MAC address directly
on the bridge: it is possible since quite some time it seems,
even if then there is the problem of the bridge not being
RUNNING when empty, contrary to what [2] says, so this will need
fixing (and this fix only happened in 3.1, so it wouldn't work
for 2.6.39)
* Using the --interface option of dnsmasq, but I saw somewhere
that it's not used by libvirt for backward compatibility. I am
not sure this would solve this problem, though, as I don't know
how dnsmasq binds itself to it with this option.
This is why this patch does what's described earlier.
This patch also makes radvd start even if the interface is
“missing” (i.e. it is not RUNNING), as it daemonizes before binding to
it, and thus sometimes does it after the interface has been brought down
by us (by closing the tap fd), and then originally stops. This also
makes it stop yelling about it in the logs when the interface is down at
a later time.
[1]
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commit;h=1faa4356a3bd89ea11fb92752d897cff3a20ec0e
[2]
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commit;h=b64b73d7d0c480f75684519c6134e79d50c1b341
The bridge driver implementation of virNetworkUpdate() removes and
re-adds iptables rules any time a network has an <ip>, <forward>, or
<forward>/<interface> element updated. There are some types of
networks that have those elements and yet have no iptables rules
associated with them, and unfortunately the functions that remove/add
iptables rules don't check the type of network before attempting to
remove/add the rules, sometimes leading to an erroneous failure of the
entire update operation.
Under normal circumstances I would refactor the lower level functions
to be more robust, but to avoid code churn as much as possible, I've
just added extra checks directly to networkUpdate().
https://www.gnu.org/licenses/gpl-howto.html recommends that
the 'If not, see <url>.' phrase be a separate sentence.
* tests/securityselinuxhelper.c: Remove doubled line.
* tests/securityselinuxtest.c: Likewise.
* globally: s/; If/. If/
Two changes are introduced in this patch:
- The first change removes ATTRIBUTE_RETURN_CHECK from
virNetDevBandwidthClear, because it was called with ignore_value
always, anyway. The function is used even when it's not necessary
to call it, just for cleanup purposes.
- The second change is added ignoring of the command's exit status,
since it may report an error even when run just as "to be sure we
clean up" function. No libvirt errors are suppresed by this.
A user on IRC had accidentally killed all of his libvirt-started
dnsmasq instances (due to a buggy dnsmasq service script in Fedora
16), and had hoped that libvirtd would notice this on restart and
reload all the dnsmasq daemons (as it does with iptables
rules). Unfortunately this was not the case - as long as the network
object had a pid registered for dnsmasq and/or radvd, it assumed that
the processes were running.
This patch takes advantage of the new utility functions in
bridge_driver.c to do a "refresh" of all radvd and dnsmasq processes
started by libvirt each time libvirtd is restarted - this function
attempts to do a SIGHUP of each existing process, and if that fails,
it restarts the process, rebuilding all the associated config files
and commandline parameters in the process. This normally has no
effect, but will be useful in solving the occasional "odd situation"
without needing to take the drastic step of destroying/re-starting the
network.
Call the network_conf function that modifies the live/persistent/both
config, then refresh/restart dnsmasq/radvd if necessary, and finally
save the config in the proper place(s).
This patch also needed to uncomment a few utility functions that were
added inside #if 0 in the previous commit (to avoid compiler errors
due to unreferenced static functions).
This patch splits the starting of dnsmasq and radvd into multiple
files, and adds new networkRefreshXX() and networkRestartXX()
functions for each. These new functions are currently commented out
because they won't be used until the next commit, and the compile options
require all static functions to be used.
networkRefreshXX() - rewrites any file-based config for dnsmasq/radvd,
and sends SIGHUP to the process to make it reread its config. If the
program isn't already running, it's just started.
networkRestartXX() - kills the given program, waits for it to exit
(see the comments in the function networkKillDaemon()), then calls
networkStartXX().
This commit is here mostly as a checkpoint to verify no change in
functional behavior after refactoring networkStartXX() functions to
fit in with these new functions.
These new functions are highly inspired by those in domain_conf.c (but
not identical), and are intended to make it simpler to update the
various combinations of live/persistent network configs.
The network driver wasn't previously as careful about the separation
between the live "status" in network->def and the persistent "config"
in network->newDef (or sometimes in network->def). This series
attempts to remedy some of that, but probably doesn't go all the way
(enough to get these functions working and enable continued work on
virNetworkUpdate though).
bridge_driver.c and test_driver.c were updated in a few places to take
advantage of the new functions and/or account for changes in argument
lists.
This patch removed the "--filterwin2k" dnsmasq command line
parameter which was unnecessary for domain specification,
possibly blocked some usage, and was command line clutter.
Gene Czarcinski <gene@czarc.net>
libvirt's network config documents that a bridge's STP "forward delay"
(called "delay" in the XML) should be specified in seconds, but
virNetDevBridgeSetSTPDelay() assumes that it is given a delay in
milliseconds (although the comment at the top of the function
incorrectly says "seconds".
This fixes the comment, and converts the delay to milliseconds before
calling virNetDevBridgeSetSTPDelay().
dnsmasq is forwarding a number of queries upstream that should not
be done. There still remains an MX query for a plain name with no
domain specified that will be forwarded is dnsmasq has --domain=xxx
--local=/xxx/ specified. This does not happen with no domain name
and --local=// ... not a libvirt problem.
BTW, thanks again to Claudio Bley!
* configure.ac, spec file: firewalld defaults to enabled if dbus is
available, otherwise is disabled. If --with_firewalld is explicitly
requested and dbus is not available, configure will fail.
* bridge_driver: add dbus filters to get the FirewallD1.Reloaded
signal and DBus.NameOwnerChanged on org.fedoraproject.FirewallD1.
When these are encountered, reload all the iptables reuls of all
libvirt's virtual networks (similar to what happens when libvirtd is
restarted).
* iptables, ebtables: use firewall-cmd's direct passthrough interface
when available, otherwise use iptables and ebtables commands. This
decision is made once the first time libvirt calls
iptables/ebtables, and that decision is maintained for the life of
libvirtd.
* Note that the nwfilter part of this patch was separated out into
another patch by Stefan in V2, so that needs to be revised and
re-reviewed as well.
================
All the configure.ac and specfile changes are unchanged from Thomas'
V3.
V3 re-ran "firewall-cmd --state" every time a new rule was added,
which was extremely inefficient. V4 uses VIR_ONCE_GLOBAL_INIT to set
up a one-time initialization function.
The VIR_ONCE_GLOBAL_INIT(x) macro references a static function called
vir(Ip|Eb)OnceInit(), which will then be called the first time that
the static function vir(Ip|Eb)TablesInitialize() is called (that
function is defined for you by the macro). This is
thread-safe, so there is no chance of any race.
IMPORTANT NOTE: I've left the VIR_DEBUG messages in these two init
functions (one for iptables, on for ebtables) as VIR_WARN so that I
don't have to turn on all the other debug message just to see
these. Even if this patch doesn't need any other modification, those
messages need to be changed to VIR_DEBUG before pushing.
This one-time initialization works well. However, I've encountered
problems with testing:
1) Whenever I have enabled the firewalld service, *all* attempts to
call firewall-cmd from within libvirtd end with firewall-cmd hanging
internally somewhere. This is *not* the case if firewall-cmd returns
non-0 in response to "firewall-cmd --state" (i.e. *that* command runs
and returns to libvirt successfully.)
2) If I start libvirtd while firewalld is stopped, then start
firewalld later, this triggers libvirtd to reload its iptables rules,
however it also spits out a *ton* of complaints about deletion failing
(I suppose because firewalld has nuked all of libvirt's rules). I
guess we need to suppress those messages (which is a more annoying
problem to fix than you might think, but that's another story).
3) I noticed a few times during this long line of errors that
firewalld made a complaint about "Resource Temporarily
unavailable. Having libvirtd access iptables commands directly at the
same time as firewalld is doing so is apparently problematic.
4) In general, I'm concerned about the "set it once and never change
it" method - if firewalld is disabled at libvirtd startup, causing
libvirtd to always use iptables/ebtables directly, this won't cause
*terrible* problems, but if libvirtd decides to use firewall-cmd and
firewalld is later disabled, libvirtd will not be able to recover.
This patch updates the network driver to properly utilize the new
attributes/elements that are now in virNetworkDef
Signed-off-by: Shradha Shah <sshah@solarflare.com>
Signed-off-by: Laine Stump <laine@laine.org>
The network pool should be able to keep track of both network device
names and PCI addresses, and return the appropriate one in the
actualDevice when networkAllocateActualDevice is called.
Signed-off-by: Shradha Shah <sshah@solarflare.com>
This patch introduces the new forward mode='hostdev' along with
attribute managed. Includes updates to the network RNG and new xml
parser/formatter code.
Signed-off-by: Shradha Shah <sshah@solarflare.com>
Existing code that creates a list of forwardIfs from a single PF
was moved to the new utility function networkCreateInterfacePool.
No functional change.
Signed-off-by: Shradha Shah <sshah@solarflare.com>
Add the ability to support VLAN tags for Open vSwitch virtual port
types. To accomplish this, modify virNetDevOpenvswitchAddPort and
virNetDevTapCreateInBridgePort to take a virNetDevVlanPtr
argument. When adding the port to the OVS bridge, setup either a
single VLAN or a trunk port based on the configuration from the
virNetDevVlanPtr.
Signed-off-by: Kyle Mestery <kmestery@cisco.com>
The network driver now looks for the vlan element in network and
portgroup objects, and logs an error at network define time if a vlan
is requested for a network type that doesn't support it. (Currently
vlan configuration is only supported for openvswitch networks, and
networks used to do hostdev assignment of SR-IOV VFs.)
At runtime, the three potential sources of vlan information are
examined in this order: interface, chosen portgroup, network, and the
first that is non-empty is used. Another check for valid network type
is made at this time, since the interface may have requested a vlan (a
legal thing to have in the interface config, since it's not known
until runtime if the chosen network will actually support it).
Since we must also check for domains requesting vlans for unsupported
connection types even if they are type='network', and since
networkAllocateActualDevice() is being called in exactly the correct
places, and has all of the necessary information to check, I slightly
modified the logic of that function so that interfaces that aren't
type='network' don't just return immediately. Instead, they also
perform all the same validation for supported features. Because of
this, it's not necessary to make this identical check in the other
three places that would normally require it: 1) qemu domain startup,
2) qemu device hotplug, 3) lxc domain startup.
This can be seen as a first step in consolidating network-related
functionality into the network driver, rather than having copies of
the same code spread around in multiple places; this will make it
easier to split the network parts off into a separate daemon, as we've
discussed recently.
Just as each physical device used by a network has a connections
counter, now each network has a connections counter which is
incremented once for each guest interface that connects using this
network.
The count is output in the live network XML, like this:
<network connections='20'>
...
</network>
It is read-only, and for informational purposes only - it isn't used
internally anywhere by libvirt.
A later patch will be adding a counter that will be
incremented/decremented each time an guest interface starts/stops
using a particular network. For this to work, all types of networks
need to go through a common return sequence rather than returning
early. To setup for this, a new success: label is added (when
necessary), a new error: label is added which does any cleanup
necessary only for error returns and then does goto cleanup, and early
returns are changed to goto error if it's a failure, or goto success
if it's successful. This way the intent of all the gotos is
unambiguous, and a successful return path never encounters the
"error:" label.
I want to include this count in the xml output of networks, but
calling it "connections" in the XML sounds better than "usageCount", and it
would be better if the name in the XML matched the variable name.
In a few places, usageCount was being initialized to 0, but this is
unnecessary, because VIR_ALLOC_N zero-fills everything anyway.
One of the original ideas behind allowing a <virtualport> in an
interface definition as well as in the <network> definition *and*one
or more <portgroup>s within the network, was that guest-specific
parameteres (like instanceid and interfaceid) could be given in the
interface's virtualport, and more general things (portid, managerid,
etc) could be given in the network and/or portgroup, with all the bits
brought together at guest startup time and combined into a single
virtualport to be used by the guest. This was somehow overlooked in
the implementation, though - it simply picks the "most specific"
virtualport, and uses the entire thing, with no attempt to merge in
details from the others.
This patch uses virNetDevVPortProfileMerge3() to combine the three
possible virtualports into one, then uses
virNetDevVPortProfileCheck*() to verify that the resulting virtualport
type is appropriate for the type of network, and that all the required
attributes for that type are present.
An example of usage is this: assuming a <network> definitions on host
ABC of:
<network>
<name>testA</name>
...
<virtualport type='openvswitch'/>
...
<portgroup name='engineering'>
<virtualport>
<parameters profileid='eng'/>
</virtualport>
</portgroup>
<portgroup name='sales'>
<virtualport>
<parameters profileid='sales'/>
</virtualport>
</portgroup>
</network>
and the same <network> on host DEF of:
<network>
<name>testA</name>
...
<virtualport type='802.1Qbg'>
<parameters typeid="1193047" typeidversion="2"/>
</virtualport>
...
<portgroup name='engineering'>
<virtualport>
<parameters managerid="11"/>
</virtualport>
</portgroup>
<portgroup name='sales'>
<virtualport>
<parameters managerid="55"/>
</virtualport>
</portgroup>
</network>
and a guest <interface> definition of:
<interface type='network'>
<source network='testA' portgroup='sales'/>
<virtualport>
<parameters instanceid="09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f"
interfaceid="09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f"\>
</virtualport>
...
</interface>
If the guest was started on host ABC, the <virtualport> used would be:
<virtualport type='openvswitch'>
<parameters interfaceid='09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f'
profileid='sales'/>
</virtualport>
but if that guest was started on host DEF, the <virtualport> would be:
<virtualport type='802.1Qbg'>
<parameters instanceid="09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f"
typeid="1193047" typeidversion="2"
managerid="55"/>
</virtualport>
Additionally, if none of the involved <virtualport>s had a specified type
(this includes cases where no virtualport is given at all),
virtPortProfile is now used by 4 different types of network devices
(NETWORK, BRIDGE, DIRECT, and HOSTDEV), and it's getting cumbersome to
replicate so much code in 4 different places just because each type
has the virtPortProfile in a slightly different place. This patch puts
a single virtPortProfile in a common place (outside the type-specific
union) in both virDomainNetDef and virDomainActualNetDef, and adjusts
the parse and format code (and the few other places where it is used)
accordingly.
Note that when a <virtualport> element is found, the parse functions
verify that the interface is of a type that supports one, otherwise an
error is generated (CONFIG_UNSUPPORTED in the case of <interface>, and
INTERNAL in the case of <actual>, since the contents of <actual> are
always generated by libvirt itself).
Per the FSF address could be changed from time to time, and GNU
recommends the following now: (http://www.gnu.org/licenses/gpl-howto.html)
You should have received a copy of the GNU General Public License
along with Foobar. If not, see <http://www.gnu.org/licenses/>.
This patch removes the explicit FSF address, and uses above instead
(of course, with inserting 'Lesser' before 'General').
Except a bunch of files for security driver, all others are changed
automatically, the copyright for securify files are not complete,
that's why to do it manually:
src/security/security_selinux.h
src/security/security_driver.h
src/security/security_selinux.c
src/security/security_apparmor.h
src/security/security_apparmor.c
src/security/security_driver.c
Update the linux bridge driver to use virReportError instead
of the networkReportError custom macro
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Introduce new members in the virMacAddr 'class'
- virMacAddrSet: set virMacAddr from a virMacAddr
- virMacAddrSetRaw: setting virMacAddr from raw 6 byte MAC address buffer
- virMacAddrGetRaw: writing virMacAddr into raw 6 byte MAC address buffer
- virMacAddrCmp: comparing two virMacAddr
- virMacAddrCmpRaw: comparing a virMacAddr with a raw 6 byte MAC address buffer
then replace raw MAC addresses by replacing
- 'unsigned char *' with virMacAddrPtr
- 'unsigned char ... [VIR_MAC_BUFLEN]' with virMacAddr
and introduce usage of above functions where necessary.
commit 52d064f42d added
VIR_NETWORK_XML_INACTIVE in order to allow suppressing the
auto-generated list of VFs in network definitions, and a --inactive
flag to virsh net-dumpxml to take advantage of the flag. However, it
missed out on two opportunities:
1) Use INACTIVE to get the current config of the network as it
exists on disk, rather than the currently active config.
2) Add INACTIVE to the flags used for the virsh net-edit command, so
that it won't include the forward-pool interfaces that were
autogenerated, and so that a re-edit of the network prior to
restarting it will show any other edits made since the last restart
of the network. (prior to this patch, if you edited a network a 2nd
time without restarting, all of the previous edits would magically
disappear).
In order to fit with the new #define-based generic edit function in
virsh.c, a new function vshNetworkGetXMLDesc() was added. This
function first tries to call virNetworkGetXMLDesc with the INACTIVE
flag added, then retries without if the first attempt fails (in the
manner expected when the server doesn't support it).
Remove the uid param from virGetUserConfigDirectory,
virGetUserCacheDirectory, virGetUserRuntimeDirectory,
and virGetUserDirectory
These functions were universally called with the
results of getuid() or geteuid(). To make it practical
to port to Win32, remove the uid parameter and hardcode
geteuid()
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
As defined in:
http://standards.freedesktop.org/basedir-spec/basedir-spec-latest.html
This offers a number of advantages:
* Allows sharing a home directory between different machines, or
sessions (eg. using NFS)
* Cleanly separates cache, runtime (eg. sockets), or app data from
user settings
* Supports performing smart or selective migration of settings
between different OS versions
* Supports reseting settings without breaking things
* Makes it possible to clear cache data to make room when the disk
is filling up
* Allows us to write a robust and efficient backup solution
* Allows an admin flexibility to change where data and settings are stored
* Dramatically reduces the complexity and incoherence of the
system for administrators
This patch will allow OpenFlow controllers to identify which interface
belongs to a particular VM by using the Domain UUID.
ovs-vsctl get Interface vnet0 external_ids
{attached-mac="52:54:00:8C:55:2C", iface-id="83ce45d6-3639-096e-ab3c-21f66a05f7fa", iface-status=active, vm-id="142a90a7-0acc-ab92-511c-586f12da8851"}
V2 changes:
Replaced vm-uuid with vm-id. There was a discussion in Open vSwitch
mailinglist that we should stick with the same DB key postfixes for the
sake of consistency (e.g iface-id, vm-id ...).
With an additional new bool added to determine whether or not to
discourage the use of the supplied MAC address by the bridge itself,
virNetDevTapCreateInBridgePort had three booleans (well, 2 bools and
an int used as a bool) in the arg list, which made it increasingly
difficult to follow what was going on. This patch combines those three
into a single flags arg, which not only shortens the arg list, but
makes it more self-documenting.
When a tap device for a domain is created and attached to a bridge,
the first byte of the tap device MAC address is set to 0xFE, while the
rest is set to match the MAC address that will be presented to the
guest as its network device MAC address. Setting this high value in
the tap's MAC address discourages the bridge from using the tap
device's MAC address as the bridge's own MAC address (Linux bridges
always take on the lowest numbered MAC address of all attached devices
as their own).
In one case within libvirt, a tap device is created and attached to
the bridge with the intent that its MAC address be taken on by the
bridge as its own (this is used to assure that the bridge has a fixed
MAC address to prevent network outages created by the bridge MAC
address "flapping" as guests are started and stopped). In this case,
the first byte of the mac address is *not* altered to 0xFE.
In the current code, callers to virNetDevTapCreateInBridgePort each
make the MAC address modification themselves before calling, which
leads to code duplication, and also prevents lower level functions
from knowing the real MAC address being used by the guest. The problem
here is that openvswitch bridges must be informed about this MAC
address, or they will be unable to pass traffic to/from the guest.
This patch centralizes the location of the MAC address "0xFE fixup"
into virNetDevTapCreateInBridgePort(), meaning 1) callers of this
function no longer need the extra strange bit of code, and 2)
bitNetDevTapCreateBridgeInPort itself now is called with the guest's
unaltered MAC address, and can pass it on, unmodified, to
virNetDevOpenvswitchAddPort.
There is no other behavioral change created by this patch.
This patch allows libvirt to add interfaces to already
existing Open vSwitch bridges. The following syntax in
domain XML file can be used:
<interface type='bridge'>
<mac address='52:54:00:d0:3f:f2'/>
<source bridge='ovsbr'/>
<virtualport type='openvswitch'>
<parameters interfaceid='921a80cd-e6de-5a2e-db9c-ab27f15a6e1d'/>
</virtualport>
<address type='pci' domain='0x0000' bus='0x00'
slot='0x03' function='0x0'/>
</interface>
or if libvirt should auto-generate the interfaceid use
following syntax:
<interface type='bridge'>
<mac address='52:54:00:d0:3f:f2'/>
<source bridge='ovsbr'/>
<virtualport type='openvswitch'>
</virtualport>
<address type='pci' domain='0x0000' bus='0x00'
slot='0x03' function='0x0'/>
</interface>
It is also possible to pass an optional profileid. To do that
use following syntax:
<interface type='bridge'>
<source bridge='ovsbr'/>
<mac address='00:55:1a:65:a2:8d'/>
<virtualport type='openvswitch'>
<parameters interfaceid='921a80cd-e6de-5a2e-db9c-ab27f15a6e1d'
profileid='test-profile'/>
</virtualport>
</interface>
To create Open vSwitch bridge install Open vSwitch and
run the following command:
ovs-vsctl add-br ovsbr
I slightly botched commit be9fb5a - I converted '--arg=value' to
'--arg value', which has no semantic change, but did trip up the
testsuite.
* src/network/bridge_driver.c (networkBuildDnsmasqArgv): Restore
expected output.
Detected by valgrind. Leaks introduced in commit 973af236.
* src/network/bridge_driver.c: fix memory leaks on failure and successful path.
* How to reproduce?
% make -C tests check TESTS=networkxml2argvtest
% cd tests && valgrind -v --leak-check=full ./networkxml2argvtest
* Actual result:
==2226== 3 bytes in 1 blocks are definitely lost in loss record 1 of 24
==2226== at 0x4A05FDE: malloc (vg_replace_malloc.c:236)
==2226== by 0x39CF0FEDE7: __vasprintf_chk (in /lib64/libc-2.12.so)
==2226== by 0x41DFF7: virVasprintf (stdio2.h:199)
==2226== by 0x41E0B7: virAsprintf (util.c:1695)
==2226== by 0x41A2D9: networkBuildDhcpDaemonCommandLine (bridge_driver.c:545)
==2226== by 0x4145C8: testCompareXMLToArgvHelper (networkxml2argvtest.c:47)
==2226== by 0x4156A1: virtTestRun (testutils.c:141)
==2226== by 0x414332: mymain (networkxml2argvtest.c:123)
==2226== by 0x414D97: virtTestMain (testutils.c:696)
==2226== by 0x39CF01ECDC: (below main) (in /lib64/libc-2.12.so)
==2226==
==2226== 3 bytes in 1 blocks are definitely lost in loss record 2 of 24
==2226== at 0x4A05FDE: malloc (vg_replace_malloc.c:236)
==2226== by 0x39CF0FEDE7: __vasprintf_chk (in /lib64/libc-2.12.so)
==2226== by 0x41DFF7: virVasprintf (stdio2.h:199)
==2226== by 0x41E0B7: virAsprintf (util.c:1695)
==2226== by 0x41A307: networkBuildDhcpDaemonCommandLine (bridge_driver.c:551)
==2226== by 0x4145C8: testCompareXMLToArgvHelper (networkxml2argvtest.c:47)
==2226== by 0x4156A1: virtTestRun (testutils.c:141)
==2226== by 0x414332: mymain (networkxml2argvtest.c:123)
==2226== by 0x414D97: virtTestMain (testutils.c:696)
==2226== by 0x39CF01ECDC: (below main) (in /lib64/libc-2.12.so)
==2226==
==2226== 5 bytes in 1 blocks are definitely lost in loss record 4 of 24
==2226== at 0x4A05FDE: malloc (vg_replace_malloc.c:236)
==2226== by 0x39CF0FEDE7: __vasprintf_chk (in /lib64/libc-2.12.so)
==2226== by 0x41DFF7: virVasprintf (stdio2.h:199)
==2226== by 0x41E0B7: virAsprintf (util.c:1695)
==2226== by 0x41A2AB: networkBuildDhcpDaemonCommandLine (bridge_driver.c:539)
==2226== by 0x4145C8: testCompareXMLToArgvHelper (networkxml2argvtest.c:47)
==2226== by 0x4156A1: virtTestRun (testutils.c:141)
==2226== by 0x414332: mymain (networkxml2argvtest.c:123)
==2226== by 0x414D97: virtTestMain (testutils.c:696)
==2226== by 0x39CF01ECDC: (below main) (in /lib64/libc-2.12.so)
==2226==
==2226== LEAK SUMMARY:
==2226== definitely lost: 11 bytes in 3 blocks
Signed-off-by: Alex Jia <ajia@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
If a system has 64 or more VF's, it is quite tedious to mention each VF
in the interface pool.
The following modification will implicitly create an interface pool from
the SR-IOV PF.
Hi,
this is the fifth version of my SRV record for DNSMasq patch rebased
for the current codebase to the bridge driver and libvirt XML file to
include support for the SRV records in the DNS. The syntax is based on
DNSMasq man page and tests for both xml2xml and xml2argv were added as
well. There are some things written a better way in comparison with
version 4, mainly there's no hack in tests/networkxml2argvtest.c and
also the xPath context is changed to use a simpler query using the
virXPathInt() function relative to the current node.
Also, the patch is also fixing the networkxml2argv test to pass both
checks, i.e. both unit tests and also syntax check.
Please review,
Michal
Signed-off-by: Michal Novotny <minovotn@redhat.com>
This patch addresses https://bugzilla.redhat.com/show_bug.cgi?id=760442
When a network has any forward type other than route, nat or none, the
network configuration should be done completely external to libvirt -
libvirt only uses these types to allow configuring guests in a manner
that isn't tied to a specific host (all the host-specific information,
in particular interface names, port profile data, and bandwidth
configuration is in the network definition, and the guest
configuration only references it).
Due to a bug in the bridge network driver, libvirt was adding iptables
rules for networks with forward type='bridge' etc. any time libvirtd
was restarted while one of these networks was active.
This patch eliminates that error by only "reloading" iptables rules if
forward type is route, nat, or none.
Only one IPv4 DHCP definition is supported. Originally the code checked
for a multiple definition and returned an error, but the new domain
definition was already added to networks. This patch moves the check
before the newly defined network is added to active networks.
*src/network/bridge_driver.c: networkDefine(): - move multiple IPv4
addresses check before
definition is used.
The virDomainNetGetActualBridgeName and virDomainNetGetActualDirectDev
methods both return strings that point to data in the virDomainDefPtr
struct, and should therefore not be freed. The return values should
thus be 'const char *' not 'char *'.
* src/conf/domain_conf.c, src/conf/domain_conf.h: Mark const
* src/network/bridge_driver.c: Update to use a const char *
Move the ifaceMacvtapLinkDump and ifaceGetNthParent functions
into virnetdevvportprofile.c since they are specific to that
code. This avoids polluting the headers with the Linux specific
netlink data types
* src/util/interface.c, src/util/interface.h: Move
ifaceMacvtapLinkDump and ifaceGetNthParent functions and delete
remaining file
* src/util/virnetdevvportprofile.c: Add ifaceMacvtapLinkDump
and ifaceGetNthParent functions
* src/network/bridge_driver.c, src/nwfilter/nwfilter_gentech_driver.c,
src/nwfilter/nwfilter_learnipaddr.c, src/util/virnetdevmacvlan.c:
Remove include of interface.h
To match up with the existing virNetDevSetIPv4Address, rename
ifaceGetIPAddress to virNetDevGetIPv4Address
* util/interface.h, util/interface.c: Rename API
* network/bridge_driver.c: Update for API rename
In preparation for code re-organization, rename the Macvtap
management APIs to have the following patterns
virNetDevMacVLanXXXXX - macvlan/macvtap interface management
virNetDevVPortProfileXXXX - virtual port profile management
* src/util/macvtap.c, src/util/macvtap.h: Rename APIs
* src/conf/domain_conf.c, src/network/bridge_driver.c,
src/qemu/qemu_command.c, src/qemu/qemu_command.h,
src/qemu/qemu_driver.c, src/qemu/qemu_hotplug.c,
src/qemu/qemu_migration.c, src/qemu/qemu_process.c,
src/qemu/qemu_process.h: Update for renamed APIs
The src/util/network.c file is a dumping ground for many different
APIs. Split it up into 5 pieces, along functional lines
- src/util/virnetdevbandwidth.c: virNetDevBandwidth type & helper APIs
- src/util/virnetdevvportprofile.c: virNetDevVPortProfile type & helper APIs
- src/util/virsocketaddr.c: virSocketAddr and APIs
- src/conf/netdev_bandwidth_conf.c: XML parsing / formatting
for virNetDevBandwidth
- src/conf/netdev_vport_profile_conf.c: XML parsing / formatting
for virNetDevVPortProfile
* src/util/network.c, src/util/network.h: Split into 5 pieces
* src/conf/netdev_bandwidth_conf.c, src/conf/netdev_bandwidth_conf.h,
src/conf/netdev_vport_profile_conf.c, src/conf/netdev_vport_profile_conf.h,
src/util/virnetdevbandwidth.c, src/util/virnetdevbandwidth.h,
src/util/virnetdevvportprofile.c, src/util/virnetdevvportprofile.h,
src/util/virsocketaddr.c, src/util/virsocketaddr.h: New pieces
* daemon/libvirtd.h, daemon/remote.c, src/conf/domain_conf.c,
src/conf/domain_conf.h, src/conf/network_conf.c,
src/conf/network_conf.h, src/conf/nwfilter_conf.h,
src/esx/esx_util.h, src/network/bridge_driver.c,
src/qemu/qemu_conf.c, src/rpc/virnetsocket.c,
src/rpc/virnetsocket.h, src/util/dnsmasq.h, src/util/interface.h,
src/util/iptables.h, src/util/macvtap.c, src/util/macvtap.h,
src/util/virnetdev.h, src/util/virnetdevtap.c,
tools/virsh.c: Update include files
Rename the virVirtualPortProfileParams struct to be
virNetDevVPortProfile, and rename the APIs to match
this prefix.
* src/util/network.c, src/util/network.h: Rename port profile
APIs
* src/conf/domain_conf.c, src/conf/domain_conf.h,
src/conf/network_conf.c, src/conf/network_conf.h,
src/network/bridge_driver.c, src/qemu/qemu_hotplug.c,
src/util/macvtap.c, src/util/macvtap.h: Update for
renamed APIs/structs
steps to reproduce:
1. having a network xml file(named default.xml) like this one:
<network>
<name>default</name>
<uuid>c5322c4c-81d0-4985-a363-ad6389780d89</uuid>
<bridge name="virbr0" />
<forward/>
<ip address="192.168.122.1" netmask="255.255.255.0">
<dhcp>
<range start="192.168.122.2" end="192.168.122.254" />
</dhcp>
</ip>
</network>
in /etc/libvirt/qemu/networks/, and mark it as autostart:
$ ls -l /etc/libvirt/qemu/networks/autostart
total 0
lrwxrwxrwx 1 root root 14 Oct 12 14:02 default.xml -> ../default.xml
2. start libvirtd and the device virbr0 is not automatically up.
The reason is that the function virNetDevExists is now returns 1 if
the device exists, comparing to the former one returns 0 if the device
exists. But with only this fix will cause a segmentation fault(the same
steps as above) that is fixed by the second chunk of code.
The socket address APIs in src/util/network.h either take the
form virSocketAddrXXX, virSocketXXX or virSocketXXXAddr.
Sanitize this so everything is virSocketAddrXXXX, and ensure
that the virSocketAddr parameter is always the first one.
* src/util/network.c, src/util/network.h: Santize socket
address API naming
* src/conf/domain_conf.c, src/conf/network_conf.c,
src/conf/nwfilter_conf.c, src/network/bridge_driver.c,
src/nwfilter/nwfilter_ebiptables_driver.c,
src/nwfilter/nwfilter_learnipaddr.c,
src/qemu/qemu_command.c, src/rpc/virnetsocket.c,
src/util/dnsmasq.c, src/util/iptables.c,
src/util/virnetdev.c, src/vbox/vbox_tmpl.c: Update for
API renaming
Following the renaming of the bridge management APIs, we can now
split the source file into 3 corresponding pieces
* src/util/virnetdev.c: APIs for any type of network interface
* src/util/virnetdevbridge.c: APIs for bridge interfaces
* src/util/virnetdevtap.c: APIs for TAP interfaces
* src/util/virnetdev.c, src/util/virnetdev.h,
src/util/virnetdevbridge.c, src/util/virnetdevbridge.h,
src/util/virnetdevtap.c, src/util/virnetdevtap.h: Copied
from bridge.{c,h}
* src/util/bridge.c, src/util/bridge.h: Split into 3 pieces
* src/lxc/lxc_driver.c, src/network/bridge_driver.c,
src/openvz/openvz_driver.c, src/qemu/qemu_command.c,
src/qemu/qemu_conf.h, src/uml/uml_conf.c, src/uml/uml_conf.h,
src/uml/uml_driver.c: Update #include directives
The existing brXXX APIs in src/util/bridge.h are renamed to
follow one of three different conventions
- virNetDevXXX - operations for any type of interface
- virNetDevBridgeXXX - operations for bridge interfaces
- virNetDevTapXXX - operations for tap interfaces
* src/util/bridge.h, src/util/bridge.c: Rename all APIs
* src/lxc/lxc_driver.c, src/network/bridge_driver.c,
src/qemu/qemu_command.c, src/uml/uml_conf.c,
src/uml/uml_driver.c: Update for API renaming
Currently every caller of the brXXX APIs has to store the returned
errno value and then raise an error message. This results in
inconsistent error messages across drivers, additional burden on
the callers and makes the error reporting inaccurate since it is
hard to distinguish different scenarios from 1 errno value.
* src/util/bridge.c: Raise errors instead of returning errnos
* src/lxc/lxc_driver.c, src/network/bridge_driver.c,
src/qemu/qemu_command.c, src/uml/uml_conf.c,
src/uml/uml_driver.c: Remove error reporting code
The bridge management APIs in src/util/bridge.c require a brControl
object to be passed around. This holds the file descriptor for the
control socket. This extra object complicates use of the API for
only a minor efficiency gain, which is in turn entirely offset by
the need to fork/exec the brctl command for STP configuration.
This patch removes the 'brControl' object entirely, instead opening
the control socket & closing it again within the scope of each method.
The parameter names for the APIs are also made to consistently use
'brname' for bridge device name, and 'ifname' for an interface
device name. Finally annotations are added for non-NULL parameters
and return check validation
* src/util/bridge.c, src/util/bridge.h: Remove brControl object
and update API parameter names & annotations.
* src/lxc/lxc_driver.c, src/network/bridge_driver.c,
src/uml/uml_conf.h, src/uml/uml_conf.c, src/uml/uml_driver.c,
src/qemu/qemu_command.c, src/qemu/qemu_conf.h,
src/qemu/qemu_driver.c: Remove reference to 'brControl' object
This patch is a fix for:
https://bugzilla.redhat.com/show_bug.cgi?id=743176
which was discovered by Dan Berrange while making bandwidth
configuration work for LXC guests.
Background: Although virtportprofile data from a network portgroup is
only applicable for direct mode interfaces, the code that copies
bandwidth data from the portgroup was also only being executed in the
case of direct mode interfaces. The result was that interfaces using
traditional virtual networks (forward mode='nat|route|none'), and
those using a host bridge for forwarding, would not pick up bandwidth
data from a portgroup defined in the network.
This patch moves that code outside the conditional, so that bandwidth
information is *alway* copied from the appropriate portgroup (unless
the <interface> definition itself already has bandwidth information,
which would take precedence over what's in the portgroup anyway).
Code altered so that it is consistent with the associated comment. The
'autoconf' variable is forced to zero.
Signed-off-by: Neil Wilson <neil@brightbox.co.uk>
In some cases the caller of virPidFileRead might like extra checks
to determine whether the pid just read is really the one they are
expecting. This adds virPidFileReadIfAlive which will check whether
the pid is still alive with kill(0, -1), and (on linux only) will
look at /proc/$PID/path
* libvirt_private.syms, util/virpidfile.c, util/virpidfile.h: Add
virPidFileReadIfValid and virPidFileReadPathIfValid
* network/bridge_driver.c: Use new APIs to check PID validity
The functions for manipulating pidfiles are in util/util.{c,h}.
We will shortly be adding some further pidfile related functions.
To avoid further growing util.c, this moves the pidfile related
functions into a dedicated virpidfile.{c,h}. The functions are
also all renamed to have 'virPidFile' as their name prefix
* util/util.h, util/util.c: Remove all pidfile code
* util/virpidfile.c, util/virpidfile.h: Add new APIs for pidfile
handling.
* lxc/lxc_controller.c, lxc/lxc_driver.c, network/bridge_driver.c,
qemu/qemu_process.c: Add virpidfile.h include and adapt for API
renames
This addresses https://bugzilla.redhat.com/show_bug.cgi?id=713728
When "defining" a new network (or one that exists but isn't currently
active) the new definition is stored in network->def, but for a
network that already exists and is active, the new definition is
stored in network->newDef, and then moved over to network->def as soon
as the network is destroyed.
However, the code that writes the dhcp and dns hosts files used by
dnsmasq was always using network->def for its information, even when
the new data was actually in network->newDef, so the hosts files
always lagged one edit behind the definition.
This patch changes the code to keep the pointer to the new definition
after it's been assigned into the network, and use it directly
(regardless of whether it's stored in network->newDef or network->def)
to construct the hosts files.
Coverity complained that 395 out of 409 virAsprintf calls are
checked, and therefore assumed that the remaining cases are bugs
waiting to happen. But in each of these cases, a failed virAsprintf
will properly set the target string to NULL, and pass on that
failure to the caller, without wasting efforts to check the call.
Adding the ignore_value silences Coverity.
* src/conf/domain_audit.c (virDomainAuditGetRdev): Ignore
virAsprintf return value, when it behaves like we need.
* src/network/bridge_driver.c (networkDnsmasqLeaseFileNameDefault)
(networkRadvdConfigFileName, networkBridgeDummyNicName)
(networkRadvdPidfileBasename): Likewise.
* src/util/storage_file.c (absolutePathFromBaseFile): Likewise.
* src/openvz/openvz_driver.c (openvzGenerateContainerVethName):
Likewise.
* src/util/command.c (virCommandTranslateStatus): Likewise.
This is in response to:
https://bugzilla.redhat.com/show_bug.cgi?id=723862
which points out that a guest on an "isolated" network could
potentially exploit the DNS forwarding provided by dnsmasq to create a
communication channel to the outside.
This patch eliminates that possibility by adding the "--no-resolv"
argument to the dnsmasq commandline, which tells dnsmasq to not
forward on any requests that it can't resolve itself (by looking at
its own static hosts files and runtime list of dhcp clients), but to
instead return a failure for those requests.
This shouldn't cause any undesirable change from current
behavior, even in the case where a guest is currently configured with
multiple interfaces, one of them being connected to an isolated
network, and another to a network that does have connectivity to the
outside. If the isolated network's DNS server is queried for a name
it doesn't know, it will return "Refused" rather than "Unknown", which
indicates to the guest that it should query other servers, so it then
queries the connected DNS server, and gets the desired response.
Every DomainNetDef has a bandwidth, as does every portgroup.
Whenever a DomainNetDef of type NETWORK is about to be used, a call is
made to networkAllocateActualDevice(). This function chooses the "best"
bandwidth object and places it in the DomainActualNetDef.
From that point on, whenever some code needs to use the bandwidth data
for the interface, it's retrieved with virDomainNetGetActualBandwidth(),
which will always return the "best" info as determined in the
previous step.
Although most functions in libvirt return 0 on success and < 0 on
failure, there are a few functions lingering around that return errno
(a positive value) on failure, and sometimes code calling those
functions incorrectly assumes the <0 standard. I noticed one of these
the other day when auditing networkStartDhcpDaemon after Guido Gunther
found a place where success was improperly returned on failure (that
patch has been acked and is pending a push). The problem was that it
expected the return value from virFileReadPid to be < 0 on failure,
but it was actually positive (it was also neglected to set the return
code in this case, similar to the bug found by Guido).
This all led to the fact that *all* of the virFile*Pid functions in
util.c are returning errno on failure. This patch remedies that
problem by changing them all to return -errno on failure, and makes
any necessary changes to callers of the functions. (In the meantime, I
also properly set the return code on failure of virFileReadPid in
networkStartDhcpDaemon).
The new listenNetwork attribute needs to learn an IP address based on a
named network. This patch provides a function networkGetNetworkAddress
which provides that.
Some networks have an IP address explicitly in their configuration
(ie, those with a forward type of "none", "route", or "nat"). For
those, we can just return the IP address from the config.
The rest will have a physical device associated with them (either via
<bridge name='...'/>, <forward ... dev='...'/>, or possibly via a pool
of interfaces inside the network's <forward> element) and we will need
to ask the kernel for a current IP address of that device (via the
newly added ifaceGetIPAddress)
If networkGetNetworkAddress encounters an error while trying to learn
the address for a network, it will return -1. In the case that libvirt
has been compiled without the network driver, the call is a macro
which reduces to -2. This allows differentiating between a failure of
the network driver, and its complete absence.
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. When WITH_NETWORK is false, these
functions are all #defined to be "0" in the .h file (effectively
becoming a NOP) to prevent link errors.
qemu_(command|driver|hotplug|process).c - add calls to the above APIs
in the appropriate places.
tests/Makefile.am - we 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...)
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.
The network XML is updated in the following ways:
1) The <forward> element can now contain a list of forward interfaces:
<forward .... >
<interface dev='eth10'/>
<interface dev='eth11'/>
<interface dev='eth12'/>
<interface dev='eth13'/>
</forward>
The first of these takes the place of the dev attribute that is
normally in <forward> - when defining a network you can specify
either one, and on output both will be present. If you specify
both on input, they must match.
2) In addition to forward modes of 'nat' and 'route', these new modes
are supported:
private, passthrough, vepa - when this network is referenced by a
domain's interface, it will have the same effect as if the
interface had been defined as type='direct', e.g.:
<interface type='direct'>
<source mode='${mode}' dev='${dev}>
...
</interface>
where ${mode} is one of the three new modes, and ${dev} is an interface
selected from the list given in <forward>.
bridge - if a <forward> dev (or multiple devs) is defined, and
forward mode is 'bridge' this is just like the modes 'private',
'passthrough', and 'vepa' above. If there is no forward dev
specified but a bridge name is given (e.g. "<bridge
name='br0'/>"), then guest interfaces using this network will use
libvirt's "host bridge" mode, equivalent to this:
<interface type='bridge'>
<source bridge='${bridge-name}'/>
...
</interface>
3) A network can have multiple <portgroup> elements, which may be
selected by the guest interface definition (by adding
"portgroup='${name}'" in the <source> element along with the
network name). Currently a portgroup can only contain a
virtportprofile, but the intent is that other configuration items
may be put there int the future (e.g. bandwidth config). When
building a guest's interface, if the <interface> XML itself has no
virtportprofile, and if the requested network has a portgroup with
a name matching the name given in the <interface> (or if one of the
network's portgroups is marked with the "default='yes'" attribute),
the virtportprofile from that portgroup will be used by the
interface.
4) A network can have a virtportprofile defined at the top level,
which will be used by a guest interface when connecting in one of
the 'direct' modes if the guest interface XML itself hasn't
specified any virtportprofile, and if there are also no matching
portgroups on the network.
Some callers expected virFileMakePath to set errno, some expected
it to return an errno value. Unify this to return 0 on success and
-1 on error. Set errno to report detailed error information.
Also optimize virFileMakePath if stat fails with an errno different
from ENOENT.
networkSaveDnsmasqHostsfile was added in 8fa9c22142 (Apr 2010).
It has a force flag. If the dnsmasq hostsfile already exists force
needs to be true to overwrite it. networkBuildDnsmasqArgv sets force
to false, networkDefine sets it to true. This results in the
hostsfile being written only in networkDefine in the common case.
If no error occurred networkSaveDnsmasqHostsfile returns true and
networkBuildDnsmasqArgv adds the --dhcp-hostsfile to the dnsmasq
command line.
networkSaveDnsmasqHostsfile was changed in 89ae9849f7 (24 Jun 2011)
to return a new dnsmasqContext instead of reusing one. This change broke
the logic of the force flag as now networkSaveDnsmasqHostsfile returns
NULL on error, but the early return -- if force was not set and the
hostsfile exists -- returns 0. This turned the early return in an error
case and networkBuildDnsmasqArgv didn't add the --dhcp-hostsfile option
anymore if the hostsfile already exists. It did because networkDefine
created the hostsfile already.
Then 9d4e2845d4 fixed the return 0 case in networkSaveDnsmasqHostsfile
but didn't apply the force option correctly to the new addnhosts file.
Now force doesn't control an early return anymore, but influences the
handling of the hostsfile context creation and dnsmasqSave is always
called now. This commit also added test cases that reveal several
problems. First, the tests now calls functions that try to write the
dnsmasq config files to disk. If someone runs this tests as root this
might overwrite actively used dnsmasq config files, this is a no-go. Also
the tests depend on configure --localstatedir, this needs to be fixed as
well, because it makes the tests fail when localstatedir is different
from /var.
This patch does several things to fix this:
1) Move dnsmasqContext creation and saving out of networkBuildDnsmasqArgv
to the caller to separate the command line generation from the config
file writing. This makes the command line generation testable without the
risk of interfering with system files, because the tests just don't call
dnsmasqSave.
2) This refactoring of networkSaveDnsmasqHostsfile makes the force flag
useless as the saving happens somewhere else now. This fixes the wrong
usage of the force flag in combination with then newly added addnhosts
file by removing the force flag.
3) Adapt the wrong test cases to the correct behavior, by adding the
missing --dhcp-hostsfile option. Both affected tests contain DHCP host
elements but missed the necessary --dhcp-hostsfile option.
4) Rename networkSaveDnsmasqHostsfile to networkBuildDnsmasqHostsfile,
because it doesn't save the dnsmasqContext anymore.
5) Move all directory creations in dnsmasq context handling code from
the *New functions to dnsmasqSave to avoid directory creations in system
paths in the test cases.
6) Now that networkBuildDnsmasqArgv doesn't create the dnsmasqContext
anymore the test case can create one with the localstatedir that is
expected by the tests instead of the configure --localstatedir given one.
If a domain name is defined for a network, add the --expand-hosts
option to the dnsmasq commandline. This results in the domain being
added to any hostname that is defined in a dns <host> element and
contains no '.' characters (i.e. it is an "unqualified"
hostname). Since PTR records are automatically created for any name
defined in <host>, the result of a PTR request will change from the
unqualified name to the qualified name.
This also has the same effect on any hostnames that dnsmasq reads
from the host's /etc/hosts file.
(In the case of guest hostnames that were learned by dnsmasq via DHCP
requests, they were already getting the domain name added on, even
without --expand-hosts).
Convert networkDnsmasqLeaseFileName to a replaceable function pointer
that allow the testsuite to use a version of that function that is not
depending on configure --localstatedir.
This fixes 5 of 6 test failures, when configure --localstatedir isn't
set to /var.
This commit introduces names definition for the DNS hosts file using
the following syntax:
<dns>
<host ip="192.168.1.1">
<name>alias1</name>
<name>alias2</name>
</host>
</dns>
Some of the improvements and fixes were done by Laine Stump so
I'm putting him into the SOB clause again ;-)
Signed-off-by: Michal Novotny <minovotn@redhat.com>
Signed-off-by: Laine Stump <laine@laine.org>
The dnsmasq commandline was being built as a part of running
dnsmasq. This patch puts the commandline build into a separate
function (and exports it as a private API) making it possible to build
a dnsmasq commandline without executing it, so that we can write a
test program to verify that the proper commandlines are being created.
Signed-off-by: Michal Novotny <minovotn@redhat.com>
This commit introduces the <dns> element and <txt> record for the
virtual DNS network. The DNS TXT record can be defined using following
syntax in the network XML file:
<dns>
<txt name="example" value="example value" />
</dns>
Also, the Relax-NG scheme has been altered to allow the texts without
spaces only for the name element and some nitpicks about memory
free'ing have been fixed by Laine so therefore I'm adding Laine to the
SOB clause ;-)
Signed-off-by: Michal Novotny <minovotn@redhat.com>
Signed-off-by: Laine Stump <laine@laine.org>
Since we virEventRegisterDefaultImpl is now a public API, callers need
a way to invoke the default registered Handle and Timeout functions. We
already have general functions for these internally, so promote
them to the public API.
v2:
Actually add APIs to libvirt.h
Change all the driver struct initializers to use the
C99 style, leaving out unused fields. This will make
it possible to add new APIs without changing every
driver. eg change:
qemudDomainResume, /* domainResume */
qemudDomainShutdown, /* domainShutdown */
NULL, /* domainReboot */
qemudDomainDestroy, /* domainDestroy */
to
.domainResume = qemudDomainResume,
.domainShutdown = qemudDomainShutdown,
.domainDestroy = qemudDomainDestroy,
And get rid of any existing C99 style initializersr which
set NULL, eg change
.listPools = vboxStorageListPools,
.numOfDefinedPools = NULL,
.listDefinedPools = NULL,
.findPoolSources = NULL,
.poolLookupByName = vboxStoragePoolLookupByName,
to
.listPools = vboxStorageListPools,
.poolLookupByName = vboxStoragePoolLookupByName,
We were 31/73 on whether to translate; since less than 50% translated
and since VIR_INFO is less than VIR_WARN which also doesn't translate,
this makes sense.
* cfg.mk (sc_prohibit_gettext_markup): Add VIR_INFO, since it
falls between WARN and DEBUG.
* daemon/libvirtd.c (qemudDispatchSignalEvent, remoteCheckAccess)
(qemudDispatchServer): Adjust offenders.
* daemon/remote.c (remoteDispatchAuthPolkit): Likewise.
* src/network/bridge_driver.c (networkReloadIptablesRules)
(networkStartNetworkDaemon, networkShutdownNetworkDaemon)
(networkCreate, networkDefine, networkUndefine): Likewise.
* src/qemu/qemu_driver.c (qemudDomainDefine)
(qemudDomainUndefine): Likewise.
* src/storage/storage_driver.c (storagePoolCreate)
(storagePoolDefine, storagePoolUndefine, storagePoolStart)
(storagePoolDestroy, storagePoolDelete, storageVolumeCreateXML)
(storageVolumeCreateXMLFrom, storageVolumeDelete): Likewise.
* src/util/bridge.c (brProbeVnetHdr): Likewise.
* po/POTFILES.in: Drop src/util/bridge.c.
These VIR_XXXX0 APIs make us confused, use the non-0-suffix APIs instead.
How do these coversions works? The magic is using the gcc extension of ##.
When __VA_ARGS__ is empty, "##" will swallow the "," in "fmt," to
avoid compile error.
example: origin after CPP
high_level_api("%d", a_int) low_level_api("%d", a_int)
high_level_api("a string") low_level_api("a string")
About 400 conversions.
8 special conversions:
VIR_XXXX0("") -> VIR_XXXX("msg") (avoid empty format) 2 conversions
VIR_XXXX0(string_literal_with_%) -> VIR_XXXX(%->%%) 0 conversions
VIR_XXXX0(non_string_literal) -> VIR_XXXX("%s", non_string_literal)
(for security) 6 conversions
Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
