Re: [PATCH V2 2/5] Add support for STP filtering

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

 



On 11/22/2011 08:51 AM, Stefan Berger wrote:
> This patch adds support for filtering of STP (spanning tree protocol) traffic
> to the parser and makes us of the ebtables support for STP filtering. This code
> now enables the filtering of traffic in chains with prefix 'stp'.
> 
> Signed-off-by: Stefan Berger <stefanb@xxxxxxxxxxxxxxxxxx>
> 
> ---
> 
> v2:
>  - addressing Eric Blake's comments
>  - replacing occurrences of number[20] with number[INT_BUFFER_BOUND(uint32)]; this
>    also works for IP masks which are expressed as "%d".

The maximum %d is one byte longer than %u, thanks to a negative sign; so
you've got a potential off-by-one issue (although a negative IP mask
doesn't make sense, so you may be safe for now).

>  
> +static const virXMLAttr2Struct stpAttributes[] = {
> +    /* spanning tree uses a special destination MAC address */
> +    {

I'd feel a bit better if this comment appears near here:

/* STP is documented by IEEE 802.1D; for a synopsis,
 * see http://www.javvin.com/protocolSTP.html */

> @@ -937,7 +950,7 @@ iptablesHandleIpHdr(virBufferPtr buf,
>                      virBufferPtr prefix)
>  {
>      char ipaddr[INET6_ADDRSTRLEN],
> -         number[20];
> +         number[INT_BUFSIZE_BOUND(uint32_t)];

Maybe it's best to rely on util.h giving us MAX, and doing:

char number[MAX(INT_BUFSIZE_BOUND(uint32_t),
                INT_BUFSIZE_BOUND(int))];

so that we can use both %u for uint32_t, and %d for int, without
worrying about any other weird type promotions going on.

ACK with those nits addressed.

-- 
Eric Blake   eblake@xxxxxxxxxx    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]