- Devel - libvirt List Archives

[libvirt] [PATCH] network: prevent a few invalid configuration combinations
by Laine Stump 06 Dec '12

06 Dec '12

This resolves: https://bugzilla.redhat.com/show_bug.cgi?id=767057 It was possible to define a network with <forward mode='bridge'> that had both a bridge device and a forward device defined. These two are mutually exclusive by definition (if you are using a bridge device, then this is a host bridge, and if you have a forward dev defined, this is using macvtap). It was also possible to put <ip>, <dns>, and <domain> elements in this definition, although those aren't supported by the current driver (although it's conceivable that some other driver might support that). The items that are invalid by definition, are now checked in the XML parser (since they will definitely *always* be wrong), and the others are checked in networkValidate() in the network driver (since, as mentioned, it's possible that some other network driver, or even this one, could some day support setting those). --- src/conf/network_conf.c | 9 +++++++++ src/libvirt_private.syms | 1 + src/network/bridge_driver.c | 29 +++++++++++++++++++++++++++++ 3 files changed, 39 insertions(+) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 6ce2e63..06932d8 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -1577,6 +1577,15 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) def->name); goto error; } + if (def->bridge && (def->nForwardIfs || nForwardPfs)) { + virReportError(VIR_ERR_XML_ERROR, + _("A network with forward mode='%s' can specify " + "a bridge name or a forward dev, but not " + "both (network '%s')"), + virNetworkForwardTypeToString(def->forwardType), + def->name); + goto error; + } break; } } diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 946bb20..bc01fe5 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -857,6 +857,7 @@ virNetworkDefParseString; virNetworkDeleteConfig; virNetworkFindByName; virNetworkFindByUUID; +virNetworkForwardTypeToString; virNetworkIpDefNetmask; virNetworkIpDefPrefix; virNetworkList; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index e8be00a..0893e9b 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2733,6 +2733,35 @@ networkValidate(struct network_driver *driver, return -1; virNetworkSetBridgeMacAddr(def); + } else { + /* They are also the only types that currently support setting + * an IP address for the host-side device (bridge) + */ + if (virNetworkDefGetIpByIndex(def, AF_UNSPEC, 0)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Unsupported <ip> element in network %s " + "with forward mode='%s'"), + def->name, + virNetworkForwardTypeToString(def->forwardType)); + return -1; + } + if (def->dns && + (def->dns->ntxtrecords || def->dns->nhosts || def->dns->nsrvrecords)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Unsupported <dns> element in network %s " + "with forward mode='%s'"), + def->name, + virNetworkForwardTypeToString(def->forwardType)); + return -1; + } + if (def->domain) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Unsupported <domain> element in network %s " + "with forward mode='%s'"), + def->name, + virNetworkForwardTypeToString(def->forwardType)); + return -1; + } } /* We only support dhcp on one IPv4 address per defined network */ -- 1.7.11.7

3 3

[libvirt] [PATCH] maint: Sort util source file definitions in makefile
by Peter Krempa 06 Dec '12

06 Dec '12

