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.
Signed-off-by: John Ferlan <jferlan@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);
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;
--
2.13.6
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@redhat.com>
Reviewed-by: Laine Stump <laine@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;
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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@linux.vnet.ibm.com>
> virNWFilterDefFree(objdef);
> obj->def = def;
> return obj;
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 01/31/2018 05:52 PM, Stefan Berger wrote:
> 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.
>
I'll reword before pushing... Suffice to say it was a confusing check
when I came across it while working through the changes... This
particular commit was written in July 2017! New wording:
Remove the unnecessary check as since commit id '46a811db07' it is
not possible to add or alter a filter using the same name, but with
a different UUID.
NB: It's not required to provide a UUID for a filter by name, but
if one is provided, then it must match the existing. If not provided,
then one is generated during ParseXML processing.
>>
>> Signed-off-by: John Ferlan <jferlan@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.
>
So, IIRC, what I determined at the time was that virNWFilterDefEqual was
only ever called with @cmpUUIDs == false. Looking through history, there
never was a case where @cmpUUIDs == true that I could find - all the way
back to your original implementation in commit '823b90339'.
>>
>> 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.
>
Not sure I agree with your last sentence.
The first search in virNWFilterObjAssignDef is FindByUUID. If a filter
in the nwfilters list matches the incoming @def->uuid, then a check is
made that the filter name's match. If the names don't match an error is
returned.
Otherwise, if a filter by the incoming UUID is not found, then a
FindByName is done. If one is found, then a check is made that the
UUID's match. If the UUID's don't match, then an error is returned
(likely!)... If the UUID's matched, then something's wrong because we
failed the FindByUUID.
If we get here, then we are assured that the uuid's and name's match, so
what we're checking is whether the incoming definition changes anything
else if a definition is found in the nwfilters list.
>> 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@linux.vnet.ibm.com>
>
Thanks - I'll push this first patch sometime soon.
John
>> virNWFilterDefFree(objdef);
>> obj->def = def;
>> return obj;
>
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.