[libvirt] [PATCH 1/4] interface: Refactor udev bridge to helper func
Laine Stump
laine at laine.org
Mon Feb 18 20:31:30 UTC 2013
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);
>
More information about the libvir-list
mailing list