--- src/Makefile.am | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/Makefile.am b/src/Makefile.am index 1a2f94f..9aff77a 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -54,44 +54,37 @@ augeastest_DATA = UTIL_SOURCES = \ util/bitmap.c util/bitmap.h \ util/buf.c util/buf.h \ + util/cgroup.c util/cgroup.h \ util/command.c util/command.h \ util/conf.c util/conf.h \ - util/cgroup.c util/cgroup.h \ + util/dnsmasq.c util/dnsmasq.h \ + util/ebtables.c util/ebtables.h \ util/event.c util/event.h \ util/event_poll.c util/event_poll.h \ util/hooks.c util/hooks.h \ + util/hostusb.c util/hostusb.h \ util/iptables.c util/iptables.h \ - util/ebtables.c util/ebtables.h \ - util/dnsmasq.c util/dnsmasq.h \ util/json.c util/json.h \ util/logging.c util/logging.h \ util/memory.c util/memory.h \ util/pci.c util/pci.h \ util/processinfo.c util/processinfo.h \ - util/hostusb.c util/hostusb.h \ util/sexpr.c util/sexpr.h \ util/stats_linux.c util/stats_linux.h \ util/storage_file.c util/storage_file.h \ util/sysinfo.c util/sysinfo.h \ - util/threads.c util/threads.h \ + util/threadpool.c util/threadpool.h \ util/threads-pthread.h \ util/threads-win32.h \ - util/threadpool.c util/threadpool.h \ - util/uuid.c util/uuid.h \ + util/threads.c util/threads.h \ util/util.c util/util.h \ + util/uuid.c util/uuid.h \ util/viratomic.h util/viratomic.c \ util/viraudit.c util/viraudit.h \ util/virauth.c util/virauth.h \ util/virauthconfig.c util/virauthconfig.h \ - util/virfile.c util/virfile.h \ - util/virnodesuspend.c util/virnodesuspend.h \ - util/virobject.c util/virobject.h \ - util/virpidfile.c util/virpidfile.h \ - util/virprocess.c util/virprocess.h \ - util/virtypedparam.c util/virtypedparam.h \ - util/xml.c util/xml.h \ - util/virterror.c util/virterror_internal.h \ util/virdbus.c util/virdbus.h \ + util/virfile.c util/virfile.h \ util/virhash.c util/virhash.h \ util/virhashcode.c util/virhashcode.h \ util/virinitctl.c util/virinitctl.h \ @@ -110,11 +103,18 @@ UTIL_SOURCES = \ util/virnetdevvlan.h util/virnetdevvlan.c \ util/virnetdevvportprofile.h util/virnetdevvportprofile.c \ util/virnetlink.c util/virnetlink.h \ + util/virnodesuspend.c util/virnodesuspend.h \ + util/virobject.c util/virobject.h \ + util/virpidfile.c util/virpidfile.h \ + util/virprocess.c util/virprocess.h \ util/virrandom.h util/virrandom.c \ util/virsocketaddr.h util/virsocketaddr.c \ util/virstring.h util/virstring.c \ + util/virterror.c util/virterror_internal.h \ util/virtime.h util/virtime.c \ - util/viruri.h util/viruri.c + util/virtypedparam.c util/virtypedparam.h \ + util/viruri.h util/viruri.c \ + util/xml.c util/xml.h EXTRA_DIST += $(srcdir)/util/virkeymaps.h $(srcdir)/util/keymaps.csv \ $(srcdir)/util/virkeycode-mapgen.py -- 1.8.0

2 2

[libvirt] [PATCH] dnsmasq: Fix parsing of the version number
by Michal Privoznik 06 Dec '12

06 Dec '12

