[libvirt] [PATCH] Don't free NULL network in cmdNetworkUpdate
If the network has not been found, virNetworkFree(NULL) was called, resulting in an extra error: error: invalid network pointer in virNetworkFree https://bugzilla.redhat.com/show_bug.cgi?id=1001094 --- tools/virsh-network.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/virsh-network.c b/tools/virsh-network.c index e1baf0b..06bf483 100644 --- a/tools/virsh-network.c +++ b/tools/virsh-network.c @@ -918,7 +918,7 @@ cmdNetworkUpdate(vshControl *ctl, const vshCmd *cmd) const char *affected; if (!(network = vshCommandOptNetwork(ctl, cmd, NULL))) - goto cleanup; + return false; if (vshCommandOptStringReq(ctl, cmd, "command", &commandStr) < 0) goto cleanup; -- 1.8.1.5
On 08/26/13 15:39, Ján Tomko wrote:
If the network has not been found, virNetworkFree(NULL) was called, resulting in an extra error: error: invalid network pointer in virNetworkFree
https://bugzilla.redhat.com/show_bug.cgi?id=1001094 --- tools/virsh-network.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK. Peter
On 08/26/2013 03:50 PM, Peter Krempa wrote:
On 08/26/13 15:39, Ján Tomko wrote:
If the network has not been found, virNetworkFree(NULL) was called, resulting in an extra error: error: invalid network pointer in virNetworkFree
https://bugzilla.redhat.com/show_bug.cgi?id=1001094 --- tools/virsh-network.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK.
Peter
Thanks, pushed. Jan
On 08/26/2013 09:39 AM, Ján Tomko wrote:
If the network has not been found, virNetworkFree(NULL) was called, resulting in an extra error: error: invalid network pointer in virNetworkFree
The patch is okay, but all vir*Free() functions should be NOPs when the argument is NULL.
On 27.08.2013 14:03, Laine Stump wrote:
On 08/26/2013 09:39 AM, Ján Tomko wrote:
If the network has not been found, virNetworkFree(NULL) was called, resulting in an extra error: error: invalid network pointer in virNetworkFree
The patch is okay, but all vir*Free() functions should be NOPs when the argument is NULL.
Just internal vir*Free() functions behave that way. The public ones, like virNetworkFree() don't. I was thinking a while ago why we have different approach like this, but the thought got buried under other stuff. Michal
On 08/27/2013 06:03 AM, Laine Stump wrote:
On 08/26/2013 09:39 AM, Ján Tomko wrote:
If the network has not been found, virNetworkFree(NULL) was called, resulting in an extra error: error: invalid network pointer in virNetworkFree
The patch is okay, but all vir*Free() functions should be NOPs when the argument is NULL.
All _internal_ vir*Free functions should be NOPs. All public vir*Free APIs (including virNetworkFree) are documented to explicitly fail on NULL arguments (more precisely, on any argument not created by an earlier API call), and we cannot change that behavior as it would be an API change that violates back-compat. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 27.08.2013 14:07, Eric Blake wrote:
On 08/27/2013 06:03 AM, Laine Stump wrote:
On 08/26/2013 09:39 AM, Ján Tomko wrote:
If the network has not been found, virNetworkFree(NULL) was called, resulting in an extra error: error: invalid network pointer in virNetworkFree
The patch is okay, but all vir*Free() functions should be NOPs when the argument is NULL.
All _internal_ vir*Free functions should be NOPs. All public vir*Free APIs (including virNetworkFree) are documented to explicitly fail on NULL arguments (more precisely, on any argument not created by an earlier API call), and we cannot change that behavior as it would be an API change that violates back-compat.
Why it would break backward compatibility? I think it will break forward compatibility instead (which we don't care about). That is, currently apps have to use: virXPtr obj = NULL; obj = virLookupBy..(); ... do something with obj; ret = 0; cleanup: if (obj) virXFree(obj); However, if virXFree would accept NULL, it won't affect older applications, while new can just: cleanup: virXFree(obj); But then again, this is forward compatibility (the code won't work with downgraded libvirtd) - but who cares? Michal
On 08/27/2013 06:11 AM, Michal Privoznik wrote:
All _internal_ vir*Free functions should be NOPs. All public vir*Free APIs (including virNetworkFree) are documented to explicitly fail on NULL arguments (more precisely, on any argument not created by an earlier API call), and we cannot change that behavior as it would be an API change that violates back-compat.
Why it would break backward compatibility? I think it will break forward compatibility instead (which we don't care about). That is, currently apps have to use:
virXPtr obj = NULL;
obj = virLookupBy..();
... do something with obj;
ret = 0; cleanup: if (obj) virXFree(obj);
However, if virXFree would accept NULL, it won't affect older applications, while new can just:
cleanup: virXFree(obj);
But then again, this is forward compatibility (the code won't work with downgraded libvirtd) - but who cares?
Back-compat says because we documented it, a user can expect: virXFree(NULL); to raise an error. If we change things so that it no longer raises an error, then their code that was expecting an error may misbehave because it now goes into code paths that were not previously present. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 08/27/2013 08:07 AM, Eric Blake wrote:
On 08/27/2013 06:03 AM, Laine Stump wrote:
On 08/26/2013 09:39 AM, Ján Tomko wrote:
If the network has not been found, virNetworkFree(NULL) was called, resulting in an extra error: error: invalid network pointer in virNetworkFree
The patch is okay, but all vir*Free() functions should be NOPs when the argument is NULL. All _internal_ vir*Free functions should be NOPs. All public vir*Free APIs (including virNetworkFree) are documented to explicitly fail on NULL arguments (more precisely, on any argument not created by an earlier API call), and we cannot change that behavior as it would be an API change that violates back-compat.
Ah, sorry. I wasn't paying close attention, just saw a Free() function and wrote a knee-jerk reply before looking further.
participants (5)
-
Eric Blake -
Ján Tomko -
Laine Stump -
Michal Privoznik -
Peter Krempa