Re: [libvirt] [PATCH v3] network: bridge: Don't start network if it collides with host routing

Cole Robinson wrote: > Fedora bug https://bugzilla.redhat.com/show_bug.cgi?id=235961 > > If using the default virtual network, an easy way to lose guest network > connectivity is to install libvirt inside the VM. The autostarted > default network inside the guest collides with host virtual network > routing. This is a long standing issue that has caused users quite a > bit of pain and confusion. > > On network startup, parse /proc/net/route and compare the requested > IP+netmask against host routing destinations: if any matches are found, > refuse to start the network. > > v2: Drop sscanf, fix a comment typo, comment that function could use > libnl instead of /proc > > v3: Consider route netmask. Compare binary data rather than convert to > string. ... > +static int networkCheckRouteCollision(virNetworkObjPtr network) > +{ > + int ret = -1, len; > + char *cur, *buf = NULL; > + enum {MAX_ROUTE_SIZE = 1024*64}; > + struct in_addr inaddress, innetmask; > + char netaddr[32]; > + > + if (!network->def->ipAddress || !network->def->netmask) > + return 0; > + > + if (inet_pton(AF_INET, network->def->ipAddress, &inaddress) <= 0) { > + networkReportError(VIR_ERR_INTERNAL_ERROR, > + _("cannot parse IP address '%s'"), > + network->def->ipAddress); > + goto error; > + } > + if (inet_pton(AF_INET, network->def->netmask, &innetmask) <= 0) { > + networkReportError(VIR_ERR_INTERNAL_ERROR, > + _("cannot parse netmask '%s'"), > + network->def->netmask); > + goto error; > + } > + > + inaddress.s_addr &= innetmask.s_addr; > + if (!inet_ntop(AF_INET, &inaddress, netaddr, sizeof(netaddr))) { > + virReportSystemError(errno, "%s", > + _("failed to format network address")); > + goto error; > + } > + > + /* Read whole routing table into memory */ > + if ((len = virFileReadAll(PROC_NET_ROUTE, MAX_ROUTE_SIZE, &buf)) < 0) > + goto error; > + /* Dropping the last character shouldn't hurt */ Hi Cole, Not sure how much you want to invest in manual parsing, but in case this code ends up staying with us for a while, here are some suggestions. Handle the case in which the file is empty. This will appease static checkers like clang and coverity. Either change the "< 0" test above to "<= 0" or do e.g., if (len > 0) > + buf[len-1] = '\0'; Future proof it by skipping that first line only if it looks like the heading we see now: if (STRPREFIX (buf, "Iface"))