[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



You're right! I missed that line, but it should use guest_interface indeed.

How do we proceed? Could you fix it or do you need me to send a new patch?

Best,

Alvaro Polo Valdenebro
Product Development & Innovation / Telefónica Digital
C/ Don Ramón de la Cruz 82-84
Madrid 28006
(+34) 609 087 054
apv tid es

El 05/06/2013, a las 17:42, John Ferlan <jferlan redhat com> escribió:

> 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
>


________________________________

Este mensaje se dirige exclusivamente a su destinatario. Puede consultar nuestra política de envío y recepción de correo electrónico en el enlace situado más abajo.
This message is intended exclusively for its addressee. We only send and receive email on the basis of the terms set out at:
http://www.tid.es/ES/PAGINAS/disclaimer.aspx


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