Re: [libvirt] [PATCH] Fix a invalid usage of virDomainNetDef in OpenVZ driver

On 06/04/2013 03:44 AM, Alvaro Polo wrote: > OpenVZ was accessing ethernet data to obtain the guest iface name > regardless the domain is configured to use ethernet or bridged > networking. This prevented the guest network interface to be rightly > named for bridged networking. > --- > src/openvz/openvz_driver.c | 20 +++++++++++++------- > 1 file changed, 13 insertions(+), 7 deletions(-) > > diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c > index c8081ce..db738a4 100644 > --- a/src/openvz/openvz_driver.c > +++ b/src/openvz/openvz_driver.c > @@ -815,6 +815,7 @@ openvzDomainSetNetwork(virConnectPtr conn, const char *vpsid, > char host_macaddr[VIR_MAC_STRING_BUFLEN]; > struct openvz_driver *driver = conn->privateData; > virCommandPtr cmd = NULL; > + char *guest_ifname = NULL; > > if (net == NULL) > return 0; > @@ -840,11 +841,15 @@ openvzDomainSetNetwork(virConnectPtr conn, const char *vpsid, > virBuffer buf = VIR_BUFFER_INITIALIZER; > int veid = openvzGetVEID(vpsid); > > - /* if user doesn't specify guest interface name, > - * then we need to generate it */ > - if (net->data.ethernet.dev == NULL) { > - net->data.ethernet.dev = openvzGenerateContainerVethName(veid); > - if (net->data.ethernet.dev == NULL) { > + /* if net is ethernet and the user has specified guest interface name, > + * let's use it; otherwise generate a new one */ > + if (net->type == VIR_DOMAIN_NET_TYPE_ETHERNET && > + net->data.ethernet.dev != NULL) { [1] I know this code was pushed, but see note below > + if (VIR_STRDUP(guest_ifname, net->data.ethernet.dev) == -1) > + goto cleanup; > + } else { > + guest_ifname = openvzGenerateContainerVethName(veid); > + if (guest_ifname == NULL) { > virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > _("Could not generate eth name for container")); > goto cleanup; > @@ -862,7 +867,7 @@ openvzDomainSetNetwork(virConnectPtr conn, const char *vpsid, > } > } > > - virBufferAdd(&buf, net->data.ethernet.dev, -1); /* Guest dev */ > + virBufferAdd(&buf, guest_ifname, -1); /* Guest dev */ > virBufferAsprintf(&buf, ",%s", macaddr); /* Guest dev mac */ > virBufferAsprintf(&buf, ",%s", net->ifname); /* Host dev */ > virBufferAsprintf(&buf, ",%s", host_macaddr); /* Host dev mac */ > @@ -871,7 +876,7 @@ openvzDomainSetNetwork(virConnectPtr conn, const char *vpsid, > if (driver->version >= VZCTL_BRIDGE_MIN_VERSION) { > virBufferAsprintf(&buf, ",%s", net->data.bridge.brname); /* Host bridge */ > } else { > - virBufferAsprintf(configBuf, "ifname=%s", net->data.ethernet.dev); > + virBufferAsprintf(configBuf, "ifname=%s", guest_ifname); > virBufferAsprintf(configBuf, ",mac=%s", macaddr); /* Guest dev mac */ > virBufferAsprintf(configBuf, ",host_ifname=%s", net->ifname); /* Host dev */ > virBufferAsprintf(configBuf, ",host_mac=%s", host_macaddr); /* Host dev mac */ > @@ -895,6 +900,7 @@ openvzDomainSetNetwork(virConnectPtr conn, const char *vpsid, > > cleanup: > virCommandFree(cmd); > + VIR_FREE(guest_ifname); > return rc; > } > > [1] My nightly Coverity run had the following complaint as a result of this patch: 818 char *guest_ifname = NULL; 819 (1) Event cond_false: Condition "net == NULL", taking false branch 820 if (net == NULL) 821 return 0; (2) Event cond_false: Condition "vpsid == NULL", taking false branch 822 if (vpsid == NULL) { 823 virReportError(VIR_ERR_INTERNAL_ERROR, "%s", 824 _("Container ID is not specified")); 825 return -1; (3) Event if_end: End of if statement 826 } 827 (4) Event cond_true: Condition "net->type != VIR_DOMAIN_NET_TYPE_BRIDGE", taking true branch (5) Event cond_false: Condition "net->type != VIR_DOMAIN_NET_TYPE_ETHERNET", taking false branch 828 if (net->type != VIR_DOMAIN_NET_TYPE_BRIDGE && 829 net->type != VIR_DOMAIN_NET_TYPE_ETHERNET) 830 return 0; 831 832 cmd = virCommandNewArgList(VZCTL, "--quiet", "set", vpsid, NULL); 833 834 virMacAddrFormat(&net->mac, macaddr); 835 virDomainNetGenerateMAC(driver->xmlopt, &host_mac); 836 virMacAddrFormat(&host_mac, host_macaddr); 837 (6) Event cond_false: Condition "net->type == VIR_DOMAIN_NET_TYPE_BRIDGE", taking false branch (7) Event cond_true: Condition "net->type == VIR_DOMAIN_NET_TYPE_ETHERNET", taking true branch (8) Event cond_true: Condition "net->data.ethernet.ipaddr == NULL", taking true branch 838 if (net->type == VIR_DOMAIN_NET_TYPE_BRIDGE || 839 (net->type == VIR_DOMAIN_NET_TYPE_ETHERNET && 840 net->data.ethernet.ipaddr == NULL)) { 841 virBuffer buf = VIR_BUFFER_INITIALIZER; 842 int veid = openvzGetVEID(vpsid); 843 844 /* if net is ethernet and the user has specified guest interface name, 845 * let's use it; otherwise generate a new one */ (9) Event cond_true: Condition "net->type == VIR_DOMAIN_NET_TYPE_ETHERNET", taking true branch (10) Event cond_false: Condition "net->data.ethernet.dev != NULL", taking false branch (12) Event var_compare_op: Comparing "net->data.ethernet.dev" to null implies that "net->data.ethernet.dev" might be null. Also see events: [var_deref_model] 846 if (net->type == VIR_DOMAIN_NET_TYPE_ETHERNET && 847 net->data.ethernet.dev != NULL) { 848 if (VIR_STRDUP(guest_ifname, net->data.ethernet.dev) == -1) 849 goto cleanup; (11) Event else_branch: Reached else branch 850 } else { 851 guest_ifname = openvzGenerateContainerVethName(veid); (13) Event cond_false: Condition "guest_ifname == NULL", taking false branch 852 if (guest_ifname == NULL) { 853 virReportError(VIR_ERR_INTERNAL_ERROR, "%s", 854 _("Could not generate eth name for container")); 855 goto cleanup; (14) Event if_end: End of if statement 856 } 857 } 858 859 /* if user doesn't specified host interface name, 860 * than we need to generate it */ (15) Event cond_true: Condition "net->ifname == NULL", taking true branch 861 if (net->ifname == NULL) { (16) Event var_deref_model: Passing null pointer "net->data.ethernet.dev" to function "openvzGenerateVethName(int, char *)", which dereferences it. [details] Also see events: [var_compare_op] 862 net->ifname = openvzGenerateVethName(veid, net->data.ethernet.dev); 863 if (net->ifname == NULL) { 864 virReportError(VIR_ERR_INTERNAL_ERROR, "%s", 865 _("Could not generate veth name")); 866 goto cleanup; 867 } 868 } 869 If I'm reading the code correctly, it seems replacing 'net->data.ethernet.dev' with 'guest_ifname' at line 862 will be the right solution. Doing so makes Coverity happy. Previously the code guarenteed data.ethernet.dev was filled in with the openvzGenerateContainerVethName() result. John