[libvirt] [PATCH v2] interface: report generic error message of lookup failure

[Laine Stump]

On 05/14/2013 08:16 PM, Guannan Ren wrote:
> "couldn't find interface named"
> "couldn't find interface with MAC address"
>
> use generic message as follows
> "couldn't find interface with"

If you were going to do this, having "with" at the end sounds awkward;
it would be better to just make it "couldn't find interface '%s'".

However, it doesn't make sense to make the messages in the driver itself
generic, because they are inside functions that are specific to mac
address / name, so it's appropriate for the error message to be specific.

I think I like Dan's suggestion (3) the best - in virsh, before doing
any lookups just try parsing the string into a mac address (with
virMacAddrParse()) - if virMacAddrParse() is successful, you then only
need to try lookupbymac (and can report "couldn't find interface with
MAC address '%s' if the lookup fails), and if virMacAddrParse fails, you
then only need to try lookupbyname.

Once you've done that, you shouldn't need any changes to the libvirtd
log messages at all.


> ---
>  src/interface/interface_backend_netcf.c | 16 ++++++++--------
>  src/interface/interface_backend_udev.c  |  8 ++++----
>  2 files changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/src/interface/interface_backend_netcf.c b/src/interface/interface_backend_netcf.c
> index cbba4fd..c6f3f42 100644
> --- a/src/interface/interface_backend_netcf.c
> +++ b/src/interface/interface_backend_netcf.c
> @@ -104,12 +104,12 @@ static struct netcf_if *interfaceDriverGetNetcfIF(struct netcf *ncf, virInterfac
>          int errcode = ncf_error(ncf, &errmsg, &details);
>          if (errcode != NETCF_NOERROR) {
>              virReportError(netcf_to_vir_err(errcode),
> -                           _("couldn't find interface named '%s': %s%s%s"),
> +                           _("couldn't find interface with '%s': %s%s%s"),
>                             ifinfo->name, errmsg, details ? " - " : "",
>                             details ? details : "");
>          } else {
>              virReportError(VIR_ERR_NO_INTERFACE,
> -                           _("couldn't find interface named '%s'"),
> +                           _("couldn't find interface with '%s'"),
>                             ifinfo->name);
>          }
>      }
> @@ -334,7 +334,7 @@ netcfConnectListAllInterfaces(virConnectPtr conn,
>              int errcode = ncf_error(driver->netcf, &errmsg, &details);
>              if (errcode != NETCF_NOERROR) {
>                  virReportError(netcf_to_vir_err(errcode),
> -                               _("couldn't find interface named '%s': %s%s%s"),
> +                               _("couldn't find interface with '%s': %s%s%s"),
>                                 names[i], errmsg,
>                                 details ? " - " : "", details ? details : "");
>                  goto cleanup;
> @@ -342,7 +342,7 @@ netcfConnectListAllInterfaces(virConnectPtr conn,
>                  /* Ignore the NETCF_NOERROR, as the interface is very likely
>                   * deleted by other management apps (e.g. virt-manager).
>                   */
> -                VIR_WARN("couldn't find interface named '%s', might be "
> +                VIR_WARN("couldn't find interface with '%s', might be "
>                           "deleted by other process", names[i]);
>                  continue;
>              }
> @@ -421,12 +421,12 @@ static virInterfacePtr netcfInterfaceLookupByName(virConnectPtr conn,
>          int errcode = ncf_error(driver->netcf, &errmsg, &details);
>          if (errcode != NETCF_NOERROR) {
>              virReportError(netcf_to_vir_err(errcode),
> -                           _("couldn't find interface named '%s': %s%s%s"),
> +                           _("couldn't find interface with '%s': %s%s%s"),
>                             name, errmsg,
>                             details ? " - " : "", details ? details : "");
>          } else {
>              virReportError(VIR_ERR_NO_INTERFACE,
> -                           _("couldn't find interface named '%s'"), name);
> +                           _("couldn't find interface with '%s'"), name);
>          }
>          goto cleanup;
>      }
> @@ -454,14 +454,14 @@ static virInterfacePtr netcfInterfaceLookupByMACString(virConnectPtr conn,
>          const char *errmsg, *details;
>          int errcode = ncf_error(driver->netcf, &errmsg, &details);
>          virReportError(netcf_to_vir_err(errcode),
> -                       _("couldn't find interface with MAC address '%s': %s%s%s"),
> +                       _("couldn't find interface with '%s': %s%s%s"),
>                         macstr, errmsg, details ? " - " : "",
>                         details ? details : "");
>          goto cleanup;
>      }
>      if (niface == 0) {
>          virReportError(VIR_ERR_NO_INTERFACE,
> -                       _("couldn't find interface with MAC address '%s'"),
> +                       _("couldn't find interface with '%s'"),
>                         macstr);
>          goto cleanup;
>      }
> diff --git a/src/interface/interface_backend_udev.c b/src/interface/interface_backend_udev.c
> index 1fd7d46..e61be52 100644
> --- a/src/interface/interface_backend_udev.c
> +++ b/src/interface/interface_backend_udev.c
> @@ -420,7 +420,7 @@ udevInterfaceLookupByName(virConnectPtr conn, const char *name)
>      dev = udev_device_new_from_subsystem_sysname(udev, "net", name);
>      if (!dev) {
>          virReportError(VIR_ERR_NO_INTERFACE,
> -                       _("couldn't find interface named '%s'"),
> +                       _("couldn't find interface with '%s'"),
>                         name);
>          goto cleanup;
>      }
> @@ -467,7 +467,7 @@ udevInterfaceLookupByMACString(virConnectPtr conn, const char *macstr)
>      /* Check that we got something back */
>      if (!dev_entry) {
>          virReportError(VIR_ERR_NO_INTERFACE,
> -                       _("couldn't find interface with MAC address '%s'"),
> +                       _("couldn't find interface with '%s'"),
>                         macstr);
>          goto cleanup;
>      }
> @@ -940,7 +940,7 @@ udevGetIfaceDef(struct udev *udev, const char *name)
>      dev = udev_device_new_from_subsystem_sysname(udev, "net", name);
>      if (!dev) {
>          virReportError(VIR_ERR_NO_INTERFACE,
> -                       _("couldn't find interface named '%s'"), name);
> +                       _("couldn't find interface with '%s'"), name);
>          goto error;
>      }
>  
> @@ -1068,7 +1068,7 @@ udevInterfaceIsActive(virInterfacePtr ifinfo)
>                                                   ifinfo->name);
>      if (!dev) {
>          virReportError(VIR_ERR_NO_INTERFACE,
> -                       _("couldn't find interface named '%s'"),
> +                       _("couldn't find interface with '%s'"),
>                         ifinfo->name);
>          status = -1;
>          goto cleanup;