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@xxxxxx
El 05/06/2013, a las 17:42, John Ferlan <jferlan@xxxxxxxxxx> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list