[libvirt] [PATCH v4] network: bridge: Don't start network if it collides with host routing
Cole Robinson
crobinso at redhat.com
Thu May 27 20:54:15 UTC 2010
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
More information about the libvir-list
mailing list