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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list