Support for firewalld is a feature that can be selectively enabled or
disabled (using --with-firewalld/--without-firewalld), not merely
something that must be accounted for in the code if it is present with
no exceptions. It is more consistent with other usage in libvirt to
use WITH_FIREWALLD rather than HAVE_FIREWALLD.
Signed-off-by: Laine Stump <laine@laine.org>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
This adds an additional directive to the dnsmasq configuration file that
notifies clients via dhcp about the link's MTU. Guests can then choose
adjust their link accordingly.
Signed-off-by: Casey Callendrello <cdc@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Allow the platform driver impls to run logic before and after the
firewall reload process.
Reviewed-by: Laine Stump <laine@laine.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
networkMigrateStateFiles was added nearly 5 years ago when the network
state directory was moved from /var/lib/libvirt to /var/run/libvirt
just prior to libvirt-1.2.4). It was only required to maintain proper
state information for networks that were active during an upgrade that
didn't involve rebooting the host. At this point the likelyhood of
anyone upgrading their libvirt from pre-1.2.4 directly to 5.0.0 or
later *without rebooting the host* is probably so close to 0 that no
properly informed bookie would take *any* odds on it happening, so it
seems appropriate to remove this pointless code.
Signed-off-by: Laine Stump <laine@laine.org>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
In many files there are header comments that contain an Author:
statement, supposedly reflecting who originally wrote the code.
In a large collaborative project like libvirt, any non-trivial
file will have been modified by a large number of different
contributors. IOW, the Author: comments are quickly out of date,
omitting people who have made significant contribitions.
In some places Author: lines have been added despite the person
merely being responsible for creating the file by moving existing
code out of another file. IOW, the Author: lines give an incorrect
record of authorship.
With this all in mind, the comments are useless as a means to identify
who to talk to about code in a particular file. Contributors will always
be better off using 'git log' and 'git blame' if they need to find the
author of a particular bit of code.
This commit thus deletes all Author: comments from the source and adds
a rule to prevent them reappearing.
The Copyright headers are similarly misleading and inaccurate, however,
we cannot delete these as they have legal meaning, despite being largely
inaccurate. In addition only the copyright holder is permitted to change
their respective copyright statement.
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
All of the ones being removed are pulled in by internal.h. The only
exception is sanlock which expects the application to include <stdint.h>
before sanlock's headers, because sanlock prototypes use fixed width
int, but they don't include stdint.h themselves, so we have to leave
that one in place.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Acked-by: Michal Privoznik <mprivozn@redhat.com>
It doesn't really make sense for us to have stdlib.h and string.h but
not stdio.h in the internal.h header.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Acked-by: Michal Privoznik <mprivozn@redhat.com>
When starting network a macmap object is created (which stores
MAC -> domain name mappings). However, if something goes wrong
(e.g. virNetDevIPCheckIPv6Forwarding() fails) then the object is
leaked.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1600468
If we are unable to read leases file (no matter what the reason
is), we return 0 - just like if there were no leases. However,
because we use virFileReadAll() an error is printed into the log.
Note that not all networks have leases file - only those for
which we start dnsmasq.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
With 'switch' we can utilize the compile time enum checks which we can't
rely on with plain 'if' conditions.
Signed-off-by: Shi Lei <shilei.massclouds@gmx.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
When libvirtd is restarted, it checks that each guest tap device is
still attached to the bridge device that the configuration info says
it should be connected to. If not, the tap will be disconnected from
[wherever it is] and connected to [wherever it should be].
The previous code that did this did not account for:
1) the IFLA_MASTER attribute in a netdev's ifinfo will be set to
"ovs-system" for any tap device connected to an OVS bridge, *not*
to the name of the bridge it is attached to.
2) virNetDevRemovePort() only works for devices that are attached to a
standard Linux host bridge. If a device is currently attached to an
OVS bridge, then virNetDevOpenvswitchRemovePort() must be called
instead.
This patch remedies those problems, and adds a couple of information
log messages to aid in debugging any future problem.
Resolves: https://bugzilla.redhat.com/1596176
Signed-off-by: Laine Stump <laine@laine.org>
ACKed-by: Michal Privoznik <mprivozn@redhat.com>
Rather than have virJSONValueArraySize return a -1 when the input
is not an array and then splat an error message, let's check for
an array before calling and then change the return to be a size_t
instead of ssize_t.
That means using the helper virJSONValueIsArray as well as using a
more generic error message such as "Malformed <something> array".
In some cases we can remove stack variables and when we cannot,
those variables should be size_t not ssize_t. Alter a few references
of if (!value) to be if (value == 0) instead as well.
Some callers can already assume an array is being worked on based
on the previous call, so there's less to do.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Ensuring that we don't call the virDrvConnectOpen method with a NULL URI
means that the drivers can drop various checks for NULL URIs. These were
not needed anymore since the probe functionality was split
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Declare what URI schemes a driver supports in its virConnectDriver
struct. This allows us to skip trying to open the driver entirely
if the URI scheme doesn't match.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Add a localOnly flag to the virConnectDriver struct which allows a
driver to indicate whether it is local-only, or permits remote
connections. Stateful drivers running inside libvirtd are generally
local only. This allows us to remote the check for uri->server != NULL
from most drivers.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Now that we have the ability to easily open connections to secondary
drivers, eg network:///system, it is possible to reimplement the
virDomainNetResolveActualType method in terms of the public API. This
avoids the need to have the network driver provide a callback for it.
Reviewed-by: John Ferlan <jferlan@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Currently the QEMU driver will call directly into the network driver
impl to modify resolve the atual type of NICs with type=network. It
has todo this before it has allocated the actual NIC. This introduces
a callback system to allow us to decouple the QEMU driver from the
network driver.
This is a short term step, as it ought to be possible to achieve the
same end goal by simply querying XML via the public network API. The
QEMU code in question though, has no virConnectPtr conveniently
available at this time.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The QEMU driver calls into the network driver to get the first IP
address of the network. This information is readily available via the
formal public API by fetching the XML doc and then parsing it.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Currently the QEMU driver will call directly into the network driver
impl to modify network device bandwidth for interfaces with
type=network. This introduces a callback system to allow us to decouple
the QEMU driver from the network driver.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Currently virt drivers will call directly into the network driver impl
to allocate domain interface devices where type=network. This introduces
a callback system to allow us to decouple the virt drivers from the
network driver.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Allow the possibility of opening a connection to only the network
driver, by defining network:///system and network:///session URIs
and registering a fake hypervisor driver that supports them.
The hypervisor drivers can now directly open a network driver
connection at time of need, instead of having to pass around a
virConnectPtr through many functions. This will facilitate the later
change to support separate daemons for each driver.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
By convention the last thing in the driver.c files should be the driver
callback table and function to register it.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
VM drivers may need to store additional private data to the status XML
so that it can be restored after libvirtd restart. Since not everything
is needed add a callback infrastructure, where VM drivers can add only
stuff they need.
Note that the private data is formatted as a <privateData> sub-element
of the <disk> or <backingStore> <source> sub-element. This is done since
storing it out of band (in the VM private data) would require a complex
matching process to allow to put the data into correct place.
There were a bunch of commentary blocks that were literally useless in
terms of describing what the code following them does, since most of
them were documenting "the obvious" or it just wouldn't help at all.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Similarly to previous patch, for some types of interface domain
and host are on the same side of RX/TX barrier. In that case, we
need to set up the QoS differently. Well, swapped.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Back in the day when I was implementing QoS for networks there
were no self inflating virBitmaps. Only the static ones.
Therefore, I had to allocate the whole 8KB of memory in order to
keep track of used/unused class IDs. This is rather wasteful
because nobody is ever gonna use that much classes (kernel
overhead would drastically lower the bandwidth). Anyway, now that
we have self inflating bitmaps we can start small and allocate
more if there's need for it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
To be consistent with the API definition, use the @maxnames instead
of @nnames when describing/comparing against the maximum names to
be provided for the *ConnectList[Defined]Networks APIs.
Signed-off-by: John Ferlan <jferlan@redhat.com>
In preparation to privatize the virNetworkObj - create an accessor function
to get the current @persistent value. Also change the value to a bool rather
than an unsigned int (since that's how it's generated anyway).
Signed-off-by: John Ferlan <jferlan@redhat.com>
In order to privatize the virNetworkObj create accessors in virnetworkobj
in order to handle the get/set of the active value.
Also rather than an unsigned int, convert it to a boolean to match other
drivers representation and the reality of what it is.
Signed-off-by: John Ferlan <jferlan@redhat.com>
In preparation for making the object private, create a couple of API's
to get the obj->def & obj->newDef and set the obj->def.
While altering networkxml2conftest.c to use the virNetworkObjSetDef
API, fix the name of the variable from @dev to @def
Signed-off-by: John Ferlan <jferlan@redhat.com>
Change the variable name to be a bit more descriptive and less confusing
when used with the data.network.actual->class_id.
Signed-off-by: John Ferlan <jferlan@redhat.com>
In preparation for making the object private, create/use a couple of API's
to get/set the obj->dnsmasqPid and obj->radvdPid.
NB: Since the pid's can sometimes changed based on intervening functions,
be sure to always fetch the latest value.
Signed-off-by: John Ferlan <jferlan@redhat.com>
In preparation for having a private virNetworkObj - let's create/move some
API's that handle the obj->macmap. The API's will be renamed to have a
virNetworkObj prefix to follow conventions and the arguments slightly
modified to accept what's necessary to complete their task.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Move networkMacMgrFileName into src/util/virmacmap.c and rename to
virMacMapFileName. We're about to move some more MacMgr processing
files into virnetworkobj and it doesn't make sense to have this helper
in the driver or in virnetworkobj.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Modify code to have two spaces between functions, follow function more
recent function formatting w/r/t args per line and function return type
and name on separate lines.
Signed-off-by: John Ferlan <jferlan@redhat.com>
If a remote call fails during event registration (more than likely from
a network failure or remote libvirtd restart timed just right), then when
calling the virObjectEventStateDeregisterID we don't want to call the
registered @freecb function because that breaks our contract that we
would only call it after succesfully returning. If the @freecb routine
were called, it could result in a double free from properly coded
applications that free their opaque data on failure to register, as seen
in the following details:
Program terminated with signal 6, Aborted.
#0 0x00007fc45cba15d7 in raise
#1 0x00007fc45cba2cc8 in abort
#2 0x00007fc45cbe12f7 in __libc_message
#3 0x00007fc45cbe86d3 in _int_free
#4 0x00007fc45d8d292c in PyDict_Fini
#5 0x00007fc45d94f46a in Py_Finalize
#6 0x00007fc45d960735 in Py_Main
#7 0x00007fc45cb8daf5 in __libc_start_main
#8 0x0000000000400721 in _start
The double dereference of 'pyobj_cbData' is triggered in the following way:
(1) libvirt_virConnectDomainEventRegisterAny is invoked.
(2) the event is successfully added to the event callback list
(virDomainEventStateRegisterClient in
remoteConnectDomainEventRegisterAny returns 1 which means ok).
(3) when function remoteConnectDomainEventRegisterAny is hit,
network connection disconnected coincidently (or libvirtd is
restarted) in the context of function 'call' then the connection
is lost and the function 'call' failed, the branch
virObjectEventStateDeregisterID is therefore taken.
(4) 'pyobj_conn' is dereferenced the 1st time in
libvirt_virConnectDomainEventFreeFunc.
(5) 'pyobj_cbData' (refered to pyobj_conn) is dereferenced the
2nd time in libvirt_virConnectDomainEventRegisterAny.
(6) the double free error is triggered.
Resolve this by adding a @doFreeCb boolean in order to avoid calling the
freeCb in virObjectEventStateDeregisterID for any remote call failure in
a remoteConnect*EventRegister* API. For remoteConnect*EventDeregister* calls,
the passed value would be true indicating they should run the freecb if it
exists; whereas, it's false for the remote call failure path.
Patch based on the investigation and initial patch posted by
fangying <fangying1@huawei.com>.
If the network isn't active during networkNotifyActualDevice(), we
would log an error message stating that the bridge device didn't
exist. This patch adds a check to see if the network is active, making
the logs more useful in the case that it isn't.
Partially resolves: https://bugzilla.redhat.com/1442700
Nothing that could happen during networkNotifyActualDevice() could
justify unceremoniously killing the qemu process, but that's what we
were doing.
In particular, new code added in commit 85bcc022 (first appearred in
libvirt-3.2.0) attempts to reattach tap devices to their assigned
bridge devices when libvirtd restarts (to make it easier to recover
from a restart of a libvirt network). But if the network has been
stopped and *not* restarted, the bridge device won't exist and
networkNotifyActualDevice() will fail.
This patch changes networkNotifyActualDevice() and
qemuProcessNotifyNets() to return void, so that qemuProcessReconnect()
will soldier on regardless of what happens (any errors will still be
logged though).
Partially resolves: https://bugzilla.redhat.com/1442700
This patch makes use of the virNetDevSetCoalesce() function to make
appropriate settings effective for devices that support them.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1414627
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Use "virNetworkObj" as a prefix for any external API in virnetworkobj.
Also a couple of functions were local to virnetworkobj.c, so remove their
external defs in virnetworkobj.h.
Rename the API to be a better description of what it does. Besides, a
subsequent patch will rename virNetworkAssignDef to virNetworkObjAssignDef
so rather than make that patch confusing we'll take the intermittent step
in this patch.
Move all the virNetworkObj related API/data structures into their own
modules virnetworkobj.{c,h} from the network_conf.{c,h}
Purely code motion at this point plus adjustments to cleanly build
The virMacMap module is there for dumping [domain, <list of is
MACs>] pairs into a file so that libvirt_guest NSS module can use
it. Whenever a interface is allocated from network (e.g. on
domain<F2> startup or NIC hotplug), network is notified and so is
virMacMap module subsequently. The module update functions
networkMacMgrAdd() and networkMacMgrDel() gracefully handle the
case when there's no module. The problem is, the module is
created if and only if network is freshly started, or if the
daemon restarts and network previously had the module.
This is not very user friendly - if users want to use the NSS
module they need to destroy their network and bring it up again
(and subsequently all the domains using it).
One disadvantage of this approach implemented here is that one
may get just partial results: any already running network does
not record mac maps, thus only newly plugged domains will be
stored in the module. The network restart scenario is not touched
by this of course. But one can argue that older libvirts had
never recorded the mac maps anyway.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1434882
Imagine the following scenario:
1) virsh net-start default
2) virsh start myFavouriteDomain
3) virsh net-destroy default
4) virsh destroy myFavouriteDomain
(assuming myFavouriteDomain has an interface from default
network)
Regardless of how unlikely this scenario looks like, we should
not crash. The problem is, on net-destroy in
networkShutdownNetworkVirtual() the virMacMap module is unrefed,
but the stale pointer is kept around. Thus when the domain
destroy procedure comes in, networkReleaseActualDevice() and
subsequently networkMacMgrDel() is called. This function sees the
stale pointer and starts calling the virMacMap module APIs which
work over freed memory.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
This adds a few validations to the devices listed for a hostdev network:
* devices must be listed by PCI address, not by netdev name
* listing a device by PCI address is valid only for hostdev networks, not
for other types of network (e.g. macvtap passthrough).
* each device in a hostdev pool must be an SR-IOV VF
Resolves: https://bugzilla.redhat.com/1004676
commit 00d28a78 added a check to see if there were any IPv6 routes
added by RA (Router Advertisement) via an interface that had accept_ra
set to something other than "2". The check was being done
unconditionally, but it's only relevant if IPv6 forwarding is going to
be turned on, and that will only happen if the network has an IPv6
address.
The prototype requires a NONNULL argument and the only caller passes in
a non-null parameter. Besides the "else if" condition would deref it anyway.
Signed-off-by: John Ferlan <jferlan@redhat.com>
If a network is destroyed and restarted, or its bridge is changed, any
tap devices that had previously been connected to the bridge will no
longer be connected. As a first step in automating a reconnection of
all tap devices when this happens, this patch modifies
networkNotifyActualDevice() (which is called once for every
<interface> of every active domain whenever libvirtd is restarted) to
reconnect any tap devices that it finds disconnected.
With this patch in place, you will need to restart libvirtd to
reconnect all the taps to their proper bridges. A future patch will
add a callback that hypervisor drivers can register with the network
driver to that the network driver can trigger this behavior
automatically whenever a network is started.
When enabling IPv6 on all interfaces, we may get the host Router
Advertisement routes discarded. To avoid this, the user needs to set
accept_ra to 2 for the interfaces with such routes.
See https://www.kernel.org/doc/Documentation/networking/ip-sysctl.txt
on this topic.
To avoid user mistakenly losing routes on their hosts, check
accept_ra values before enabling IPv6 forwarding. If a RA route is
detected, but neither the corresponding device nor global accept_ra
is set to 2, the network will fail to start.
It was pointed out here:
https://bugzilla.redhat.com/show_bug.cgi?id=1331796#c4
that we shouldn't be adding a "no-resolv" to the dnsmasq.conf file for
a network if there isn't any <forwarder> element that specifies an IP
address but no qualifying domain. If there is such an element, it will
handle all DNS requests that weren't otherwise handled by one of the
forwarder entries with a matching domain attribute. If not, then DNS
requests that don't match the domain of any <forwarder> would not be
resolved if we added no-resolv.
So, only add "no-resolv" when there is at least one <forwarder>
element that specifies an IP address but no qualifying domain.
In GCC 7 there is a new warning triggered when a switch
case has a conditional statement (eg if ... else...) and
some of the code paths fallthrough to the next switch
statement. e.g.
conf/domain_conf.c: In function 'virDomainChrEquals':
conf/domain_conf.c:14926:12: error: this statement may fall through [-Werror=implicit-fallthrough=]
if (src->targetTypeAttr != tgt->targetTypeAttr)
^
conf/domain_conf.c:14928:5: note: here
case VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE:
^~~~
conf/domain_conf.c: In function 'virDomainChrDefFormat':
conf/domain_conf.c:22143:12: error: this statement may fall through [-Werror=implicit-fallthrough=]
if (def->targetTypeAttr) {
^
conf/domain_conf.c:22151:5: note: here
default:
^~~~~~~
GCC introduced a __attribute__((fallthrough)) to let you
indicate that this is intentionale behaviour rather than
a bug.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
virNetDevTapCreateInBridgePort() has always set the new tap device to
the current MTU of the bridge it's being attached to. There is one
case where we will want to set the new tap device to a different
(usually larger) MTU - if that's done with the very first device added
to the bridge, the bridge's MTU will be set to the device's MTU. This
patch allows for that possibility by adding "int mtu" to the arg list
for virNetDevTapCreateInBridgePort(), but all callers are sending -1,
so it doesn't yet have any effect.
Since the requested MTU isn't necessarily what is used in the end (for
example, if there is no MTU requested, the tap device will be set to
the current MTU of the bridge), and the hypervisor may want to know
the actual MTU used, we also return the actual MTU to the caller (if
actualMTU is non-NULL).
After 478ddedc12 a bug is fixed where we wrongly presumed loopack
device name on non-Linux systems. It's lo0. However, the fix is
not reflected in the tests which are failing now.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Similarly to localOnly DNS domain, localPtr attribute can be used to
tell the DNS server not to forward reverse lookups for unknown IPs which
belong to the virtual network.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Now that we have a module that's able to track
<domain, mac addres list> pairs, hook it up into
our network driver.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
In dd7bfb2cdc I've removed locking of the network driver upon
it's allocation. However, I forgot to remove one location of the
driver unlock.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
commit 9065cfaa added the ability to disable DNS services for a
libvirt virtual network. If neither DNS nor DHCP is needed for a
network, then we don't need to start dnsmasq, so code was added to
check for this.
Unfortunately, it was written with a great lack of attention to detail
(I can say that, because I was the author), and the loop that checked
if DHCP is needed for the network would never end if the network had
multiple IP addresses and the first <ip> had no <dhcp> subelement
(which would have contained a <range> or <host> subelement, thus
requiring DHCP services).
This patch rewrites the check to be more compact and (more
importantly) finite.
This bug was present in release 2.2.0 and 2.3.0, so will need to be
backported to any relevant maintainence branches.
Reported here:
https://www.redhat.com/archives/libvirt-users/2016-October/msg00032.htmlhttps://www.redhat.com/archives/libvirt-users/2016-October/msg00045.html
New line character in name of network is now forbidden because it
mess virsh output and can be confusing for users. Validation of
name is done in network driver, after parsing XML to avoid
problems with disappeared network which was already created with
new-line char in name.
Closes-Bug: https://bugzilla.redhat.com/show_bug.cgi?id=818064
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
The dnsmasq man page recommends that dhcp-authoritative "should be
set when dnsmasq is definitely the only DHCP server on a network".
This is the case for libvirt-managed virtual networks.
The effect of this is that VMs that fail to renew their DHCP lease
in time (e.g. if the VM or host is suspended) will be able to
re-acquire the lease even if it's expired, unless the IP address has
been taken by some other host. This avoids various annoyances caused
by changing VM IP addresses.
For some unknown reason the original implementation of the <forwarder>
element only took advantage of part of the functionality in the
dnsmasq feature it exposes - it allowed specifying the ip address of a
DNS server which *all* DNS requests would be forwarded to, like this:
<forwarder addr='192.168.123.25'/>
This is a frontend for dnsmasq's "server" option, which also allows
you to specify a domain that must be matched in order for a request to
be forwarded to a particular server. This patch adds support for
specifying the domain. For example:
<forwarder domain='example.com' addr='192.168.1.1'/>
<forwarder domain='www.example.com'/>
<forwarder domain='travesty.org' addr='10.0.0.1'/>
would forward requests for bob.example.com, ftp.example.com and
joe.corp.example.com all to the DNS server at 192.168.1.1, but would
forward requests for travesty.org and www.travesty.org to
10.0.0.1. And due to the second line, requests for www.example.com,
and odd.www.example.com would be resolved by the libvirt network's own
DNS server (i.e. thery wouldn't be immediately forwarded) even though
they also match 'example.com' - the match is given to the entry with
the longest matching domain. DNS requests not matching any of the
entries would be resolved by the libvirt network's own DNS server.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1331796
If you define a libvirt virtual network with one or more IP addresses,
it starts up an instance of dnsmasq. It's always been possible to
avoid dnsmasq's dhcp server (simply don't include a <dhcp> element),
but until now it wasn't possible to avoid having the DNS server
listening; even if the network has no <dns> element, it is started
using default settings.
This patch adds a new attribute to <dns>: enable='yes|no'. For
backward compatibility, it defaults to 'yes', but if you don't want a
DNS server created for the network, you can simply add:
<dns enable='no'/>
to the network configuration, and next time the network is started
there will be no dns server created (if there is dhcp configuration,
dnsmasq will be started with "port=0" which disables the DNS server;
if there is no dhcp configuration, dnsmasq won't be started at all).
The new forward mode 'open' is just like mode='route', except that no
firewall rules are added to assure that any traffic does or doesn't
pass. It is assumed that either they aren't necessary, or they will be
setup outside the scope of libvirt.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=846810
IPv6 RA always contains an implicit default route via
the link-local address of the source of RA. This forces
the guest to install a route via isolated network, which
may disturb the guest's networking in case of multiple interfaces.
More info in 013427e6e7.
The validity of this route is controlled by "default [route] lifetime"
field of RA. If the lifetime is set to 0 seconds, then no route
is installed by receiver.
dnsmasq 2.67+ supports "ra-param=<interface>,<RA interval>,<default
lifetime>" option. We pass "ra-param=*,0,0"
(here, RA_interval=0 means default) to disable default gateway in RA
for isolated networks.
These functions all need to be called from a utility function that
must be located in the util directory, so we move them all into
util/virnetdevip.[ch] now that it exists.
Function and struct names were appropriately changed for the new
location, but all code is unchanged aside from motion and renaming.
This patch splits virnetdev.[ch] into multiple files, with the new
virnetdevip.[ch] containing all the functions related to setting and
retrieving IP-related info for a device (both addresses and routes).
I'm tired of mistyping this all the time, so let's do it the same all
the time (similar to how we changed all "Pci" to "PCI" awhile back).
(NB: I've left alone some things in the esx and vbox drivers because
I'm unable to compile them and they weren't obviously *not* a part of
some API. I also didn't change a couple of variables named,
e.g. "somethingIptables", because they were derived from the name of
the "iptables" command)
Although dns host records are stored in a separate configuration file
that is reread by dnsmasq when it receives a SIGHUP, the txt and srv
records are directly in the dnsmasq .conf file which can't be reread
after initial dnsmasq startup. This means that if an srv or txt record
is modified in a network config, libvirt needs to restart the dnsmasq
process rather than just sending a SIGHUP.
This was pointed out in a question in
https://bugzilla.redhat.com/show_bug.cgi?id=988718 , but no separate
BZ was filed.
SRIOV VFs used in macvtap passthrough mode can take advantage of the
SRIOV card's transparent vlan tagging. All the code was there to set
the vlan tag, and it has been used for SRIOV VFs used for hostdev
interfaces for several years, but for some reason, the vlan tag for
macvtap passthrough devices was stubbed out with a -1.
This patch moves a bit of common validation down to a lower level
(virNetDevReplaceNetConfig()) so it is shared by hostdev and macvtap
modes, and updates the macvtap caller to actually send the vlan config
instead of -1.
Both instances use VIR_WARN() to print the error from a failed
virDBusGetSystemBus() call. Rather than use the virGetLastError
and need to check for valid return err pointer, just use the
virGetLastErrorMessage.
Signed-off-by: John Ferlan <jferlan@redhat.com>
We will segfault of a daemon reload picks up a new network config
that needs to be autostarted. We shouldn't be passing NULL for
network_driver here. This seems like it was missed in the larger
rework in commit 1009a61e
We had both and the only difference was that the latter also included
information about multifunction setting. The problem with that was that
we couldn't use functions made for only one of the structs (e.g.
parsing). To consolidate those two structs, use the one in virpci.h,
include that in domain_conf.h and add the multifunction member in it.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
According to the dnsmasq manpage, the netmask for IPv4 address ranges
will be auto-deteremined from the interface dnsmasq is listening on,
but it can't do this for IPv6 for some reason - it instead assumes a
network prefix of 64 for all IPv6 address ranges. If this is
incorrect, dnsmasq will refuse to give out an address to clients,
instead logging this message:
dnsmasq-dhcp[2380]: no address range available for DHCPv6 request via virbr0
The solution is for libvirt to add ",$prefix" to all IPv6 dhcp-range
arguments when building the dnsmasq.conf file.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1033739
There are times when it's necessary to learn the actual type of a
network connection before any resources have been allocated
(e.g. during qemuProcessPrepareDomain()), but in the past it was
necessary to call networkAllocateActualDevice() in order to have the
actual type filled in.
This new function returns the type of network that *will be* setup
once it actually happens, but without making any changes on the host.
networkStartNetwork() and networkShutdownNetwork() were calling the
wrong type-specific function in the case of networks that were
configured for macvtap ("direct") bridge mode - they were instead
calling the functions for a tap+bridge network. Currently none of
these functions does anything (they just return 0) so it hasn't
created any problems, but that could change in the future.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1316465
An attempt to simplify the code for the VIR_NETWORK_FORWARD_BRIDGE
case of networkUpdateState in commit b61db335 (first in release
1.2.14) resulted in networks based on macvtap bridge mode being
erroneously marked as inactive any time libvirtd was restarted.
The problem is that the original code had differentiated between a
network using tap devices to connect to an existing host-bridge device
(forward mode of VIR_NETWORK_FORWARD_BRIDGE and a non-NULL
def->bridge), and one using macvtap bridge mode to connect to any
ethernet device (still forward mode VIR_NETWORK_FORWARD_BRIDGE, but
null def->bridge), but the changed code assumed that all networks with
VIR_NETWORK_FORWARD_BRIDGE were tap + host-bridge networks, so a null
def->bridge was interpreted as "inactive".
This patch restores the original code in networkUpdateState
We include the file in plenty of places. This is mostly due to
historical reasons. The only place that needs something from the
header file is storage_backend_fs which opens _PATH_MOUNTED. But
it gets the file included indirectly via mntent.h. At no other
place in our code we need _PATH_.*. Drop the include and
configure check then.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
There are three functions that deal with allocating and freeing
devices from a networks netdev/pci device pool:
network(Allocate|Notify|Release)ActualDevice(). These functions also
maintain a counter of the number of domains currently using a network
(regardless of whether or not that network uses a device pool). Each
of these functions had multiple log messages (output using VIR_DEBUG)
that were in slightly different formats and gave varying amounts of
information.
This patch creates a single function to log the pertinent information
in a consistent manner for all three of these functions. Along with
assuring that all the functions produce a consistent form of output
(and making it simpler to change), it adds the MAC address of the
domain interface involved in the operation, making it possible to
verify which interface of which domain the operation is being done for
(assuming that all MAC addresses are unique, of course).
All of these messages are raised from DEBUG to INFO, since they don't
happen that often (once per interface per domain/libvirtd start or
domain stop), and can be very informative and helpful - eliminating
the need to log debug level messages makes it much easier to sort
these out.
networkReleaseActualDevice() and networkNotifyActualDevice() both were
updating the individual devices' connections count in two separate
places (unlike networkAllocateActualDevice() which does it in a single
unified place after success:). The code is correct, but prone to
confusion / later breakage. All of these updates are anyway located at
the end of if/else clauses that are (with the exception of a single
VIR_DEBUG() in each case) immediately followed by the success: label
anyway, so this patch replaces the duplicated ++/-- instructions with
a single ++/-- inside a qualifying "if (dev)" down below success:.
(NB: if dev != NULL, by definition we are using a device (either pci
or netdev, doesn't matter for these purposes) from the network's pool)
The VIR_DEBUG args (which will be replaced in a followup patch anyway)
were all adjusted to account for the connection count being out of
date at the time.
And use the newly added caps->host.netprefix (if it exists) for
interface names that match the autogenerated target names.
Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
Instead of comparing garbage strings against real MAC addresses,
introduce an error mesage for unparsable ones:
$ virsh net-dhcp-leases default --mac t12
error: Failed to get leases info for default
error: invalid MAC address: t12
https://bugzilla.redhat.com/show_bug.cgi?id=1261432
A PCI device may have the capability to setup virtual functions (VFs)
but have them currently all disabled. Prior to this patch, if that was
the case the the node device XML for the device wouldn't report any
virtual_functions capability.
With this patch, if a file called "sriov_totalvfs" is found in the
device's sysfs directory, its contents will be interpreted as a
decimal number, and that value will be reported as "maxCount" in a
capability element of the device's XML, e.g.:
<capability type='virtual_functions' maxCount='7'/>
This will be reported regardless of whether or not any VFs are
currently enabled for the device.
NB: sriov_numvfs (the number of VFs currently active) is also
available in sysfs, but that value is implied by the number of items
in the list that is inside the capability element, so there is no
reason to explicitly provide it as an attribute.
sriov_totalvfs and sriov_numvfs are available in kernels at least as far
back as the 2.6.32 that is in RHEL6.7, but in the case that they
simply aren't there, libvirt will behave as it did prior to this patch
- no maxCount will be displayed, and the virtual_functions capability
will be absent from the device's XML when 0 VFs are enabled.
commit db488c79 assumed that dnsmasq would complete IPv6 DAD before
daemonizing, but in reality it doesn't wait, which creates problems
when libvirt's bridge driver sets the matching "dummy tap device" to
IFF_DOWN prior to DAD completing.
This patch waits for DAD completion by periodically polling the kernel
using netlink to check whether there are any IPv6 addresses assigned
to bridge which have a 'tentative' state (if there are any in this
state, then DAD hasn't yet finished). After DAD is finished, execution
continues. To avoid an endless hang in case something was wrong with
the kernel's DAD, we wait a maximum of 5 seconds.
The internal representation of a JSON array counts the items in
size_t. However, for some reason, when asking for the count it's
reported as int. Firstly, we need the function to return a signed
type as it's returning -1 on an error. But, not every system has
integer the same size as size_t. Therefore, lets return ssize_t.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
There's a check right at the beginning of the function that
shortcuts if the function was called over all NULL arguments.
However, this was meant just as a fool-proof check so that we
don't crash if function is used in a bad manner. Anyway, it makes
Coverity unhappy as it then thinks any of the arguments could be
NULL. Well, with the current state of the code it can't.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
It may happen that an interface don't have any bandwidth set and
a new one is to be set. In that case, @ifaceBand will be NULL.
This will cause troubles later in the code when deciding what to
do.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
So, if a domain vNIC's bandwidth has been successfully set, it's
possible that because @floor is set on network's bridge, this
part may need updating too. And that's exactly what this function
does. While the previous commit introduced a function to check if
@floor can be satisfied, this does all the hard work. In general,
there may be three, well four possibilities:
1) No change in @floor value (either it remain unset, or its
value hasn't changed)
2) The @floor value has changed from a non-zero to a non-zero
value
3) New @floor is to be set
4) Old @floor must be cleared out
The difference between 2), 3) and 4) is, that while in 2) the QoS
tree on the network's bridge already has a special class for the
vNIC, in 3) the class must be created from scratch. In 4) it must
be removed. Fortunately, we have helpers for all three
interesting cases.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
When a domain vNIC's bandwidth is to be changed (at runtime) it is
possible that guaranteed minimal bandwidth (@floor) will change too.
Well, so far it is, because we still don't have an implementation that
allows setting it dynamically, so it's effectively erased on:
#virsh domiftune $dom vnet0 --inbound 0
However, that's slightly unfortunate. We do some checks on domain
startup to see if @floor can be guaranteed. We ought do the same if
QoS is changed at runtime.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
This is no functional change. It's just that later in the series we
will need to pass class_id as an integer.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
There are now many more reasons that virSocketAddrGetRange() could
fail, so it is much more informative to report the error there instead
of in the caller. (one of the two callers was previously assuming
success, which is almost surely safe based on the parsing that has
already happened to the config by that time, but it still is nicer to
account for an error "just in case")
Part of fix for: https://bugzilla.redhat.com/show_bug.cgi?id=985653
This loop had automatic variable definitions mixed with code. This
patch moves the definitions to the top of the function and puts
cleanup for them at the bottom. No functional change.
Part of fix for: https://bugzilla.redhat.com/show_bug.cgi?id=985653
virSocketAddrGetRange() has been updated to take the network address
and prefix, and now checks that both the start and end of the range
are within that network, thus validating that the entire range of
addresses is in the network. For IPv4, it also checks that ranges to
not start with the "network address" of the subnet, nor end with the
broadcast address of the subnet (this check doesn't apply to IPv6,
since IPv6 doesn't have a broadcast or network address)
Negative tests have been added to the network update and socket tests
to verify that bad ranges properly generate an error.
This resolves: https://bugzilla.redhat.com/show_bug.cgi?id=985653
To silence Coverity just add a 'p &&' in front of the check in
networkFindUnusedBridgeName after the strchr() call. Even though
we know it's not possible to have strchr return NULL since the only
way into the function is if there is a '%' in def->bridge or it's NULL.
Signed-off-by: John Ferlan <jferlan@redhat.com>
For some reason, we allow a bridge name with %d in it, which we replace
with an unsigned integer to form a bridge name that does not yet exist
on the host.
Do not blindly pass it to virAsprintf if it's not the only conversion,
to prevent crashing on input like:
<network>
<name>test</name>
<forward mode='none'/>
<bridge name='virbr%d%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s'/>
</network>
Ignore any template strings that do not have exactly one %d conversion,
like we do in various drivers before calling virNetDevTapCreateInBridgePort.
Since some people use the same naming convention as libvirt for bridge
devices they create outside the context of libvirt, it is much nicer
if we check for those devices when looking for a bridge device name to
auto-assign to a new network.
We already check that any auto-assigned bridge device name for a
virtual network (e.g. "virbr1") doesn't conflict with the bridge name
for any existing libvirt network (via virNetworkSetBridgeName() in
conf/network_conf.c).
We also want to check that the name doesn't conflict with any bridge
device created on the host system outside the control of libvirt
(history: possibly due to the ploriferation of references to libvirt's
bridge devices in HOWTO documents all around the web, it is not
uncommon for an admin to manually create a bridge in their host's
system network config and name it "virbrX"). To add such a check to
virNetworkBridgeInUse() (which is called by virNetworkSetBridgeName())
we would have to call virNetDevExists() (from util/virnetdev.c); this
function calls ioctl(SIOCGIFFLAGS), which everyone on the mailing list
agreed should not be done from an XML parsing function in the conf
directory.
To remedy that problem, this patch removes virNetworkSetBridgeName()
from conf/network_conf.c and puts an identically functioning
networkBridgeNameValidate() in network/bridge_driver.c (because it's
reasonable for the bridge driver to call virNetDevExists(), although
we don't do that yet because I wanted this patch to have as close to 0
effect on function as possible).
There are a couple of inevitable changes though:
1) We no longer check the bridge name during
virNetworkLoadConfig(). Close examination of the code shows that
this wasn't necessary anyway - the only *correct* way to get XML
into the config files is via networkDefine(), and networkDefine()
will always call networkValidate(), which previously called
virNetworkSetBridgeName() (and now calls
networkBridgeNameValidate()). This means that the only way the
bridge name can be unset during virNetworkLoadConfig() is if
someone edited the config file on disk by hand (which we explicitly
prohibit).
2) Just on the off chance that somebody *has* edited the file by hand,
rather than crashing when they try to start their malformed
network, a check for non-NULL bridge name has been added to
networkStartNetworkVirtual().
(For those wondering why I don't instead call
networkValidateBridgeName() there to set a bridge name if one
wasn't present - the problem is that during
networkStartNetworkVirtual(), the lock for the network being
started has already been acquired, but the lock for the network
list itself *has not* (because we aren't adding/removing a
network). But virNetworkBridgeInuse() iterates through *all*
networks (including this one) and locks each network as it is
checked for a duplicate entry; it is necessary to lock each network
even before checking if it is the designated "skip" network because
otherwise some other thread might acquire the list lock and delete
the very entry we're examining. In the end, permitting a setting of
the bridge name during network start would require that we lock the
entire network list during any networkStartNetwork(), which
eliminates a *lot* of parallelism that we've worked so hard to
achieve (it can make a huge difference during libvirtd startup). So
rather than try to adjust for someone playing against the rules, I
choose to instead give them the error they deserve.)
3) virNetworkAllocateBridge() (now removed) would leak any "template"
string set as the bridge name. Its replacement
networkFindUnusedBridgeName() doesn't leak the template string - it
is properly freed.
Rename it to virNetDevGetIPv4AddressIoctl and make
virNetDevGetIPAddress a wrapper around it, allowing
other ways of getting the address to be implemented,
and still falling back to the old method.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
This function does not make any sense now, that network driver is
(almost) dropped. I mean, previously, when threads were
serialized, this function was there to check, if no other network
with the same name or UUID exists. However, nowadays that threads
can run more in parallel, this function is useless, in fact it
gives misleading return values. Consider the following scenario.
Two threads, both trying to define networks with same name but
different UUID (e.g. because it was generated during XML parsing
phase, whatever). Lets assume that both threads are about to call
networkValidate() which immediately calls
virNetworkObjIsDuplicate().
T1: calls virNetworkObjIsDuplicate() and since no network with
given name or UUID exist, success is returned.
T2: calls virNetworkObjIsDuplicate() and since no network with
given name or UUID exist, success is returned.
T1: calls virNetworkAssignDef() and successfully places its
network into the virNetworkObjList.
T2: calls virNetworkAssignDef() and since network with the same
name exists, the network definition is replaced.
Okay, this is mainly because virNetworkAssignDef() does not check
whether name and UUID matches. Well, lets make it so! And drop
useless function too.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
There's no need to lock the network driver, as network driver
initialization is done prior accepting any client. There's nobody
to hop in and do something over partially initialized driver. Nor
qemu driver is doing that.
==30532== Observed (incorrect) order is: acquisition of lock at 0x1439EF50
==30532== at 0x4C31A26: pthread_mutex_lock (in /usr/lib64/valgrind/vgpreload_helgrind-amd64-linux.so)
==30532== by 0x5324895: virMutexLock (virthread.c:88)
==30532== by 0x5307E86: virObjectLock (virobject.c:323)
==30532== by 0x5396440: virNetworkObjListForEach (network_conf.c:4511)
==30532== by 0x19B29308: networkStateInitialize (bridge_driver.c:686)
==30532== by 0x53E1CCC: virStateInitialize (libvirt.c:777)
==30532== by 0x11DEB7: daemonRunStateInit (libvirtd.c:906)
==30532== by 0x5324B6A: virThreadHelper (virthread.c:197)
==30532== by 0x4C30456: ??? (in /usr/lib64/valgrind/vgpreload_helgrind-amd64-linux.so)
==30532== by 0xA1EC1F2: start_thread (in /lib64/libpthread-2.19.so)
==30532== by 0xA4EDC8C: clone (in /lib64/libc-2.19.so)
==30532==
==30532== followed by a later acquisition of lock at 0x1439CD60
==30532== at 0x4C31A26: pthread_mutex_lock (in /usr/lib64/valgrind/vgpreload_helgrind-amd64-linux.so)
==30532== by 0x5324895: virMutexLock (virthread.c:88)
==30532== by 0x19B27B2C: networkDriverLock (bridge_driver.c:102)
==30532== by 0x19B27B60: networkGetDnsmasqCaps (bridge_driver.c:113)
==30532== by 0x19B2856A: networkUpdateState (bridge_driver.c:389)
==30532== by 0x53963E9: virNetworkObjListForEachHelper (network_conf.c:4488)
==30532== by 0x52E2224: virHashForEach (virhash.c:521)
==30532== by 0x539645B: virNetworkObjListForEach (network_conf.c:4512)
==30532== by 0x19B29308: networkStateInitialize (bridge_driver.c:686)
==30532== by 0x53E1CCC: virStateInitialize (libvirt.c:777)
==30532== by 0x11DEB7: daemonRunStateInit (libvirtd.c:906)
==30532== by 0x5324B6A: virThreadHelper (virthread.c:197)
==30532==
==30532== Required order was established by acquisition of lock at 0x1439CD60
==30532== at 0x4C31A26: pthread_mutex_lock (in /usr/lib64/valgrind/vgpreload_helgrind-amd64-linux.so)
==30532== by 0x5324895: virMutexLock (virthread.c:88)
==30532== by 0x19B27B2C: networkDriverLock (bridge_driver.c:102)
==30532== by 0x19B28DF9: networkStateInitialize (bridge_driver.c:609)
==30532== by 0x53E1CCC: virStateInitialize (libvirt.c:777)
==30532== by 0x11DEB7: daemonRunStateInit (libvirtd.c:906)
==30532== by 0x5324B6A: virThreadHelper (virthread.c:197)
==30532== by 0x4C30456: ??? (in /usr/lib64/valgrind/vgpreload_helgrind-amd64-linux.so)
==30532== by 0xA1EC1F2: start_thread (in /lib64/libpthread-2.19.so)
==30532== by 0xA4EDC8C: clone (in /lib64/libc-2.19.so)
==30532==
==30532== followed by a later acquisition of lock at 0x1439EF50
==30532== at 0x4C31A26: pthread_mutex_lock (in /usr/lib64/valgrind/vgpreload_helgrind-amd64-linux.so)
==30532== by 0x5324895: virMutexLock (virthread.c:88)
==30532== by 0x5307E86: virObjectLock (virobject.c:323)
==30532== by 0x538A09C: virNetworkAssignDef (network_conf.c:527)
==30532== by 0x5391EB2: virNetworkLoadState (network_conf.c:3008)
==30532== by 0x53922D4: virNetworkLoadAllState (network_conf.c:3128)
==30532== by 0x19B2929A: networkStateInitialize (bridge_driver.c:671)
==30532== by 0x53E1CCC: virStateInitialize (libvirt.c:777)
==30532== by 0x11DEB7: daemonRunStateInit (libvirtd.c:906)
==30532== by 0x5324B6A: virThreadHelper (virthread.c:197)
==30532== by 0x4C30456: ??? (in /usr/lib64/valgrind/vgpreload_helgrind-amd64-linux.so)
==30532== by 0xA1EC1F2: start_thread (in /lib64/libpthread-2.19.so)
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
The following is a long winded way to say this patch is avoiding a
false positive.
Coverity complains that calling networkPlugBandwidth() could eventually
end up with a NULL dereference on iface->bandwidth because in the
networkAllocateActualDevice there's a check of 'iface->bandwidth'
before deciding to try to use the 'portgroup' if it exists or to not
perferm the virNetDevBandwidthCopy if 'bandwidth' is not NULL.
Later in networkPlugBandwidth the 'iface->bandwidth' is sourced from
virDomainNetGetActualBandwidth - which would be either iface->bandwidth
or (preferably) iface->data.network.actual->bandwidth which would have
been filled in from either 'iface->bandwidth' or 'portgroup->bandwidth'
back in networkAllocateActualDevice
There *is* a check in networkCheckBandwidth for the result of the
virDomainNetGetActualBandwidth being NULL and a return 1 based on
that which would cause networkPlugBandwidth to exit properly and thus
never hit the condition that Coverity complains about.
However, since Coverity checks all paths - it somehow believes that
a return of 0 by networkCheckBandwidth in this condition would end
up causing the possible NULL dereference. The "fix" to silence Coverity
is to not have networkCheckBandwidth also call virDomainNetGetActualBandwidth
in order to get the ifaceBand, but rather have it accept it as an argument
which causes Coverity to "see" that it's the exit condition of 1 that won't
have the possible NULL dereference. Since we're passing that, I added the
passing of iface->mac rather than passing iface as well. This just hopefully
makes sure someone doesn't undo this in the future...
Valgrind detected a leak:
==17820== 102 (56 direct, 46 indirect) bytes in 1 blocks are definitely lost in loss record 479 of 646
==17820== at 0x4A08946: calloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==17820== by 0x508521A: virAllocVar (viralloc.c:560)
==17820== by 0x50D9FCA: virObjectNew (virobject.c:193)
==17820== by 0x50A4FD9: dnsmasqCapsNewEmpty (virdnsmasq.c:784)
==17820== by 0x50A514E: dnsmasqCapsNewFromBinary (virdnsmasq.c:830)
==17820== by 0x1B508287: networkStateInitialize (bridge_driver.c:666)
It looks like commit 172acef introduced the problem, because
networkGetDnsmasqCaps() increments the reference count but an
early exit never does a matching decrement.
* src/network/bridge_driver.c (networkStateCleanup): Plug leak.
Signed-off-by: Eric Blake <eblake@redhat.com>
Now that the network driver lock is ash heap of history,
we can use more of networkObjFromNetwork().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Now that we have fine grained locks, there's no need to
lock the whole driver. We can rely on self-locking APIs.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
This is not an immutable pointer and can change during lifetime.
Therefore, in order to drop network driver lock, we must use an
internal accessor which does not lock the network driver yet, but
it will soon. Now it merely returns an referenced object.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Well, network driver code has the driver accessible as a global
variable. This makes any rework hard, as it's unclear where the
variable is accessed and/or modified. Lets just pass the driver
as a parameter to all functions where needed.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
This patch turns both virNetworkObjFindByUUID() and
virNetworkObjFindByName() to return an referenced object so that
even if caller unlocks it, it's for sure that object won't
disappear meanwhile. Especially if the object (in general) is
locked and unlocked during the caller run.
Moreover, this commit is nicely small, since the object unrefing
can be done in virNetworkObjEndAPI().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
So far, this is pure code replacement. But once we introduce
reference counting to virNetworkObj this will be more handy as
there'll be only one function to change: virNetworkObjEndAPI().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
So far it's just a structure which happens to have 'Obj' in its
name, but otherwise it not related to virObject at all. No
reference counting, not virObjectLock(), nothing.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Undefining a running, autostarted domain removes the autostart link, but
dom->autostart is not cleared. If the domain is subsequently redefined,
libvirt thinks it is already autostarted and will not create the link
even if requested:
# virsh dominfo example | grep Autostart
Autostart: enable
# ls /etc/libvirt/qemu/autostart/example.xml
/etc/libvirt/qemu/autostart/example.xml
# virsh undefine example
Domain example has been undefined
# virsh define example.xml
Domain example defined from example.xml
# virsh dominfo example | grep Autostart
Autostart: enable
# virsh autostart example
Domain example marked as autostarted
# ls /etc/libvirt/qemu/autostart/example.xml
ls: cannot access /etc/libvirt/qemu/autostart/example.xml: No such file or directory
This commit ensures dom->autostart is cleared whenever the config and
autostart link (if present) are removed.
The bridge network driver cleared this flag itself in networkUndefine.
This commit moves this into virNetworkDeleteConfig for symmetry with
virDomainDeleteConfig, and to ensure it is not missed in future network
drivers.
Signed-off-by: Michael Chapman <mike@very.puzzling.org>
Well, one day this will be self-locking object, but not today.
But lets prepare the code for that! Moreover,
virNetworkObjListFree() is no longer needed, so turn it into
virNetworkObjListDispose().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
In order to hide the object internals (and use just accessors
everywhere), lets store a pointer to the object, instead of object
itself.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Instead of copying the whole object onto stack when calling the
function, just pass the pointer to the object and save up some
space on the stack. Moreover, this prepares the code to hide the
virNetworkObjList structure into network_conf.c and use accessors
only.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Moreover, there are two points within the function, where we're
missing 'goto cleanup'. Fix this too.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Okay, this is mainly for educational purposes since is called
from single point only with all the possible locks held. So
there's no way for other thread to hop in and do something wrong.
Nevertheless, we should not give bad example.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
We have this function networkObjFromNetwork() which for given
virNetworkPtr tries to find corresponding virNetworkObjPtr. If no
object is found, a nice error message is printed out:
no network with matching uuid '$uuid' ($name)
Let's improve the error message produced by networkLookupByUUID to
follow that logic.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
libvirt was unconditionally calling virNetDevBandwidthClear() for
every interface (and network bridge) of a type that supported
bandwidth, whether it actually had anything set or not. This doesn't
hurt anything (unless ifname == NULL!), but is wasteful.
This patch makes sure that all calls to virNetDevBandwidthClear() are
qualified by checking that the interface really had some bandwidth
setup done, and checks for a null ifname inside
virNetDevBandwidthClear(), silently returning success if it is null
(as well as removing the ATTRIBUTE_NONNULL from that function's
prototype, since we can't guarantee that it is never null,
e.g. sometimes a type='ethernet' interface has no ifname as it is
provided on the fly by qemu).
The function that parses the <forward> subelement of a network used to
fail/log an error if the network definition contained both a <pf>
element as well as at least one <interface> or <address> element. That
check was present because the configuration of a network should have
either one <pf>, one or more <interface>, or one or more <address>,
but never combinations of multiple kinds.
This caused a problem when libvirtd was restarted with a network
already active - when a network with a <pf> element is started, the
referenced PF (Physical Function of an SRIOV-capable network card) is
checked for VFs (Virtual Functions), and the <forward> is filled in
with a list of all VFs for that PF either in the form of their PCI
addresses (a list of <address>) or their netdev names (a list of
<interface>); the <pf> element is not removed though. When libvirtd is
restarted, it parses the network status and finds both the original
<pf> from the config, as well as the list of either <address> or
<interface>, fails the parse, and the network is not added to the
active list. This failure is often obscured because the network is
marked as autostart so libvirt immediately restarts it.
It seems odd to me that <interface> and <address> are stored in the
same array rather than keeping two separate arrays, and having
separate arrays would have made the check much simpler. However,
changing to use two separate arrays would have required changes in
more places, potentially creating more conflicts and (more
importantly) more possible regressions in the event of a backport, so
I chose to keep the existing data structure in order to localize the
change.
It appears that this problem has been in the code ever since support
for <pf> was added (0.9.10), but until commit
34cc3b2f10 (first in libvirt 1.2.4)
networks with interface pools were not properly marked as active on
restart anyway, so there is no point in backporting this patch any
further than that.
Not all files we want to find using virFileFindResource{,Full} are
generated when libvirt is built, some of them (such as RNG schemas) are
distributed with sources. The current API was not able to find source
files if libvirt was built in VPATH.
Both RNG schemas and cpu_map.xml are distributed in source tarball.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
When defining and creating networks, we have been checking to make
sure there is only a single "default" portgroup, but haven't verified
that no two portgroups have the same name. We *do* check for multiple
definitions when updating the portgroups in an existing network
though.
This patch adds a check to networkValidate(), which is called when a
network is defined or created, to disallow duplicate names. It would
actually make sense to do this in the network XML parser (since it's
not really "something that might make sense but isn't supported by
this driver", but is instead "something that should never be
allowed"), but doing that carries the danger of causing errors when
rereading the config of existing networks when libvirtd is restarted
after an upgrade, and that would result in networks disappearing from
libvirt's list. (I'm thinking I should change the error to "XML_ERROR"
instead of "UNSUPPORTED", even though that's not the type of error
that networkValidate is intended for)
This resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1115858
For stateless, client side drivers, it is never correct to
probe for secondary drivers. It is only ever appropriate to
use the secondary driver that is associated with the
hypervisor in question. As a result the ESX & HyperV drivers
have both been forced to do hacks where they register no-op
drivers for the ones they don't implement.
For stateful, server side drivers, we always just want to
use the same built-in shared driver. The exception is
virtualbox which is really a stateless driver and so wants
to use its own server side secondary drivers. To deal with
this virtualbox has to be built as 3 separate loadable
modules to allow registration to work in the right order.
This can all be simplified by introducing a new struct
recording the precise set of secondary drivers each
hypervisor driver wants
struct _virConnectDriver {
virHypervisorDriverPtr hypervisorDriver;
virInterfaceDriverPtr interfaceDriver;
virNetworkDriverPtr networkDriver;
virNodeDeviceDriverPtr nodeDeviceDriver;
virNWFilterDriverPtr nwfilterDriver;
virSecretDriverPtr secretDriver;
virStorageDriverPtr storageDriver;
};
Instead of registering the hypervisor driver, we now
just register a virConnectDriver instead. This allows
us to remove all probing of secondary drivers. Once we
have chosen the primary driver, we immediately know the
correct secondary drivers to use.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
This adds a new "localOnly" attribute on the domain element of the
network xml. With this set to "yes", DNS requests under that domain
will only be resolved by libvirt's dnsmasq, never forwarded upstream.
This was how it worked before commit f69a6b987d, and I found that
functionality useful. For example, I have my host's NetworkManager
dnsmasq configured to forward that domain to libvirt's dnsmasq, so I can
easily resolve guest names from outside. But if libvirt's dnsmasq
doesn't know a name and forwards it to the host, I'd get an endless
forwarding loop. Now I can set localOnly="yes" to prevent the loop.
Signed-off-by: Josh Stone <jistone@redhat.com>
Moving code for parsing and formatting network routes to
networkcommon_conf helps reusing those routes for domains. The route
definition has been hidden to help reducing the number of unnecessary
checks in the format function.
Lack of a lease (whether mac is given or not) is a normal expected
scenario, since we are already filling in rv with nleases (which is
okay as 0 if there is no lease). There is no need to raise an error.
This fixes:
> virsh # net-dhcp-leases --mac 00:50:56:c0:00:01 default
> error: Failed to get leases info for default
> error: internal error: no lease with matching MAC address: 00:50:56:c0:00:01
Signed-off-by: Nehal J Wani <nehaljw.kkd1@gmail.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
When the bridge device for a network has macTableManager='libvirt' the
intent is that all kernel management of the bridge's MAC table
(Forwarding Database, or fdb, in the case of a Linux Host Bridge) be
disabled, with libvirt handling updates to the table instead. The
setup required for the bridge itself is:
1) set the "vlan_filtering" property of the bridge device to 1.
2) If the bridge has a "Dummy" tap device used to set a fixed MAC
address on the bridge (which is always the case for a bridge created
by libvirt, and never the case for a bridge created by the host system
network config), turn off learning and unicast_flood on this tap (this
is needed even though this tap is never IFF_UP, because the kernel
ignores the IFF_UP flag of devices when using their settings to
automatically decide whether or not to turn off promiscuous mode for
any attached device).
(1) is done both for libvirt-created/managed bridges, and for bridges
that are created by the host system config, while (2) is done only for
bridges created by libvirt (i.e. for forward modes of nat, routed, and
isolated bridges)
There is no attempt to turn vlan_filtering off when destroying the
network because in the case of a libvirt-created bridge, the bridge is
about to be destroyed anyway, and in the case of a system bridge, if
the other devices attached to the bridge could operate properly before
destroying libvirt's network object, they will continue to operate
properly (this is similar to the way that libvirt will enable
ip_forwarding whenever a routed/natted network is started, but will
never attempt to disable it if they are stopped).
