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

Re: [libvirt] [PATCH 1/4] interface: Refactor udev bridge to helper func



On 02/17/2013 08:56 PM, Doug Goldstein wrote:
> Mechanical move to break up udevIfaceGetIfaceDef() into different
> helpers for each of the interface types to hopefully make the code
> easier to follow. This moves the bridge code to
> udevIfaceGetIfaceDefBridge().

Before I noticed that this was all just code movement, I looked at the
incoming code and found some issues. Since this patch is code movement,
ACK to it anyway, but I think the issues I point out should be taken
care of in a separate patch.

(Oh, but you should remove the extra declaration of
udevIfaceGetIfaceDefBridge , and combine its ATTRIBUTEs with the
definition.)

> ---
>  src/interface/interface_backend_udev.c | 161 +++++++++++++++++++--------------
>  1 file changed, 92 insertions(+), 69 deletions(-)
>
> diff --git a/src/interface/interface_backend_udev.c b/src/interface/interface_backend_udev.c
> index 92c35d9..abd9895 100644
> --- a/src/interface/interface_backend_udev.c
> +++ b/src/interface/interface_backend_udev.c
> @@ -41,6 +41,8 @@ typedef enum {
>      VIR_UDEV_IFACE_ALL
>  } virUdevStatus ;
>  
> +static virInterfaceDef *udevIfaceGetIfaceDef(struct udev *udev, const char *name);
> +
>  static const char *
>  virUdevStatusString(virUdevStatus status)
>  {
> @@ -492,14 +494,14 @@ err:
>  }
>  
>  /**
> - * Helper function for our use of scandir()
> + * Helper function for finding bridge members using scandir()
>   *
>   * @param entry - directory entry passed by scandir()
>   *
>   * @return 1 if we want to add it to scandir's list, 0 if not.
>   */
>  static int
> -udevIfaceScanDirFilter(const struct dirent *entry)
> +udevIfaceBridgeScanDirFilter(const struct dirent *entry)
>  {
>      if (STREQ(entry->d_name, ".") || STREQ(entry->d_name, ".."))
>          return 0;
> @@ -538,18 +540,101 @@ udevIfaceFreeIfaceDef(virInterfaceDef *ifacedef)
>      VIR_FREE(ifacedef);
>  }
>  
> +static int
> +udevIfaceGetIfaceDefBridge(struct udev *udev,
> +                           struct udev_device *dev,
> +                           const char *name,
> +                           virInterfaceDef *ifacedef)
> +ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
> +    ATTRIBUTE_NONNULL(4) ATTRIBUTE_RETURN_CHECK;
> +
> +static int
> +udevIfaceGetIfaceDefBridge(struct udev *udev,
> +                           struct udev_device *dev,
> +                           const char *name,
> +                           virInterfaceDef *ifacedef)

Instead of a separate declaration, why not just embed the ATTRIBUTE
macros in the definition:

static int
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2_ ATTRIBUTE_NONNULL(3)
ATTRIBUTE_NONNULL(4)
ATTRIBUTE_RETURN_CHECK
udevIfaceGetIfaceDefBridge(struct udev *udev, .....)

> +{
> +    struct dirent **member_list = NULL;
> +    int member_count = 0;
> +    char *member_path;
> +    const char *stp_str;
> +    int stp;
> +    int i;
> +
> +    /* Set our type to Bridge  */
> +    ifacedef->type = VIR_INTERFACE_TYPE_BRIDGE;
> +
> +    /* Bridge specifics */
> +    ifacedef->data.bridge.delay =
> +        strdup(udev_device_get_sysattr_value(dev, "bridge/forward_delay"));

Is it possible that the above udev function could ever legitimately
return NULL? If so, you could potentially be treating a "no
forward_delay value given" return as an OOM error.

> +    if (!ifacedef->data.bridge.delay) {
> +        virReportOOMError();
> +        goto cleanup;
> +    }
> +
> +    stp_str = udev_device_get_sysattr_value(dev, "bridge/stp_state");

Likewise, could that ever return NULL in anything other than an error
condition?

> +    if (virStrToLong_i(stp_str, NULL, 10, &stp) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                _("Could not parse STP state '%s'"), stp_str);
> +        goto cleanup;
> +    }
> +    ifacedef->data.bridge.stp = stp;

I doubt it would ever make any difference in practice, but stp should
always be set to 0, 1, or -1. If it was set to some other non-0 value,
it wouldn't be included in the output of the format function.


> +
> +    /* Members of the bridge */
> +    if (virAsprintf(&member_path, "%s/%s",
> +                udev_device_get_syspath(dev), "brif") < 0) {
> +        virReportOOMError();
> +        goto cleanup;
> +    }
> +
> +    /* Get each member of the bridge */
> +    member_count = scandir(member_path, &member_list,
> +            udevIfaceBridgeScanDirFilter, alphasort);

This is going to end up including all of the guest tap devices, isn't
it? That could make it difficult for virt-manager to determine which
physdev to list in its dropdown menu. Perhaps there's some non-yucky way
to eliminate libvirt's tap devices? (the danger here would be that you
could possibly eliminate some *other* tap device that *was* supposed to
be listed, for example the tap used for a VPN or something).

