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: NJohn 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.

We've been explicitly requesting IPv4 for some reason,
even if there were only IPv6 addresses in the network
definition.

https://bugzilla.redhat.com/show_bug.cgi?id=1192318

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: NJohn Ferlan <jferlan@redhat.com>
Signed-off-by: NJá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: NMichal 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: NMichal 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 172acef4 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: NEric Blake <eblake@redhat.com>

Now that the network driver lock is ash heap of history,
we can use more of networkObjFromNetwork().
Signed-off-by: NMichal 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: NMichal 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: NMichal 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: NMichal 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: NMichal 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: NMichal 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: NMichal 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: NMichael Chapman <mike@very.puzzling.org>

Instead of finding the next clear bit by calling virBitmapGetBit
in a loop, use the virBitmapNextClearBit helper.

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: NMichal Privoznik <mprivozn@redhat.com>

Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

It's returning virNetworkObjPtr after all. And it matches the
pattern laid out by domain_conf.h.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

It's returning virNetworkObjPtr after all. And it matches the
pattern laid out by domain_conf.h.
Signed-off-by: NMichal 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: NMichal 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: NMichal Privoznik <mprivozn@redhat.com>

Moreover, there are two points within the function, where we're
missing 'goto cleanup'. Fix this too.
Signed-off-by: NMichal 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: NMichal 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: NMichal 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
34cc3b2f (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: NJiri 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: NDaniel 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 f69a6b98, 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: NJosh 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: NNehal J Wani <nehaljw.kkd1@gmail.com>
Signed-off-by: NEric Blake <eblake@redhat.com>

Renamed virNetDevSetIPv4Address as it also handles IPv6 addresses.

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).