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

Re: [libvirt] [PATCH] Don't expose 'vnet%d' to the user



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


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