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

[Stefan Berger]

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.

Sorry, but difficult to parse this sentence.

>
> Signed-off-by: John Ferlan <jferlan at redhat.com>
> ---
>   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);

Thinking out lout here (and below): virNWFilterDefFormat() will write 
the UUID into the XML. The STREQ will only ever be equal if def1 and 
def2 have the same UUID.

>
>    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))) {

We will not get here if a filter with the same uuid and name exists or 
same name and different uuid. So we can have the same uuid and a 
different name at this point.

>           objdef = obj->def;
> -        if (virNWFilterDefEqual(def, objdef, false)) {
> +        if (virNWFilterDefEqual(def, objdef)) {

This call doesn't need to 'shadow' the uuid. It's the only caller. so 
your change seems to be correct.

Reviewed-by: Stefan Berger <stefanb at linux.vnet.ibm.com>

>               virNWFilterDefFree(objdef);
>               obj->def = def;
>               return obj;