On 11/24/2014 12:48 PM, Laine Stump wrote:
> These functions all set/get items in the sysfs for a bridge device.
Probably could have split up the "virNetDevBridgePort*" from the
virNetDevBridgeSetVlanFiltering - if only to make it a bit clearer in
the commit message that you're adding functions to manage? the
BridgePort settings.
> ---
> src/libvirt_private.syms | 6 ++
> src/util/virnetdevbridge.c | 236 ++++++++++++++++++++++++++++++++++++++++++++-
> src/util/virnetdevbridge.h | 28 +++++-
> 3 files changed, 268 insertions(+), 2 deletions(-)
>
Couple of nits follow - not necessary for a v2.
ACK w/ the changes
John
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 2647d36..6453826 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1662,9 +1662,15 @@ virNetDevBridgeCreate;
> virNetDevBridgeDelete;
> virNetDevBridgeGetSTP;
> virNetDevBridgeGetSTPDelay;
> +virNetDevBridgeGetVlanFiltering;
> +virNetDevBridgePortGetLearning;
> +virNetDevBridgePortGetUnicastFlood;
> +virNetDevBridgePortSetLearning;
> +virNetDevBridgePortSetUnicastFlood;
> virNetDevBridgeRemovePort;
> virNetDevBridgeSetSTP;
> virNetDevBridgeSetSTPDelay;
> +virNetDevBridgeSetVlanFiltering;
>
>
> # util/virnetdevmacvlan.h
> diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c
> index 15434de..9be835c 100644
> --- a/src/util/virnetdevbridge.c
> +++ b/src/util/virnetdevbridge.c
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (C) 2007-2013 Red Hat, Inc.
> + * Copyright (C) 2007-2014 Red Hat, Inc.
> *
> * This library is free software; you can redistribute it and/or
> * modify it under the terms of the GNU Lesser General Public
> @@ -217,8 +217,175 @@ static int virNetDevBridgeGet(const char *brname,
> VIR_FREE(path);
> return ret;
> }
> +
Spurrious? Or evidence of moving things around late in self review?
> #endif /* __linux__ */
>
> +#if defined(__linux__)
> +static
> +int
place on same line, eg "static int" (vs. 2 lines)
> +virNetDevBridgePortSet(const char *brname,
> + const char *ifname,
> + const char *paramname,
> + unsigned long value)
> +{
> + char *path = NULL;
> + char valuestr[INT_BUFSIZE_BOUND(value)];
> + int ret = -1;
> +
> + snprintf(valuestr, sizeof(valuestr), "%lu", value);
> +
> + if (virAsprintf(&path, "%s/%s/brif/%s/%s",
> + SYSFS_NET_DIR, brname, ifname, paramname) < 0)
> + return -1;
> +
> + if (!virFileExists(path))
> + errno = EINVAL;
> + else
> + ret = virFileWriteStr(path, valuestr, 0);
> +
> + if (ret < 0) {
> + virReportSystemError(errno,
> + _("Unable to set bridge %s port %s %s to %s"),
> + brname, ifname, paramname, valuestr);
> + }
> +
> + VIR_FREE(path);
> + return ret;
> +}
> +
> +
> +static
> +int
ditto
> +virNetDevBridgePortGet(const char *brname,
> + const char *ifname,
> + const char *paramname,
> + unsigned long *value)
> +{
> + char *path = NULL;
> + char *valuestr = NULL;
> + int ret = -1;
> +
> + if (virAsprintf(&path, "%s/%s/brif/%s/%s",
> + SYSFS_NET_DIR, brname, ifname, paramname) < 0)
> + return -1;
> +
> + if (virFileReadAll(path, INT_BUFSIZE_BOUND(unsigned long), &valuestr) < 0)
> + goto cleanup;
> +
> + if (virStrToLong_ul(valuestr, NULL, 10, value) < 0) {
> + virReportSystemError(EINVAL,
> + _("Unable to get bridge %s port %s %s"),
> + brname, ifname, paramname);
> + goto cleanup;
> + }
> +
> + ret = 0;
> + cleanup:
> + VIR_FREE(path);
> + VIR_FREE(valuestr);
> + return ret;
> +}
> +
> +
> +int
> +virNetDevBridgePortGetLearning(const char *brname,
> + const char *ifname,
> + bool *enable)
> +{
> + int ret = -1;
> + unsigned long value;
> +
> + if (virNetDevBridgePortGet(brname, ifname, "learning", &value) < 0)
> + goto cleanup;
> +
> + *enable = !!value;
Every time I see one of these "!!"...
I see virNetDevBridgeGetSTP() uses *enabled = val ? true : false; -
that's at least easier to read for me. Your call on which way to go though.
> + ret = 0;
> + cleanup:
> + return ret;
> +}
> +
> +
> +int
> +virNetDevBridgePortSetLearning(const char *brname,
> + const char *ifname,
> + bool enable)
> +{
> + return virNetDevBridgePortSet(brname, ifname, "learning", enable ? 1 : 0);
> +}
> +
> +
> +int
> +virNetDevBridgePortGetUnicastFlood(const char *brname,
> + const char *ifname,
> + bool *enable)
> +{
> + int ret = -1;
> + unsigned long value;
> +
> + if (virNetDevBridgePortGet(brname, ifname, "unicast_flood", &value) < 0)
> + goto cleanup;
> +
> + *enable = !!value;
> + ret = 0;
> + cleanup:
> + return ret;
> +}
> +
> +
> +int
> +virNetDevBridgePortSetUnicastFlood(const char *brname,
> + const char *ifname,
> + bool enable)
> +{
> + return virNetDevBridgePortSet(brname, ifname, "unicast_flood", enable ? 1 : 0);
> +}
> +
> +
> +#else
> +int
> +virNetDevBridgePortGetLearning(const char *brname ATTRIBUTE_UNUSED,
> + const char *ifname ATTRIBUTE_UNUSED,
> + bool *enable ATTRIBUTE_UNUSED)
> +{
> + virReportSystemError(ENOSYS, "%s",
> + _("Unable to get bridge port learning on this platform"));
> + return -1;
> +}
> +
> +
> +int
> +virNetDevBridgePortSetLearning(const char *brname ATTRIBUTE_UNUSED,
> + const char *ifname ATTRIBUTE_UNUSED,
> + bool enable)
> +{
> + virReportSystemError(ENOSYS, "%s",
> + _("Unable to set bridge port learning on this platform"));
> + return -1;
> +}
> +
> +
> +int
> +virNetDevBridgePortGetUnicastFlood(const char *brname ATTRIBUTE_UNUSED,
> + const char *ifname ATTRIBUTE_UNUSED,
> + bool *enable ATTRIBUTE_UNUSED)
> +{
> + virReportSystemError(ENOSYS, "%s",
> + _("Unable to get bridge port unicast_flood on this platform"));
> + return -1;
> +}
> +
> +
> +int
> +virNetDevBridgePortSetUnicastFlood(const char *brname ATTRIBUTE_UNUSED,
> + const char *ifname ATTRIBUTE_UNUSED,
> + bool enable ATTRIBUTE_UNUSED)
> +{
> + virReportSystemError(ENOSYS, "%s",
> + _("Unable to set bridge port unicast_flood on this platform"));
> + return -1;
> +}
> +#endif
> +
>
> /**
> * virNetDevBridgeCreate:
> @@ -682,3 +849,70 @@ int virNetDevBridgeGetSTP(const char *brname,
> return -1;
> }
> #endif
> +
> +#if defined(HAVE_STRUCT_IFREQ) && defined(__linux__)
> +/**
> + * virNetDevBridgeGetVlanFiltering:
> + * @brname: the bridge device name
> + * @enable: true or false
> + *
> + * Retrives the vlan_filtering setting for the bridge device @brname
s/Retrives/Retrieves/
> + * storing it in @enable.
> + *
> + * Returns 0 on success, -1 on error+
There's an extraneous "+" at the end ^
> + */
> +int
> +virNetDevBridgeGetVlanFiltering(const char *brname,
> + bool *enable)
> +{
> + int ret = -1;
> + unsigned long value;
> +
> + if (virNetDevBridgeGet(brname, "vlan_filtering", &value, -1, NULL) < 0)
> + goto cleanup;
> +
> + *enable = !!value;
> + ret = 0;
> + cleanup:
> + return ret;
> +}
> +
> +
> +/**
> + * virNetDevBridgeSetVlanFiltering:
> + * @brname: the bridge name
> + * @enable: true or false
> + *
> + * Set the bridge vlan_filtering mode
> + *
> + * Returns 0 in case of success or -1 on failure
> + */
> +
> +int
> +virNetDevBridgeSetVlanFiltering(const char *brname,
> + bool enable)
> +{
> + return virNetDevBridgeSet(brname, "vlan_filtering", enable ? 1 : 0, -1, NULL);
> +}
> +
> +
> +#else
> +int
> +virNetDevBridgeGetVlanFiltering(const char *brname ATTRIBUTE_UNUSED,
> + bool *enable ATTRIBUTE_UNUSED)
> +{
> + virReportSystemError(ENOSYS, "%s",
> + _("Unable to get bridge vlan_filtering on this platform"));
> + return -1;
> +}
> +
> +
> +int
> +virNetDevBridgeSetVlanFiltering(const char *brname ATTRIBUTE_UNUSED,
> + bool enable ATTRIBUTE_UNUSED)
> +{
> + virReportSystemError(ENOSYS, "%s",
> + _("Unable to set bridge vlan_filtering on this platform"));
> + return -1;
> +}
> +#endif
> diff --git a/src/util/virnetdevbridge.h b/src/util/virnetdevbridge.h
> index 13776d2..8188a2f 100644
> --- a/src/util/virnetdevbridge.h
> +++ b/src/util/virnetdevbridge.h
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (C) 2007-2011 Red Hat, Inc.
> + * Copyright (C) 2007-2012, 2014 Red Hat, Inc.
I forget if we're just allowed to say 2007-2014...
> *
> * This library is free software; you can redistribute it and/or
> * modify it under the terms of the GNU Lesser General Public
> @@ -51,4 +51,30 @@ int virNetDevBridgeGetSTP(const char *brname,
> bool *enable)
> ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
>
> +int virNetDevBridgeSetVlanFiltering(const char *brname,
> + bool enable)
> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
> +int virNetDevBridgeGetVlanFiltering(const char *brname,
> + bool *enable)
> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
> +
> +int virNetDevBridgePortGetLearning(const char *brname,
> + const char *ifname,
> + bool *enable)
> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
> + ATTRIBUTE_RETURN_CHECK;
> +int virNetDevBridgePortSetLearning(const char *brname,
> + const char *ifname,
> + bool enable)
> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
> +int virNetDevBridgePortGetUnicastFlood(const char *brname,
> + const char *ifname,
> + bool *enable)
> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
> + ATTRIBUTE_RETURN_CHECK;
> +int virNetDevBridgePortSetUnicastFlood(const char *brname,
> + const char *ifname,
> + bool enable)
> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
> +
> #endif /* __VIR_NETDEV_BRIDGE_H__ */
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list