Re: [PATCH V4] nwfilter: Add support for ipset

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

 



On 05/14/2012 07:00 PM, Stefan Berger wrote:
> This patch adds support for the recent ipset iptables extension
> to libvirt's nwfilter subsystem. Ipset allows to maintain 'sets'
> of IP addresses, ports and other packet parameters and allows for
> faster lookup (in the order of O(1) vs. O(n)) and rule evaluation
> to achieve higher throughput than what can be achieved with
> individual iptables rules.
> 

> 
> FYI: Here is the man page for ipset:
> 
> https://ipset.netfilter.org/ipset.man.html

s/https/http/

> 
> +static bool
> +ipsetValidator(enum attrDatatype datatype ATTRIBUTE_UNUSED, union data
> *val,

Not sure why this line wrapped in my reply, but I don't think it is a
problem in the real patch.


> +static bool
> +ipsetFlagsFormatter(virBufferPtr buf,
> +                    virNWFilterRuleDefPtr nwf ATTRIBUTE_UNUSED,
> +                    nwItemDesc *item)
> +{
> +    uint8_t ctr;
> +
> +    for (ctr = 0; ctr < item->u.ipset.numFlags; ctr++) {
> +        if (ctr != 0)
> +            virBufferAddLit(buf, ",");

I would have used this, but I don't think it makes any difference in speed:

virBufferAddChar(buf, ',')


> +    case DATATYPE_IPSETFLAGS:

> +
> +        flags = virBufferContentAndReset(&vb);
> +
> +        if (snprintf(buf, bufsize, "%s", flags) >= bufsize) {
> +            virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                                   _("Buffer too small for IPSETFLAGS
> type"));

Missed an instance of virStrncpy being nicer than snprintf.

Other than that, you hit all my review points, so you have my:

ACK.

If by Tuesday, no one speaks up with a counter-argument against this
patch as-is, then I say go ahead and apply with the nits fixed.

-- 
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]