Rather than dereferencing obj->def->XXX or nwfilters->objs[i]->X
create local virNWFilterObjPtr and virNWFilterDefPtr variables.
Future adjustments will be privatizing the object more, so this just
prepares the code for that reality.
Signed-off-by: John Ferlan <jferlan@redhat.com>
---
src/conf/virnwfilterobj.c | 80 +++++++++++++++++++++++++++---------------
src/nwfilter/nwfilter_driver.c | 33 ++++++++++-------
2 files changed, 72 insertions(+), 41 deletions(-)
diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c
index cb697f9..3c6bdbb 100644
--- a/src/conf/virnwfilterobj.c
+++ b/src/conf/virnwfilterobj.c
@@ -37,11 +37,16 @@ VIR_LOG_INIT("conf.virnwfilterobj");
void
virNWFilterObjFree(virNWFilterObjPtr obj)
{
+ virNWFilterDefPtr def;
+ virNWFilterDefPtr newDef;
+
if (!obj)
return;
+ def = obj->def;
+ newDef = obj->newDef;
- virNWFilterDefFree(obj->def);
- virNWFilterDefFree(obj->newDef);
+ virNWFilterDefFree(def);
+ virNWFilterDefFree(newDef);
virMutexDestroy(&obj->lock);
@@ -87,12 +92,16 @@ virNWFilterObjFindByUUID(virNWFilterObjListPtr nwfilters,
const unsigned char *uuid)
{
size_t i;
+ virNWFilterObjPtr obj;
+ virNWFilterDefPtr def;
for (i = 0; i < nwfilters->count; i++) {
- virNWFilterObjLock(nwfilters->objs[i]);
- if (!memcmp(nwfilters->objs[i]->def->uuid, uuid, VIR_UUID_BUFLEN))
- return nwfilters->objs[i];
- virNWFilterObjUnlock(nwfilters->objs[i]);
+ obj = nwfilters->objs[i];
+ virNWFilterObjLock(obj);
+ def = obj->def;
+ if (!memcmp(def->uuid, uuid, VIR_UUID_BUFLEN))
+ return obj;
+ virNWFilterObjUnlock(obj);
}
return NULL;
@@ -104,12 +113,16 @@ virNWFilterObjFindByName(virNWFilterObjListPtr nwfilters,
const char *name)
{
size_t i;
+ virNWFilterObjPtr obj;
+ virNWFilterDefPtr def;
for (i = 0; i < nwfilters->count; i++) {
- virNWFilterObjLock(nwfilters->objs[i]);
- if (STREQ_NULLABLE(nwfilters->objs[i]->def->name, name))
- return nwfilters->objs[i];
- virNWFilterObjUnlock(nwfilters->objs[i]);
+ obj = nwfilters->objs[i];
+ virNWFilterObjLock(obj);
+ def = obj->def;
+ if (STREQ_NULLABLE(def->name, name))
+ return obj;
+ virNWFilterObjUnlock(obj);
}
return NULL;
@@ -125,6 +138,7 @@ _virNWFilterObjDefLoopDetect(virNWFilterObjListPtr nwfilters,
size_t i;
virNWFilterEntryPtr entry;
virNWFilterObjPtr obj;
+ virNWFilterDefPtr objdef;
if (!def)
return 0;
@@ -141,9 +155,9 @@ _virNWFilterObjDefLoopDetect(virNWFilterObjListPtr nwfilters,
obj = virNWFilterObjFindByName(nwfilters,
entry->include->filterref);
if (obj) {
- rc = _virNWFilterObjDefLoopDetect(nwfilters,
- obj->def, filtername);
-
+ objdef = obj->def;
+ rc = _virNWFilterObjDefLoopDetect(nwfilters, objdef,
+ filtername);
virNWFilterObjUnlock(obj);
if (rc < 0)
break;
@@ -226,24 +240,26 @@ virNWFilterObjAssignDef(virNWFilterObjListPtr nwfilters,
virNWFilterDefPtr def)
{
virNWFilterObjPtr obj;
+ virNWFilterDefPtr objdef;
- obj = virNWFilterObjFindByUUID(nwfilters, def->uuid);
+ if ((obj = virNWFilterObjFindByUUID(nwfilters, def->uuid))) {
+ objdef = obj->def;
- if (obj) {
- if (STRNEQ(def->name, obj->def->name)) {
+ if (STRNEQ(def->name, objdef->name)) {
virReportError(VIR_ERR_OPERATION_FAILED,
_("filter with same UUID but different name "
"('%s') already exists"),
- obj->def->name);
+ objdef->name);
virNWFilterObjUnlock(obj);
return NULL;
}
virNWFilterObjUnlock(obj);
} else {
- obj = virNWFilterObjFindByName(nwfilters, def->name);
- if (obj) {
+ if ((obj = virNWFilterObjFindByName(nwfilters, def->name))) {
char uuidstr[VIR_UUID_STRING_BUFLEN];
- virUUIDFormat(obj->def->uuid, uuidstr);
+
+ objdef = obj->def;
+ virUUIDFormat(objdef->uuid, uuidstr);
virReportError(VIR_ERR_OPERATION_FAILED,
_("filter '%s' already exists with uuid %s"),
def->name, uuidstr);
@@ -261,8 +277,9 @@ virNWFilterObjAssignDef(virNWFilterObjListPtr nwfilters,
if ((obj = virNWFilterObjFindByName(nwfilters, def->name))) {
- if (virNWFilterDefEqual(def, obj->def, false)) {
- virNWFilterDefFree(obj->def);
+ objdef = obj->def;
+ if (virNWFilterDefEqual(def, objdef, false)) {
+ virNWFilterDefFree(objdef);
obj->def = def;
return obj;
}
@@ -275,7 +292,7 @@ virNWFilterObjAssignDef(virNWFilterObjListPtr nwfilters,
return NULL;
}
- virNWFilterDefFree(obj->def);
+ virNWFilterDefFree(objdef);
obj->def = def;
obj->newDef = NULL;
return obj;
@@ -312,11 +329,13 @@ virNWFilterObjNumOfNWFilters(virNWFilterObjListPtr nwfilters,
{
size_t i;
int nfilters = 0;
+ virNWFilterDefPtr def;
for (i = 0; i < nwfilters->count; i++) {
virNWFilterObjPtr obj = nwfilters->objs[i];
virNWFilterObjLock(obj);
- if (!aclfilter || aclfilter(conn, obj->def))
+ def = obj->def;
+ if (!aclfilter || aclfilter(conn, def))
nfilters++;
virNWFilterObjUnlock(obj);
}
@@ -334,12 +353,14 @@ virNWFilterObjGetNames(virNWFilterObjListPtr nwfilters,
{
int nnames = 0;
size_t i;
+ virNWFilterDefPtr def;
for (i = 0; i < nwfilters->count && nnames < maxnames; i++) {
virNWFilterObjPtr obj = nwfilters->objs[i];
virNWFilterObjLock(obj);
- if (!aclfilter || aclfilter(conn, obj->def)) {
- if (VIR_STRDUP(names[nnames], obj->def->name) < 0) {
+ def = obj->def;
+ if (!aclfilter || aclfilter(conn, def)) {
+ if (VIR_STRDUP(names[nnames], def->name) < 0) {
virNWFilterObjUnlock(obj);
goto failure;
}
@@ -368,6 +389,7 @@ virNWFilterObjListExport(virConnectPtr conn,
int nfilters = 0;
virNWFilterPtr filter = NULL;
virNWFilterObjPtr obj = NULL;
+ virNWFilterDefPtr def;
size_t i;
int ret = -1;
@@ -382,9 +404,9 @@ virNWFilterObjListExport(virConnectPtr conn,
for (i = 0; i < nwfilters->count; i++) {
obj = nwfilters->objs[i];
virNWFilterObjLock(obj);
- if (!aclfilter || aclfilter(conn, obj->def)) {
- if (!(filter = virGetNWFilter(conn, obj->def->name,
- obj->def->uuid))) {
+ def = obj->def;
+ if (!aclfilter || aclfilter(conn, def)) {
+ if (!(filter = virGetNWFilter(conn, def->name, def->uuid))) {
virNWFilterObjUnlock(obj);
goto cleanup;
}
diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
index 72b9b8e..7e88a40 100644
--- a/src/nwfilter/nwfilter_driver.c
+++ b/src/nwfilter/nwfilter_driver.c
@@ -358,6 +358,7 @@ nwfilterLookupByUUID(virConnectPtr conn,
const unsigned char *uuid)
{
virNWFilterObjPtr obj;
+ virNWFilterDefPtr def;
virNWFilterPtr ret = NULL;
nwfilterDriverLock();
@@ -369,11 +370,12 @@ nwfilterLookupByUUID(virConnectPtr conn,
"%s", _("no nwfilter with matching uuid"));
goto cleanup;
}
+ def = obj->def;
- if (virNWFilterLookupByUUIDEnsureACL(conn, obj->def) < 0)
+ if (virNWFilterLookupByUUIDEnsureACL(conn, def) < 0)
goto cleanup;
- ret = virGetNWFilter(conn, obj->def->name, obj->def->uuid);
+ ret = virGetNWFilter(conn, def->name, def->uuid);
cleanup:
if (obj)
@@ -387,6 +389,7 @@ nwfilterLookupByName(virConnectPtr conn,
const char *name)
{
virNWFilterObjPtr obj;
+ virNWFilterDefPtr def;
virNWFilterPtr ret = NULL;
nwfilterDriverLock();
@@ -398,11 +401,12 @@ nwfilterLookupByName(virConnectPtr conn,
_("no nwfilter with matching name '%s'"), name);
goto cleanup;
}
+ def = obj->def;
- if (virNWFilterLookupByNameEnsureACL(conn, obj->def) < 0)
+ if (virNWFilterLookupByNameEnsureACL(conn, def) < 0)
goto cleanup;
- ret = virGetNWFilter(conn, obj->def->name, obj->def->uuid);
+ ret = virGetNWFilter(conn, def->name, def->uuid);
cleanup:
if (obj)
@@ -467,6 +471,7 @@ nwfilterDefineXML(virConnectPtr conn,
{
virNWFilterDefPtr def;
virNWFilterObjPtr obj = NULL;
+ virNWFilterDefPtr objdef;
virNWFilterPtr ret = NULL;
if (!driver->privileged) {
@@ -487,15 +492,15 @@ nwfilterDefineXML(virConnectPtr conn,
if (!(obj = virNWFilterObjAssignDef(&driver->nwfilters, def)))
goto cleanup;
+ def = NULL;
+ objdef = obj->def;
- if (virNWFilterSaveDef(driver->configDir, def) < 0) {
+ if (virNWFilterSaveDef(driver->configDir, objdef) < 0) {
virNWFilterObjRemove(&driver->nwfilters, obj);
- def = NULL;
goto cleanup;
}
- def = NULL;
- ret = virGetNWFilter(conn, obj->def->name, obj->def->uuid);
+ ret = virGetNWFilter(conn, objdef->name, objdef->uuid);
cleanup:
virNWFilterDefFree(def);
@@ -513,6 +518,7 @@ static int
nwfilterUndefine(virNWFilterPtr nwfilter)
{
virNWFilterObjPtr obj;
+ virNWFilterDefPtr def;
int ret = -1;
nwfilterDriverLock();
@@ -525,8 +531,9 @@ nwfilterUndefine(virNWFilterPtr nwfilter)
"%s", _("no nwfilter with matching uuid"));
goto cleanup;
}
+ def = obj->def;
- if (virNWFilterUndefineEnsureACL(nwfilter->conn, obj->def) < 0)
+ if (virNWFilterUndefineEnsureACL(nwfilter->conn, def) < 0)
goto cleanup;
if (virNWFilterObjTestUnassignDef(obj) < 0) {
@@ -536,7 +543,7 @@ nwfilterUndefine(virNWFilterPtr nwfilter)
goto cleanup;
}
- if (virNWFilterDeleteDef(driver->configDir, obj->def) < 0)
+ if (virNWFilterDeleteDef(driver->configDir, def) < 0)
goto cleanup;
virNWFilterObjRemove(&driver->nwfilters, obj);
@@ -559,6 +566,7 @@ nwfilterGetXMLDesc(virNWFilterPtr nwfilter,
unsigned int flags)
{
virNWFilterObjPtr obj;
+ virNWFilterDefPtr def;
char *ret = NULL;
virCheckFlags(0, NULL);
@@ -572,11 +580,12 @@ nwfilterGetXMLDesc(virNWFilterPtr nwfilter,
"%s", _("no nwfilter with matching uuid"));
goto cleanup;
}
+ def = obj->def;
- if (virNWFilterGetXMLDescEnsureACL(nwfilter->conn, obj->def) < 0)
+ if (virNWFilterGetXMLDescEnsureACL(nwfilter->conn, def) < 0)
goto cleanup;
- ret = virNWFilterDefFormat(obj->def);
+ ret = virNWFilterDefFormat(def);
cleanup:
if (obj)
--
2.9.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Apr 24, 2017 at 03:18:31PM -0400, John Ferlan wrote:
> Rather than dereferencing obj->def->XXX or nwfilters->objs[i]->X
> create local virNWFilterObjPtr and virNWFilterDefPtr variables.
>
> Future adjustments will be privatizing the object more, so this just
> prepares the code for that reality.
>
> Signed-off-by: John Ferlan <jferlan@redhat.com>
> ---
> src/conf/virnwfilterobj.c | 80 +++++++++++++++++++++++++++---------------
> src/nwfilter/nwfilter_driver.c | 33 ++++++++++-------
> 2 files changed, 72 insertions(+), 41 deletions(-)
>
> diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c
> index cb697f9..3c6bdbb 100644
> --- a/src/conf/virnwfilterobj.c
> +++ b/src/conf/virnwfilterobj.c
> @@ -37,11 +37,16 @@ VIR_LOG_INIT("conf.virnwfilterobj");
> void
> virNWFilterObjFree(virNWFilterObjPtr obj)
> {
> + virNWFilterDefPtr def;
> + virNWFilterDefPtr newDef;
> +
> if (!obj)
> return;
> + def = obj->def;
> + newDef = obj->newDef;
>
> - virNWFilterDefFree(obj->def);
> - virNWFilterDefFree(obj->newDef);
> + virNWFilterDefFree(def);
> + virNWFilterDefFree(newDef);
This was discussed in the secret cleanup series. In this case it just adds
some lines to the code without any real benefit, so it's just a noise in this
case. This change makes sense for functions where the *def* is used several
times, but for those simple usages of def there is no point of having a
separate variable.
Now I know that some future patches may benefit from this change, however
there is no guarantee that the patches will be accepted and pushed I think
that you should wait with these changes for that future series.
Pavel
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 04/25/2017 09:18 AM, Pavel Hrdina wrote:
> On Mon, Apr 24, 2017 at 03:18:31PM -0400, John Ferlan wrote:
>> Rather than dereferencing obj->def->XXX or nwfilters->objs[i]->X
>> create local virNWFilterObjPtr and virNWFilterDefPtr variables.
>>
>> Future adjustments will be privatizing the object more, so this just
>> prepares the code for that reality.
>>
>> Signed-off-by: John Ferlan <jferlan@redhat.com>
>> ---
>> src/conf/virnwfilterobj.c | 80 +++++++++++++++++++++++++++---------------
>> src/nwfilter/nwfilter_driver.c | 33 ++++++++++-------
>> 2 files changed, 72 insertions(+), 41 deletions(-)
>>
>> diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c
>> index cb697f9..3c6bdbb 100644
>> --- a/src/conf/virnwfilterobj.c
>> +++ b/src/conf/virnwfilterobj.c
>> @@ -37,11 +37,16 @@ VIR_LOG_INIT("conf.virnwfilterobj");
>> void
>> virNWFilterObjFree(virNWFilterObjPtr obj)
>> {
>> + virNWFilterDefPtr def;
>> + virNWFilterDefPtr newDef;
>> +
>> if (!obj)
>> return;
>> + def = obj->def;
>> + newDef = obj->newDef;
>>
>> - virNWFilterDefFree(obj->def);
>> - virNWFilterDefFree(obj->newDef);
>> + virNWFilterDefFree(def);
>> + virNWFilterDefFree(newDef);
>
> This was discussed in the secret cleanup series. In this case it just adds
> some lines to the code without any real benefit, so it's just a noise in this
> case. This change makes sense for functions where the *def* is used several
> times, but for those simple usages of def there is no point of having a
> separate variable.
>
> Now I know that some future patches may benefit from this change, however
> there is no guarantee that the patches will be accepted and pushed I think
> that you should wait with these changes for that future series.
>
> Pavel
>
I wouldn't be the first and I won't be the last to make/post changes to
code to help some future yet unposted series that has the some 'issue'
w/r/t a "guarantee" that it would accepted/pushed.
Like I pointed out in the secrets series - stopping progress here
because it's undesirable to have @def alone doesn't mean it's not
technically correct and the reality is the compiler will do whatever it
wants...
Again, like the secrets code I can agree if it's just "obj->def", I can
restore that, but once it's "obj->def->X", then having a "def" is more
desirable regardless if there's only one such reference in the code.
Pulling this patch from the entire series begins to impact (e.g. force
me down the git merge path) starting at patch 5. So it's very painful
to do.
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Apr 25, 2017 at 11:42:37AM -0400, John Ferlan wrote:
>
>
> On 04/25/2017 09:18 AM, Pavel Hrdina wrote:
> > On Mon, Apr 24, 2017 at 03:18:31PM -0400, John Ferlan wrote:
> >> Rather than dereferencing obj->def->XXX or nwfilters->objs[i]->X
> >> create local virNWFilterObjPtr and virNWFilterDefPtr variables.
> >>
> >> Future adjustments will be privatizing the object more, so this just
> >> prepares the code for that reality.
> >>
> >> Signed-off-by: John Ferlan <jferlan@redhat.com>
> >> ---
> >> src/conf/virnwfilterobj.c | 80 +++++++++++++++++++++++++++---------------
> >> src/nwfilter/nwfilter_driver.c | 33 ++++++++++-------
> >> 2 files changed, 72 insertions(+), 41 deletions(-)
> >>
> >> diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c
> >> index cb697f9..3c6bdbb 100644
> >> --- a/src/conf/virnwfilterobj.c
> >> +++ b/src/conf/virnwfilterobj.c
> >> @@ -37,11 +37,16 @@ VIR_LOG_INIT("conf.virnwfilterobj");
> >> void
> >> virNWFilterObjFree(virNWFilterObjPtr obj)
> >> {
> >> + virNWFilterDefPtr def;
> >> + virNWFilterDefPtr newDef;
> >> +
> >> if (!obj)
> >> return;
> >> + def = obj->def;
> >> + newDef = obj->newDef;
> >>
> >> - virNWFilterDefFree(obj->def);
> >> - virNWFilterDefFree(obj->newDef);
> >> + virNWFilterDefFree(def);
> >> + virNWFilterDefFree(newDef);
> >
> > This was discussed in the secret cleanup series. In this case it just adds
> > some lines to the code without any real benefit, so it's just a noise in this
> > case. This change makes sense for functions where the *def* is used several
> > times, but for those simple usages of def there is no point of having a
> > separate variable.
> >
> > Now I know that some future patches may benefit from this change, however
> > there is no guarantee that the patches will be accepted and pushed I think
> > that you should wait with these changes for that future series.
> >
> > Pavel
> >
>
> I wouldn't be the first and I won't be the last to make/post changes to
> code to help some future yet unposted series that has the some 'issue'
> w/r/t a "guarantee" that it would accepted/pushed.
>
> Like I pointed out in the secrets series - stopping progress here
> because it's undesirable to have @def alone doesn't mean it's not
> technically correct and the reality is the compiler will do whatever it
> wants...
>
> Again, like the secrets code I can agree if it's just "obj->def", I can
> restore that, but once it's "obj->def->X", then having a "def" is more
> desirable regardless if there's only one such reference in the code.
That sounds reasonable, I can agree on that. I don't want to stop
any progress, I just didn't like the first case that will be dropped.
Pavel
> Pulling this patch from the entire series begins to impact (e.g. force
> me down the git merge path) starting at patch 5. So it's very painful
> to do.
>
> John
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.