[libvirt] [PATCH] Fix a invalid usage of virDomainNetDef in OpenVZ driver
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) { + 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.7.12.4 (Apple Git-37)
On Tue, Jun 04, 2013 at 09:44:32AM +0200, 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.
Ooh, nasty one!
--- 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) { + 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;
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 06/04/2013 01: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(-)
ACK and pushed. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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
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
On 06/05/2013 11:53 AM, ALVARO POLO VALDENEBRO wrote:
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?
If you post the patch difference I can ACK and push for you. I still haven't really worked out a "process" to deal with daily coverity issues - I usually just figure out which change resulted in the issue, reply to that thread, and allow the author to fix. Sometimes the fix isn't as simple as this one and requires a bit more changes by the author... Most are fairly easy, but I've learned even the easy ones can turn into an adventure. John
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ó:
<snip>
participants (5)
-
Alvaro Polo -
ALVARO POLO VALDENEBRO -
Daniel P. Berrange -
Eric Blake -
John Ferlan