[libvirt] [PATCH v2 3/3] nwfilter: Rebuild filters only if new filter is different than current

[Eric Blake]

On 01/18/2012 09:20 AM, Stefan Berger wrote:
> Compare two filter definitions for equality and only rebuild/instantiate
> the new filter if the two filters are found to be different. This improves
> performance during an update of a filter with no obvious change or the reloading
> of filters during a 'kill -SIGHUP'
> 
> Unforuntately this more involved than a mere memcmp() on the structures.

s/Unforuntately/Unfortunately/

> 
> ---
>  src/Makefile.am               |    1 
>  src/conf/nwfilter_conf.c      |  213 ++++++++++++++++++++
>  src/conf/nwfilter_params.c    |   18 +
>  src/conf/nwfilter_params.h    |    2 
>  src/conf/nwfilter_protocols.c |  447 ++++++++++++++++++++++++++++++++++++++++++
>  src/conf/nwfilter_protocols.h |   56 +++++
>  6 files changed, 737 insertions(+)

Fails 'make syntax-check':

prohibit_empty_lines_at_EOF
src/conf/nwfilter_protocols.c
maint.mk: empty line(s) or no newline at EOF

And while fixing that, I noticed that you didn't bump any copyright
years.  Emacs users can get this for free with this in ~/.emacs (not
sure if vim has a comparable hook):

(require 'copyright)
(defun my-copyright-update (&optional arg)
  "My improvements to `copyright-update'."
  (interactive "*P")
  (and (not (eq major-mode 'fundamental-mode))
       (copyright-update arg))
  nil)
(add-hook 'before-save-hook 'my-copyright-update)

> Index: libvirt-iterator/src/conf/nwfilter_params.c
> ===================================================================
> --- libvirt-iterator.orig/src/conf/nwfilter_params.c
> +++ libvirt-iterator/src/conf/nwfilter_params.c
> @@ -747,6 +747,19 @@ err_exit:
>      return -1;
>  }
>  
> +bool
> +virNWFilterHashTableEqual(const virNWFilterHashTablePtr hash1,
> +                          const virNWFilterHashTablePtr hash2)
> +{
> +    if (hash1 == hash2)
> +        return true;
> +
> +    if (!hash1 || !hash2)
> +        return false;
> +
> +    return virHashEqual(hash1->hashTable, hash2->hashTable,
> +                           (virHashValueComparator)strcmp);

Indentation - I'd remove 3 spaces, so that the case lines up with hash1
on the line before.

> Index: libvirt-iterator/src/conf/nwfilter_protocols.c
> ===================================================================
> --- /dev/null
> +++ libvirt-iterator/src/conf/nwfilter_protocols.c
> @@ -0,0 +1,447 @@
> +/*
> + * nwfilter_protocols.c: protocol handling
> + *
> + * Copyright (C) 2011 IBM Corporation

It's 2012, now :)

> +
> +#define VIR_FROM_THIS VIR_FROM_NWFILTER
> +
> +#define NWITEMDESCEQUAL(ITEM) \
> +    virNWFilterNWItemDescEqual(&def1->ITEM, &def2->ITEM)

You don't have to change if you don't want, but it took me a while to
parse that name.  Maybe NW_ITEM_DESC_EQUAL would be more readable.

> Index: libvirt-iterator/src/conf/nwfilter_protocols.h
> ===================================================================
> --- /dev/null
> +++ libvirt-iterator/src/conf/nwfilter_protocols.h
> @@ -0,0 +1,56 @@
> +/*
> + * nwfilter_protocols.h: protocol handling
> + *
> + * Copyright (C) 2011 IBM Corporation

2012

Big, but looks like a rather mechanical conversion of the various
protocols into a highly repetitive algorithm for comparisons.  I didn't
notice any major problems, so:

ACK with nits fixed.

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120123/4fccfb6d/attachment-0001.sig>