If debugging is enabled, the debug messages are sent to stderr. Moreover, if a command has catching of stderr set, the messages gets mixed with stdout output (assuming both outputs are stored in the same variable). The resulting string then doesn't necessarily have to start with desired prefix then. This bug exposes itself when parsing dnsmasq output: 2012-12-06 11:18:11.445+0000: 18491: error : dnsmasqCapsSetFromBuffer:664 : internal error cannot parse /usr/sbin/dnsmasq version number in '2012-12-06 11:11:02.232+0000: 18492: debug : virFileClose:72 : Closed fd 22' We can clearly see that the output of dnsmasq --version doesn't start with expected "Dnsmasq version " string but a libvirt debug output. --- src/util/dnsmasq.c | 2 -- 1 files changed, 0 insertions(+), 2 deletions(-) diff --git a/src/util/dnsmasq.c b/src/util/dnsmasq.c index de0293a..a627ed2 100644 --- a/src/util/dnsmasq.c +++ b/src/util/dnsmasq.c @@ -715,7 +715,6 @@ dnsmasqCapsRefreshInternal(dnsmasqCapsPtr caps, bool force) cmd = virCommandNewArgList(caps->binaryPath, "--version", NULL); virCommandSetOutputBuffer(cmd, &version); - virCommandSetErrorBuffer(cmd, &version); virCommandAddEnvPassCommon(cmd); virCommandClearCaps(cmd); if (virCommandRun(cmd, NULL) < 0) { @@ -727,7 +726,6 @@ dnsmasqCapsRefreshInternal(dnsmasqCapsPtr caps, bool force) cmd = virCommandNewArgList(caps->binaryPath, "--help", NULL); virCommandSetOutputBuffer(cmd, &help); - virCommandSetErrorBuffer(cmd, &help); virCommandAddEnvPassCommon(cmd); virCommandClearCaps(cmd); if (virCommandRun(cmd, NULL) < 0) { -- 1.7.8.6

2 1

[libvirt] [PATCH 0/4] qemu: Fix leaks in handling PCI devices
by Jiri Denemark 06 Dec '12

06 Dec '12

https://bugzilla.redhat.com/show_bug.cgi?id=877095 Patch 1/4 fixes a possible double free on error path and the other patches deal with memory and file descriptor leaks in PCI device attachment code. Jiri Denemark (4): qemu: Don't free PCI device if adding it to activePciHostdevs fails util: Slightly refactor PCI list functions qemu: Fix memory (and FD) leak on PCI device detach qemu: Do not keep PCI device config file open src/libvirt_private.syms | 3 +++ src/qemu/qemu_hostdev.c | 22 ++++++++-------- src/util/pci.c | 66 ++++++++++++++++++++++++++++-------------------- src/util/pci.h | 5 ++++ 4 files changed, 58 insertions(+), 38 deletions(-) -- 1.8.0

7 20

[libvirt] virSecurity hook for hugepages?
by Serge Hallyn 05 Dec '12

05 Dec '12

Hi, Currently the hugepages support can automatically detect the hugepages mount, but it doesn't update the security information. At least for apparmor we need to be able to add permission for the domain to access the hugetlbfs mount path. There are a few ways this could be done, 1. add a virSecuritySetSecurityHugepages or virSecuritySetSecurityHugepagesFD hook which is called perhaps at qemudStartup 2. optionally add the qemu_driver->hugepage_path to the xml output, at least for the internal format (which is passed to virt-aa-helper). The concern I have with this is that it brings up the issue of what to do when defining a domain which has such an entry. 3. reproduce the logic in virt-aa-helper for detecting the hugepages mount path. Not preferred obviously. My guess would be that (1) would be preferred, but I wanted to ask here first and see if there are other suggestions. thanks, -serge

1 1

[libvirt] [PATCH] storage: Error out earlier if the volume target path already exists
by Osier Yang 05 Dec '12

05 Dec '12

https://bugzilla.redhat.com/show_bug.cgi?id=832302 It's odd to fall through to buildVol, and the existed file is removed when buildVol fails. This checks if the volume target path already exists in createVol. The reason for not using error like "Volume already exists" is that there isn't volume maintained by libvirt for the path until a operation like pool-refresh, using error like that will just cause confusion. --- BTW, It inspires me again that perhaps we should integrate things like inotify to storage. --- src/storage/storage_backend_fs.c | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 4e6ebbf..6cddad0 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -1000,6 +1000,13 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn ATTRIBUTE_UNUSED, return -1; } + if (virFileExists(vol->target.path)) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("volume target path '%s' already exists"), + vol->target.path); + return -1; + } + VIR_FREE(vol->key); vol->key = strdup(vol->target.path); if (vol->key == NULL) { -- 1.7.7.6

2 4

[libvirt] [PATCH 0/2] Avoid the thread race condition
by Osier Yang 05 Dec '12

05 Dec '12

https://bugzilla.redhat.com/show_bug.cgi?id=866524 The two patches are to fix the same bug in different method, 1/2 is suggested by Daniel in the bug. 2/2 is an alternative way to fix it by locking the whole virConnect object. The reason to post 2/2 is that I think 1/2 doesn't fix the root problem, though I believe it must fix the bug. A thread can get the lock before/during doRemoteClose, and it might drop into the similar situation, though it's not the case now because remoteClientCloseFunc doesn't do changes which can cause race for the later doRemoteClose. However, I'm not sure if locking the whole object will cause some unexpected result. Osier Yang (2): remote: Avoid the thread race condition Lock the whole virConnect Object when disposing to avoid the thread race src/datatypes.c | 4 ++-- src/remote/remote_driver.c | 4 ++++ 2 files changed, 6 insertions(+), 2 deletions(-) Regards, Osier

2 5

Re: [libvirt] [libvirt-users] NWFilter and IPv6
by Laine Stump 05 Dec '12

05 Dec '12

(No extra content from me, but I'm setting Followup-To: libvir-list(a)redhat.com for this (and setting To: to the same), since this is talking about new development, so we want to make sure as many developers as possible see it...) On 12/05/2012 09:23 AM, Guido Winkelmann wrote: > Am Dienstag, 4. Dezember 2012, 19:18:01 schrieb Stefan Berger: >> On 12/04/2012 09:39 AM, Guido Winkelmann wrote: >>> Am Montag, 26. November 2012, 12:24:11 schrieb Stefan Berger: >>>> On 11/26/2012 10:41 AM, Laine Stump wrote: > [...] >>>> One problem I want to mention, though: A bigger problem would be if a >>>> machine wanted to use IPv4 and IPv6 (dual stack) and use DHCP for both , >>>> which in effect would result in two variables that need to have values >>>> detected which in turn would require partial instantiation of filters >>>> (since one variable may not have a value assigned while the other has), >>>> which does not currently work... >>> Hm, how do you even do it with one variable? Do you leave the firewall >>> undefined until you could detect the dhcp-answer package and then pull it >>> up? >> We assume that DHCP is being used and for example put a filter in that >> only allows DHCP traffic to pass and once we grab the IP address we >> instantiate the user-provided filter. For that we use $IP. The variable >> is set once the IP address has been detected. For IPv6 we should >> probably use $IPV6 (reserved variable). > How do you control this behavior? Can you just set the $IP to a value in the > filterref instantiation to disable it? Like so: > > <filterref filter='clean-traffic-with-v6'>" > <parameter name='MAC' value='11:11:11:11:11:11'/>"; > <parameter name='IP' value='192.168.0.10'/>"; > <parameter name='IP' value='192.168.0.11'/>"; > </filterref>" > > Anyway, I think combining DHCP and DHCPv6 is going to be a minor problem in > practice, because most people will probably use stateless autoconfiguration to > set the IPv6 address on a device and use DHCPv6 only for additional > information, like DNS servers or NTP servers. > > How about this approach instead for combining DHCP and DHCPv6: > > - Instead of putting up a special network filter for the detection phase, we > put up the actual user-requested filter, but with $IP and $IPV6 unset. (except > of course when these variables are specifically set by the user as above...) > - The default-shipped filters like clean-traffic should let DHCP(v6) through > in this configuration. If the user-requested filters don't, that's just a > configuration error. > - As soon as we detect an incoming DHCP or DHCPv6 packet for the guest, we add > that address to the filter parameters, reinitialize the filter with the new > parameters and stop detecting addresses for this particular L3 protocol. > >>>> Also as I recall for IPv4 the ARP-equivalent is NDP (Neighbor Discovery >>>> Protocol based on ICMPv6), which may need support in ebtables. At least >>>> a while ago there was no support for filtering that NDP subset of ICMPv6 >>>> in ebtables. >>> According to the ebtables man-page, you've got --ip6-icmp-type, which >>> should be enough for this. Router advertisements have ICMPv6 type 134 and >>> multicast router advertisements are 153. AFAICT, you can just filter by >>> those... >> I am not the expert on IPv6, but from reading on this page here >> >> http://www.tcpipguide.com/free/t_ICMPv6NeighborAdvertisementandNeighborSolic >> itation-2.htm > BTW, I wouldn't recommend this particular guide. Not only is it cluttered with > advertisements to the point of a major annoyance, there's at least one part > where it has simply plain wrong information: On > > http://www.tcpipguide.com/free/t_IPv6InterfaceIdentifiersandPhysicalAddress… > > Bit 7 of the EUI-64 address needs to be flipped, not just set to 1. (It took > me a while to figure out why my code would arrvive at different autoconfigured > addresses than the linux kernel for virtualized machines...) > >> I get the impression that for example the target address should be >> verified for possible 'abuse'. > That's only for neighbor advertisements. Router advertisements can simply be > blocked wholesale. Normal network nodes have no business sending those under > any circumstances, and your actual routers are hopefully trusted enough to not > need their router advertisements checked for sanity... > > Then again, for neighbor advertisements, you're right. I was under the > impression that, for those, it was enough to check the source address in the > ipv6 header. Apparently I was wrong. > >> I don't think one can grab that field >> with ebtables and compare against allowed values. > No, but ip6tables has --u32, which possibly could be abused for that... > > Guido > >

1 0

[libvirt] Sophie's choice: corrupt user data (on USB disk) or cancel migration?
by David Jaša 05 Dec '12

05 Dec '12

Hi, I've encountered an interesting problem: I did a live migration through libvirt (virsh migrate --live <domain> <dst_libvirt_uri> [<dst_qemu_uri>]) with usb transfer ongoing from qemu guest through usbredir over spice to usb device plugged to the client. The spice client failed to connect to the destination qemu/spice but the migration was conducted anyway, so the data transfer was abruptly terminated. Background ---------- When performing migration with client connected, two monitor commands are issued to the qemu: (qemu) client_migrate_info spice <client connection details> main_channel_client_handle_migrate_connected: client <ID> connected: 1 seamless: 1 (qemu) migrate -d tcp:<dst_qemu_address>:<port> The first command instructs spice client to reconnect to the new machine and the reply says that if connection is successful and if it is seamless (required for migration with USB redirection active). Problem ------- The behaviour should follow "least harm" principle: 1) In "severe" scenarios when the choice is between VM migration causing potential data corruption on redirected USB and stopping of the VM, current behavior is perfectly fine 2) In "regular" scenarios (automatic load balancing, manual migration requests by administrator), refuse to migrate (unless some "I really know what I'm doing" button is pressed) is better thing to do Possible solutions? ------------------- Just one possible way to fix this occured to me: 1. libvirt gets new switch to migrate command to force migration even when client can not connect 2. if this switch is not set and client won't connect after client_migrate_info qemu command is issued, cance migration 3. management application (ovirt-engine through vdsm) or administrator sets the switch to force migration in cases like 1) above I'll file the necessary bugs but it should be IMO first agreed what the correct approach is in such situations. David -- David Jaša, RHCE SPICE QE based in Brno GPG Key: 22C33E24 Fingerprint: 513A 060B D1B4 2A72 7F0D 0278 B125 CD00 22C3 3E24

1 0

[libvirt] [PATCH] pci: Fix building of 32bit PCI command array
by Peter Krempa 05 Dec '12

05 Dec '12

The pciWrite32 function assembled the array of data to be written to the fd with a bad offset on the last byte. This issue was probably caused by a typo (14, 24). --- src/util/pci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/pci.c b/src/util/pci.c index cf7722d..418c83d 100644 --- a/src/util/pci.c +++ b/src/util/pci.c @@ -268,7 +268,7 @@ pciWrite16(pciDevice *dev, int cfgfd, unsigned pos, uint16_t val) static void pciWrite32(pciDevice *dev, int cfgfd, unsigned pos, uint32_t val) { - uint8_t buf[4] = { (val >> 0), (val >> 8), (val >> 16), (val >> 14) }; + uint8_t buf[4] = { (val >> 0), (val >> 8), (val >> 16), (val >> 24) }; pciWrite(dev, cfgfd, pos, &buf[0], sizeof(buf)); } -- 1.8.0

2 2