> +
> +    /* Don't need the path anymore */
> +    VIR_FREE(member_path);
> +
> +    if (member_count < 0) {
> +        virReportSystemError(errno,
> +                _("Could not get members of bridge '%s'"),
> +                name);
> +        goto cleanup;
> +    }
> +
> +    /* Allocate our list of member devices */
> +    if (VIR_ALLOC_N(ifacedef->data.bridge.itf, member_count) < 0) {
> +        virReportOOMError();
> +        goto cleanup;
> +    }
> +    ifacedef->data.bridge.nbItf = member_count;
> +
> +    for (i = 0; i < member_count; i++) {
> +        ifacedef->data.bridge.itf[i] =
> +            udevIfaceGetIfaceDef(udev, member_list[i]->d_name);

You should check for NULL return here log an error if it happens.

> +        VIR_FREE(member_list[i]);
> +    }
> +
> +    VIR_FREE(member_list);
> +
> +    return 0;
> +
> +cleanup:
> +    for (i = 0; i < member_count; i++) {
> +        VIR_FREE(member_list[i]);
> +    }
> +    VIR_FREE(member_list);
> +
> +    return -1;
> +}
>  
>  static virInterfaceDef * ATTRIBUTE_NONNULL(1)
> -udevIfaceGetIfaceDef(struct udev *udev, char *name)
> +udevIfaceGetIfaceDef(struct udev *udev, const char *name)
>  {
>      struct udev_device *dev = NULL;
>      virInterfaceDef *ifacedef;
>      unsigned int mtu;
>      const char *mtu_str;
>      char *vlan_parent_dev = NULL;
> -    struct dirent **member_list = NULL;
> -    int member_count = 0;
> -    int i;
>  
>      /* Allocate our interface definition structure */
>      if (VIR_ALLOC(ifacedef) < 0) {
> @@ -629,66 +714,8 @@ udevIfaceGetIfaceDef(struct udev *udev, char *name)
>          ifacedef->data.vlan.tag = vid;
>          ifacedef->data.vlan.devname = vlan_parent_dev;
>      } else if (STREQ_NULLABLE(udev_device_get_devtype(dev), "bridge")) {
> -        /* Check if its a bridge device */
> -        char *member_path;
> -        const char *stp_str;
> -        int stp;
> -
> -        /* Set our type to Bridge  */
> -        ifacedef->type = VIR_INTERFACE_TYPE_BRIDGE;
> -
> -        /* Bridge specifics */
> -        ifacedef->data.bridge.delay =
> -            strdup(udev_device_get_sysattr_value(dev, "bridge/forward_delay"));
> -        if (!ifacedef->data.bridge.delay) {
> -            virReportOOMError();
> -            goto cleanup;
> -        }
> -
> -        stp_str = udev_device_get_sysattr_value(dev, "bridge/stp_state");
> -        if (virStrToLong_i(stp_str, NULL, 10, &stp) < 0) {
> -            virReportError(VIR_ERR_INTERNAL_ERROR,
> -                           _("Could not parse STP state '%s'"), stp_str);
> -            goto cleanup;
> -        }
> -        ifacedef->data.bridge.stp = stp;
> -
> -        /* Members of the bridge */
> -        if (virAsprintf(&member_path, "%s/%s",
> -                        udev_device_get_syspath(dev), "brif") < 0) {
> -            virReportOOMError();
> -            goto cleanup;
> -        }
> -
> -        /* Get each member of the bridge */
> -        member_count = scandir(member_path, &member_list,
> -                               udevIfaceScanDirFilter, alphasort);
> -
> -        /* Don't need the path anymore */
> -        VIR_FREE(member_path);
> -
> -        if (member_count < 0) {
> -            virReportSystemError(errno,
> -                                 _("Could not get members of bridge '%s'"),
> -                                 name);
> -            goto cleanup;
> -        }
> -
> -        /* Allocate our list of member devices */
> -        if (VIR_ALLOC_N(ifacedef->data.bridge.itf, member_count) < 0) {
> -            virReportOOMError();
> +        if (udevIfaceGetIfaceDefBridge(udev, dev, name, ifacedef))
>              goto cleanup;
> -        }
> -        ifacedef->data.bridge.nbItf = member_count;
> -
> -        for (i = 0; i < member_count; i++) {
> -            ifacedef->data.bridge.itf[i] =
> -                udevIfaceGetIfaceDef(udev, member_list[i]->d_name);
> -            VIR_FREE(member_list[i]);
> -        }
> -
> -        VIR_FREE(member_list);
> -
>      } else {
>          /* Set our type to ethernet */
>          ifacedef->type = VIR_INTERFACE_TYPE_ETHERNET;
> @@ -700,10 +727,6 @@ udevIfaceGetIfaceDef(struct udev *udev, char *name)
>  
>  cleanup:
>      udev_device_unref(dev);
> -    for (i = 0; i < member_count; i++) {
> -        VIR_FREE(member_list[i]);
> -    }
> -    VIR_FREE(member_list);
>  
>      udevIfaceFreeIfaceDef(ifacedef);
>  


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