[libvirt] [PATCH v4 1/4] nwfilter: Remove unnecessary UUID comparison bypass

[Laine Stump]

On 12/08/2017 09:01 AM, John Ferlan wrote:
> Commit id '46a811db07' added code to check if the filter by Name
> already existed with a different UUID, to go along with the existing
> found filter by UUID and compare the Names match thus making it
> impossible to reach this find by Name condition without both the
> Name and UUID already matching, so remove the need to "filter"
> out the UUID for the virNWFilterDefEqual.

I can't be certain, but it looks to me like the original nwfilter author
intended to allow changing the uuid of an existing nwfilter, and put in
all the code checking for "matches everything except uuid" specifically
to make that work correctly, but a bug report
(https://bugzilla.redhat.com/1077009) convinced someone else to prohibit
that, thus rendering all the stuff you've removed (and I think a bit
more) irrelevant. What it's been reduced to now is "if the new
definition is *exactly* identical to the old definition, then replace
the old with the new but don't rebuild client filters", which seems
mostly pointless, but also harmless (it's just replacing the original
with a new copy that is exactly identical).


>
> Signed-off-by: John Ferlan <jferlan at redhat.com>

Reviewed-by: Laine Stump <laine at laine.org>


> ---
>  src/conf/virnwfilterobj.c | 18 ++++--------------
>  1 file changed, 4 insertions(+), 14 deletions(-)
>
> diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c
> index 408e575ca..87d7e7270 100644
> --- a/src/conf/virnwfilterobj.c
> +++ b/src/conf/virnwfilterobj.c
> @@ -287,18 +287,11 @@ virNWFilterObjTestUnassignDef(virNWFilterObjPtr obj)
>  
>  static bool
>  virNWFilterDefEqual(const virNWFilterDef *def1,
> -                    virNWFilterDefPtr def2,
> -                    bool cmpUUIDs)
> +                    virNWFilterDefPtr def2)
>  {
>      bool ret = false;
> -    unsigned char rem_uuid[VIR_UUID_BUFLEN];
> -    char *xml1, *xml2 = NULL;
> -
> -    if (!cmpUUIDs) {
> -        /* make sure the UUIDs are equal */
> -        memcpy(rem_uuid, def2->uuid, sizeof(rem_uuid));
> -        memcpy(def2->uuid, def1->uuid, sizeof(def2->uuid));
> -    }
> +    char *xml1 = NULL;
> +    char *xml2 = NULL;
>  
>      if (!(xml1 = virNWFilterDefFormat(def1)) ||
>          !(xml2 = virNWFilterDefFormat(def2)))
> @@ -307,9 +300,6 @@ virNWFilterDefEqual(const virNWFilterDef *def1,
>      ret = STREQ(xml1, xml2);
>  
>   cleanup:
> -    if (!cmpUUIDs)
> -        memcpy(def2->uuid, rem_uuid, sizeof(rem_uuid));
> -
>      VIR_FREE(xml1);
>      VIR_FREE(xml2);
>  
> @@ -360,7 +350,7 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
>      if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) {
>  
>          objdef = obj->def;
> -        if (virNWFilterDefEqual(def, objdef, false)) {
> +        if (virNWFilterDefEqual(def, objdef)) {
>              virNWFilterDefFree(objdef);
>              obj->def = def;
>              return obj;