2:44 p.m.
In a number of places in the bhyve driver, virObjectUnlock()
is called with an arg without check if the arg is non-NULL, which
could result in passing NULL value and a warning like:
virObjectUnlock:340 : Object 0x0 ((unknown)) is not a virObjectLockable instance
* src/bhyve/bhyve_driver.c (bhyveDomainGetInfo)
(bhyveDomainGetState, bhyveDomainGetAutostart)
(bhyveDomainSetAutostart, bhyveDomainIsActive)
(bhyveDomainIsPersistent, bhyveDomainGetXMLDesc)
(bhyveDomainUndefine, bhyveDomainLookupByUUID)
(bhyveDomainLookupByName, bhyveDomainLookupByID)
(bhyveDomainCreateWithFlags, bhyveDomainOpenConsole):
Check if arg is not NULL before calling virObjectUnlock on it.
---
src/bhyve/bhyve_driver.c | 39 ++++++++++++++++++++++++++-------------
1 file changed, 26 insertions(+), 13 deletions(-)
diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c
index 580b124..c8902bf 100644
--- a/src/bhyve/bhyve_driver.c
+++ b/src/bhyve/bhyve_driver.c
@@ -309,7 +309,8 @@ bhyveDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info)
ret = 0;
cleanup:
- virObjectUnlock(vm);
+ if (vm)
+ virObjectUnlock(vm);
return ret;
}
@@ -334,7 +335,8 @@ bhyveDomainGetState(virDomainPtr domain,
ret = 0;
cleanup:
- virObjectUnlock(vm);
+ if (vm)
+ virObjectUnlock(vm);
return ret;
}
@@ -354,7 +356,8 @@ bhyveDomainGetAutostart(virDomainPtr domain, int *autostart)
ret = 0;
cleanup:
- virObjectUnlock(vm);
+ if (vm)
+ virObjectUnlock(vm);
return ret;
}
@@ -417,7 +420,8 @@ bhyveDomainSetAutostart(virDomainPtr domain, int autostart)
cleanup:
VIR_FREE(configFile);
VIR_FREE(autostartLink);
- virObjectUnlock(vm);
+ if (vm)
+ virObjectUnlock(vm);
return ret;
}
@@ -436,7 +440,8 @@ bhyveDomainIsActive(virDomainPtr domain)
ret = virDomainObjIsActive(obj);
cleanup:
- virObjectUnlock(obj);
+ if (obj)
+ virObjectUnlock(obj);
return ret;
}
@@ -455,7 +460,8 @@ bhyveDomainIsPersistent(virDomainPtr domain)
ret = obj->persistent;
cleanup:
- virObjectUnlock(obj);
+ if (obj)
+ virObjectUnlock(obj);
return ret;
}
@@ -474,7 +480,8 @@ bhyveDomainGetXMLDesc(virDomainPtr domain, unsigned int flags)
ret = virDomainDefFormat(vm->def, flags);
cleanup:
- virObjectUnlock(vm);
+ if (vm)
+ virObjectUnlock(vm);
return ret;
}
@@ -562,7 +569,8 @@ bhyveDomainUndefine(virDomainPtr domain)
ret = 0;
cleanup:
- virObjectUnlock(vm);
+ if (vm)
+ virObjectUnlock(vm);
return ret;
}
@@ -730,7 +738,8 @@ bhyveDomainLookupByUUID(virConnectPtr conn,
dom->id = vm->def->id;
cleanup:
- virObjectUnlock(vm);
+ if (vm)
+ virObjectUnlock(vm);
return dom;
}
@@ -757,7 +766,8 @@ static virDomainPtr bhyveDomainLookupByName(virConnectPtr conn,
dom->id = vm->def->id;
cleanup:
- virObjectUnlock(vm);
+ if (vm)
+ virObjectUnlock(vm);
return dom;
}
@@ -785,7 +795,8 @@ bhyveDomainLookupByID(virConnectPtr conn,
dom->id = vm->def->id;
cleanup:
- virObjectUnlock(vm);
+ if (vm)
+ virObjectUnlock(vm);
return dom;
}
@@ -820,7 +831,8 @@ bhyveDomainCreateWithFlags(virDomainPtr dom,
start_flags);
cleanup:
- virObjectUnlock(vm);
+ if (vm)
+ virObjectUnlock(vm);
return ret;
}
@@ -960,7 +972,8 @@ bhyveDomainOpenConsole(virDomainPtr dom,
ret = 0;
cleanup:
- virObjectUnlock(vm);
+ if (vm)
+ virObjectUnlock(vm);
return ret;
}
--
1.9.0
5:38 a.m.
On Sat, May 17, 2014 at 11:44:48PM +0400, Roman Bogorodskiy wrote:
In a number of places in the bhyve driver, virObjectUnlock()
is called with an arg without check if the arg is non-NULL, which
could result in passing NULL value and a warning like:
virObjectUnlock:340 : Object 0x0 ((unknown)) is not a virObjectLockable instance
* src/bhyve/bhyve_driver.c (bhyveDomainGetInfo)
(bhyveDomainGetState, bhyveDomainGetAutostart)
(bhyveDomainSetAutostart, bhyveDomainIsActive)
(bhyveDomainIsPersistent, bhyveDomainGetXMLDesc)
(bhyveDomainUndefine, bhyveDomainLookupByUUID)
(bhyveDomainLookupByName, bhyveDomainLookupByID)
(bhyveDomainCreateWithFlags, bhyveDomainOpenConsole):
Check if arg is not NULL before calling virObjectUnlock on it.
---
src/bhyve/bhyve_driver.c | 39 ++++++++++++++++++++++++++-------------
1 file changed, 26 insertions(+), 13 deletions(-)
ACK
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
10:55 a.m.
Daniel P. Berrange wrote:
On Sat, May 17, 2014 at 11:44:48PM +0400, Roman Bogorodskiy wrote:
> In a number of places in the bhyve driver, virObjectUnlock()
> is called with an arg without check if the arg is non-NULL, which
> could result in passing NULL value and a warning like:
>
> virObjectUnlock:340 : Object 0x0 ((unknown)) is not a virObjectLockable instance
>
> * src/bhyve/bhyve_driver.c (bhyveDomainGetInfo)
> (bhyveDomainGetState, bhyveDomainGetAutostart)
> (bhyveDomainSetAutostart, bhyveDomainIsActive)
> (bhyveDomainIsPersistent, bhyveDomainGetXMLDesc)
> (bhyveDomainUndefine, bhyveDomainLookupByUUID)
> (bhyveDomainLookupByName, bhyveDomainLookupByID)
> (bhyveDomainCreateWithFlags, bhyveDomainOpenConsole):
> Check if arg is not NULL before calling virObjectUnlock on it.
> ---
> src/bhyve/bhyve_driver.c | 39 ++++++++++++++++++++++++++-------------
> 1 file changed, 26 insertions(+), 13 deletions(-)
ACK
Pushed, thanks!
Roman Bogorodskiy
11:45 a.m.
On 05/17/2014 01:44 PM, Roman Bogorodskiy wrote:
In a number of places in the bhyve driver, virObjectUnlock()
is called with an arg without check if the arg is non-NULL, which
could result in passing NULL value and a warning like:
virObjectUnlock:340 : Object 0x0 ((unknown)) is not a virObjectLockable instance
Doesn't this instead argue that we should fix virObjectUnlock to
gracefully handle a NULL parameter, rather than making every caller uglier?
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
11:57 a.m.
Eric Blake wrote:
On 05/17/2014 01:44 PM, Roman Bogorodskiy wrote:
> In a number of places in the bhyve driver, virObjectUnlock()
> is called with an arg without check if the arg is non-NULL, which
> could result in passing NULL value and a warning like:
>
> virObjectUnlock:340 : Object 0x0 ((unknown)) is not a virObjectLockable instance
Doesn't this instead argue that we should fix virObjectUnlock to
gracefully handle a NULL parameter, rather than making every caller uglier?
Calling it with NULL seems to be harmless anyway and the only problem
is this annoying warning.
So, what you propose is checking explicitly for NULL in
virObjectUnlock before we do virObjectIsClass(), and if the passed
argument is NULL indeed, just return, without logging anything about
that?
BTW, that's going to be a vast change, quick grep shows more than 750
calls of that function.
Roman Bogorodskiy
12:12 p.m.
On 05/19/2014 10:57 AM, Roman Bogorodskiy wrote:
Eric Blake wrote:
> On 05/17/2014 01:44 PM, Roman Bogorodskiy wrote:
>> In a number of places in the bhyve driver, virObjectUnlock()
>> is called with an arg without check if the arg is non-NULL, which
>> could result in passing NULL value and a warning like:
>>
>> virObjectUnlock:340 : Object 0x0 ((unknown)) is not a virObjectLockable instance
>
> Doesn't this instead argue that we should fix virObjectUnlock to
> gracefully handle a NULL parameter, rather than making every caller uglier?
Calling it with NULL seems to be harmless anyway and the only problem
is this annoying warning.
So, what you propose is checking explicitly for NULL in
virObjectUnlock before we do virObjectIsClass(), and if the passed
argument is NULL indeed, just return, without logging anything about
that?
Yes, since we have other virObject code that special cases NULL (for
example, virObjectUnref).
BTW, that's going to be a vast change, quick grep shows more than 750
calls of that function.
It doesn't invalidate any existing caller to make virObjectUnlock()
special-case NULL. And while it DOES make any existing caller that also
checks for NULL be a case of redundant code, we don't have to clean all
callers up in a single patch.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
12:26 p.m.
Eric Blake wrote:
> BTW, that's going to be a vast change, quick grep shows more
than 750
> calls of that function.
It doesn't invalidate any existing caller to make virObjectUnlock()
special-case NULL. And while it DOES make any existing caller that also
checks for NULL be a case of redundant code, we don't have to clean all
callers up in a single patch.
Good, so I'll add a check for NULL in virObjectUnlock() and prepare a
series that cleans up the redundant if (obj) check before calling
virObjectUnlock(obj), say, on per-driver basis.
Roman Bogorodskiy
5:19 p.m.
On Mon, May 19, 2014 at 11:12:05AM -0600, Eric Blake wrote:
On 05/19/2014 10:57 AM, Roman Bogorodskiy wrote:
> Eric Blake wrote:
>
>> On 05/17/2014 01:44 PM, Roman Bogorodskiy wrote:
>>> In a number of places in the bhyve driver, virObjectUnlock()
>>> is called with an arg without check if the arg is non-NULL, which
>>> could result in passing NULL value and a warning like:
>>>
>>> virObjectUnlock:340 : Object 0x0 ((unknown)) is not a virObjectLockable
instance
>>
>> Doesn't this instead argue that we should fix virObjectUnlock to
>> gracefully handle a NULL parameter, rather than making every caller uglier?
>
> Calling it with NULL seems to be harmless anyway and the only problem
> is this annoying warning.
>
> So, what you propose is checking explicitly for NULL in
> virObjectUnlock before we do virObjectIsClass(), and if the passed
> argument is NULL indeed, just return, without logging anything about
> that?
Yes, since we have other virObject code that special cases NULL (for
example, virObjectUnref).
IMHO passing NULL into the locking APIs is a coding error we shouldn't
try to paper over by ignoring.
Ultimately I think the real flaw is the way we obtain the virDomainPtr
pointers in the first place. ie the virDomainObjListLookup functions
don't acquire a reference on the object they return. So the caller has
to worry about the object being released behind their back, hence all
our logic which has to set 'vm = NULL' in various places where it might
have been deleted.
IOW, I'd much rather we looked at changing our design here so that we
didn't have so much NULL vm pointers in the first place.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
9:02 a.m.
Daniel P. Berrange wrote:
On Mon, May 19, 2014 at 11:12:05AM -0600, Eric Blake wrote:
> On 05/19/2014 10:57 AM, Roman Bogorodskiy wrote:
> > Eric Blake wrote:
> >
> >> On 05/17/2014 01:44 PM, Roman Bogorodskiy wrote:
> >>> In a number of places in the bhyve driver, virObjectUnlock()
> >>> is called with an arg without check if the arg is non-NULL, which
> >>> could result in passing NULL value and a warning like:
> >>>
> >>> virObjectUnlock:340 : Object 0x0 ((unknown)) is not a virObjectLockable
instance
> >>
> >> Doesn't this instead argue that we should fix virObjectUnlock to
> >> gracefully handle a NULL parameter, rather than making every caller
uglier?
> >
> > Calling it with NULL seems to be harmless anyway and the only problem
> > is this annoying warning.
> >
> > So, what you propose is checking explicitly for NULL in
> > virObjectUnlock before we do virObjectIsClass(), and if the passed
> > argument is NULL indeed, just return, without logging anything about
> > that?
>
> Yes, since we have other virObject code that special cases NULL (for
> example, virObjectUnref).
IMHO passing NULL into the locking APIs is a coding error we shouldn't
try to paper over by ignoring.
Ultimately I think the real flaw is the way we obtain the virDomainPtr
pointers in the first place. ie the virDomainObjListLookup functions
don't acquire a reference on the object they return. So the caller has
to worry about the object being released behind their back, hence all
our logic which has to set 'vm = NULL' in various places where it might
have been deleted.
IOW, I'd much rather we looked at changing our design here so that we
didn't have so much NULL vm pointers in the first place.
Eric, could you please comment on that?
Roman Bogorodskiy
9:29 a.m.
On 05/21/2014 08:02 AM, Roman Bogorodskiy wrote:
>>> So, what you propose is checking explicitly for NULL in
>>> virObjectUnlock before we do virObjectIsClass(), and if the passed
>>> argument is NULL indeed, just return, without logging anything about
>>> that?
>>
>> Yes, since we have other virObject code that special cases NULL (for
>> example, virObjectUnref).
>
> IMHO passing NULL into the locking APIs is a coding error we shouldn't
> try to paper over by ignoring.
>
> Ultimately I think the real flaw is the way we obtain the virDomainPtr
> pointers in the first place. ie the virDomainObjListLookup functions
> don't acquire a reference on the object they return. So the caller has
> to worry about the object being released behind their back, hence all
> our logic which has to set 'vm = NULL' in various places where it might
> have been deleted.
>
> IOW, I'd much rather we looked at changing our design here so that we
> didn't have so much NULL vm pointers in the first place.
Eric, could you please comment on that?
I trust Daniel's judgment here, since he wrote virObjectPtr and
virObjectUnlock. Cleaning up the DomainObjListLookup function to grab a
reference will have a big ripple effect, so it probably doesn't need to
be done now (that is, you don't necessarily have to be the one doing the
cleanup he proposes); but I guess that means for the present that we are
still stuck with the current design pattern of checking for NULL ourself
before calling virObjectUnlock.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
10:41 a.m.
Eric Blake wrote:
On 05/21/2014 08:02 AM, Roman Bogorodskiy wrote:
>>>> So, what you propose is checking explicitly for NULL in
>>>> virObjectUnlock before we do virObjectIsClass(), and if the passed
>>>> argument is NULL indeed, just return, without logging anything about
>>>> that?
>>>
>>> Yes, since we have other virObject code that special cases NULL (for
>>> example, virObjectUnref).
>>
>> IMHO passing NULL into the locking APIs is a coding error we shouldn't
>> try to paper over by ignoring.
>>
>> Ultimately I think the real flaw is the way we obtain the virDomainPtr
>> pointers in the first place. ie the virDomainObjListLookup functions
>> don't acquire a reference on the object they return. So the caller has
>> to worry about the object being released behind their back, hence all
>> our logic which has to set 'vm = NULL' in various places where it might
>> have been deleted.
>>
>> IOW, I'd much rather we looked at changing our design here so that we
>> didn't have so much NULL vm pointers in the first place.
>
> Eric, could you please comment on that?
I trust Daniel's judgment here, since he wrote virObjectPtr and
virObjectUnlock. Cleaning up the DomainObjListLookup function to grab a
reference will have a big ripple effect, so it probably doesn't need to
be done now (that is, you don't necessarily have to be the one doing the
cleanup he proposes); but I guess that means for the present that we are
still stuck with the current design pattern of checking for NULL ourself
before calling virObjectUnlock.
OK, then I'll push the current bhyve patch that was already ACKed by
Daniel and leave virObjectUnlock related cleanup for now; thanks.
Roman Bogorodskiy
3841
days inactive
3845
days old
10 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Roman Bogorodskiy