On Tue, Aug 18, 2009 at 01:38:35PM +0100, Mark McLoughlin wrote:

https://bugzilla.redhat.com/517371

Matt Booth points out that if you use a non-existent bridge name when start a guest you get a weird error message:

Failed to add tap interface 'vnet%d' to bridge 'virbr0'

and dev='vnet%d' appears in the dumpxml output.

Fix that by not including 'vnet%d' in the error message and freeing the 'vnet%d' string if adding the tap device to the bridge fails.

* src/qemu_conf.c, src/uml_conf.c: fix qemudNetworkIfaceConnect() and umlConnectTapDevice() to not expose 'vnet%d' to the user --- src/qemu_conf.c | 25 +++++++++++++++++-------- src/uml_conf.c | 28 +++++++++++++++++++--------- 2 files changed, 36 insertions(+), 17 deletions(-)

diff --git a/src/qemu_conf.c b/src/qemu_conf.c index 082f107..bcdab11 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -1038,6 +1038,7 @@ qemudNetworkIfaceConnect(virConnectPtr conn, int err; int tapfd = -1; int vnet_hdr = 0; + int template_ifname = 0;

if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { virNetworkPtr network = virNetworkLookupByName(conn, @@ -1059,6 +1060,14 @@ qemudNetworkIfaceConnect(virConnectPtr conn, return -1; }

+ char ebuf[1024]; + if (!driver->brctl && (err = brInit(&driver->brctl))) { + qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("cannot initialize bridge support: %s"), + virStrerror(err, ebuf, sizeof ebuf)); + return -1; + } + if (!net->ifname || STRPREFIX(net->ifname, "vnet") || strchr(net->ifname, '%')) { @@ -1067,14 +1076,8 @@ qemudNetworkIfaceConnect(virConnectPtr conn, virReportOOMError(conn); return -1; } - } - - char ebuf[1024]; - if (!driver->brctl && (err = brInit(&driver->brctl))) { - qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, - _("cannot initialize bridge support: %s"), - virStrerror(err, ebuf, sizeof ebuf)); - return -1; + /* avoid exposing vnet%d in dumpxml or error outputs */ + template_ifname = 1; }

if (qemuCmdFlags & QEMUD_CMD_FLAG_VNET_HDR && @@ -1088,12 +1091,18 @@ qemudNetworkIfaceConnect(virConnectPtr conn, qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("Failed to add tap interface to bridge. " "%s is not a bridge device"), brname); + } else if (template_ifname) { + qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("Failed to add tap interface to bridge '%s' : %s"), + brname, virStrerror(err, ebuf, sizeof ebuf)); } else { qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("Failed to add tap interface '%s' " "to bridge '%s' : %s"), net->ifname, brname, virStrerror(err, ebuf, sizeof ebuf)); }

Both the original and new error reporting code here is using the wrong error code here. Any error that has an associated errno value should use virReportSystemError(conn, errno, fmt, args).

+ if (template_ifname) + VIR_FREE(net->ifname); return -1; }

diff --git a/src/uml_conf.c b/src/uml_conf.c index 4f756d4..a4c434f 100644 --- a/src/uml_conf.c +++ b/src/uml_conf.c @@ -104,17 +104,10 @@ umlConnectTapDevice(virConnectPtr conn, virDomainNetDefPtr net, const char *bridge) { + brControl *brctl = NULL; int tapfd = -1; + int template_ifname = 0; int err; - brControl *brctl = NULL; - - if (!net->ifname || - STRPREFIX(net->ifname, "vnet") || - strchr(net->ifname, '%')) { - VIR_FREE(net->ifname); - if (!(net->ifname = strdup("vnet%d"))) - goto no_memory; - }

if ((err = brInit(&brctl))) { char ebuf[1024]; @@ -124,6 +117,16 @@ umlConnectTapDevice(virConnectPtr conn, goto error; }

+ if (!net->ifname || + STRPREFIX(net->ifname, "vnet") || + strchr(net->ifname, '%')) { + VIR_FREE(net->ifname); + if (!(net->ifname = strdup("vnet%d"))) + goto no_memory; + /* avoid exposing vnet%d in dumpxml or error outputs */ + template_ifname = 1; + } + if ((err = brAddTap(brctl, bridge, &net->ifname, BR_TAP_PERSIST, &tapfd))) { if (errno == ENOTSUP) { @@ -131,6 +134,11 @@ umlConnectTapDevice(virConnectPtr conn, umlReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("Failed to add tap interface to bridge. " "%s is not a bridge device"), bridge); + } else if (template_ifname) { + char ebuf[1024]; + umlReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("Failed to add tap interface to bridge '%s' : %s"), + bridge, virStrerror(err, ebuf, sizeof ebuf)); } else { char ebuf[1024]; umlReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, @@ -138,6 +146,8 @@ umlConnectTapDevice(virConnectPtr conn, "to bridge '%s' : %s"), net->ifname, bridge, virStrerror(err, ebuf, sizeof ebuf)); } + if (template_ifname) + VIR_FREE(net->ifname);

Same errno issue here THe patch looks OK in general though Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|