[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On 05/27/2010 04:54 AM, Jim Meyering wrote:
> 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"))
> 

Thanks for the review, I incorporated the above recommendations.
However, I went back to using sscanf which greatly simplifies the
parsing, and is used in a safe manner AFAICT. Updated patch coming shortly.

Thanks,
Cole


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]