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

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



On 05/27/2010 04:04 PM, 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.
> ...
>> v4: Return to using sscanf, drop inet functions in favor of virSocket,
>>     parsing safety checks. Don't make parse failures fatal, in case
>>     expected format changes.
> ...
> 
> Hi Cole,
> 
> This is better indeed.  Thanks.
> 
>> +    while (cur) {
>> +        char dest[128], iface[17], mask[128];
> 
> Please reorder to match the expected order/use below:
> 
>            char iface[17], dest[128], mask[128];
> 
>> +        unsigned int addr_val, mask_val;
>> +        int num;
>> +
>> +        cur++;
>> +        num = sscanf(cur, "%16s %127s %*s %*s %*s %*s %*s %127s",
>> +                     iface, dest, mask);
> 
> This is fine, except when there are fewer than 8 columns.
> When that happens, it will take additional items from the next line.
> You can avoid that by NUL-terminating the line.  i.e.,
> put this just before the sscanf:
> 
>            /* NUL-terminate the line, so sscanf doesn't go beyond a newline.  */
>            char *nl = strchr(cur, '\n');
>            if (nl)
>                *nl = '\0';
> 
> 
>> +        if (num != 3) {
>> +            VIR_DEBUG("Failed to parse %s", PROC_NET_ROUTE);
>> +            break;
>> +        }
>> +
>> +        if (virStrToLong_ui(dest, NULL, 16, &addr_val) < 0) {
>> +            VIR_DEBUG("Failed to convert network address %s to uint", dest);
>> +            break;
>> +        }
>> +
>> +        if (virStrToLong_ui(mask, NULL, 16, &mask_val) < 0) {
>> +            VIR_DEBUG("Failed to convert network mask %s to uint", mask);
>> +            break;
>> +        }
> 
> Each of the three tests above does a "break" upon surprising input,
> which effectively discards all remaining lines in the file.
> It would be more consistent to skip only the offending line,
> so that this function behaves the same when a single offending
> line is at the end of the file as when it's at the beginning.
> So, s/break/continue/ ?
> 
>> +        addr_val &= mask_val;
>> +
>> +        if ((net_dest == addr_val) &&
>> +            (innetmask.inet4.sin_addr.s_addr == mask_val)) {
>> +            networkReportError(VIR_ERR_INTERNAL_ERROR,
>> +                              _("Network %s/%s is already in use by "
>> +                                "interface %s"),
>> +                                network->def->ipAddress,
>> +                                network->def->netmask, iface);
>> +            goto error;
>> +        }
>> +
>> +        cur = strchr(cur, '\n');
> 
> Then you can change this:
> 
>            cur = nl;

Had to refactor things a bit, since a continue; wouldn't hit this last
statement, but I incorporated your recommended changes and resent.

Thanks,
Cole


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