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

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
 


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