[libvirt] [PATCH] conf: Actually make virDomainChrSourceDef an object
In 2ada9ef1465f we've tried to turn virDomainChrSourceDef into virObject. Well, this requires 'virObject' member to be stored on the first position of the struct. This adjustment is missing in the original commit leading to all sorts of funny memleaks and data corruptions. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 89a7131fdb..1426f115ed 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -57,6 +57,7 @@ # include "virtypedparam.h" # include "virsavecookie.h" # include "virresctrl.h" +# include "virobject.h" /* forward declarations of all device types, required by * virDomainDeviceDef @@ -1180,6 +1181,7 @@ typedef virDomainChrSourceReconnectDef *virDomainChrSourceReconnectDefPtr; /* The host side information for a character device. */ struct _virDomainChrSourceDef { + virObject parent; int type; /* virDomainChrType */ virObjectPtr privateData; union { -- 2.16.1
On Thu, Apr 12, 2018 at 09:14:50AM +0200, Michal Privoznik wrote:
In 2ada9ef1465f we've tried to turn virDomainChrSourceDef into virObject. Well, this requires 'virObject' member to be stored on the first position of the struct. This adjustment is missing in the original commit leading to all sorts of funny memleaks and data corruptions.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.h | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 89a7131fdb..1426f115ed 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -57,6 +57,7 @@ # include "virtypedparam.h" # include "virsavecookie.h" # include "virresctrl.h" +# include "virobject.h"
syntax-check fails ^here because the header is already included on line 49 With that fixed: Reviewed-by: Erik Skultety <eskultet@redhat.com>
On 04/12/2018 04:44 AM, Erik Skultety wrote:
On Thu, Apr 12, 2018 at 09:14:50AM +0200, Michal Privoznik wrote:
In 2ada9ef1465f we've tried to turn virDomainChrSourceDef into virObject. Well, this requires 'virObject' member to be stored on the first position of the struct. This adjustment is missing in the original commit leading to all sorts of funny memleaks and data corruptions.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.h | 2 ++ 1 file changed, 2 insertions(+)
Doh! Thanks.... I guess not only do we need a way to detect that we're using VIR_ALLOC on an object (as Eric pointed out in my original patches), but we really should detect when we use virObjectNew without the virObject parent. For some reason, I have a recollection of altering changes during my meanderings through virObject code in order to point out more directly some misuses, but it was rejected. Although I cannot recall if this particular instance of not having virObject parent was addressed... Tks - John
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 89a7131fdb..1426f115ed 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -57,6 +57,7 @@ # include "virtypedparam.h" # include "virsavecookie.h" # include "virresctrl.h" +# include "virobject.h"
syntax-check fails ^here because the header is already included on line 49
With that fixed: Reviewed-by: Erik Skultety <eskultet@redhat.com>
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 04/12/2018 01:08 PM, John Ferlan wrote:
On 04/12/2018 04:44 AM, Erik Skultety wrote:
On Thu, Apr 12, 2018 at 09:14:50AM +0200, Michal Privoznik wrote:
In 2ada9ef1465f we've tried to turn virDomainChrSourceDef into virObject. Well, this requires 'virObject' member to be stored on the first position of the struct. This adjustment is missing in the original commit leading to all sorts of funny memleaks and data corruptions.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.h | 2 ++ 1 file changed, 2 insertions(+)
Doh! Thanks.... I guess not only do we need a way to detect that we're using VIR_ALLOC on an object (as Eric pointed out in my original patches), but we really should detect when we use virObjectNew without the virObject parent. For some reason, I have a recollection of altering changes during my meanderings through virObject code in order to point out more directly some misuses, but it was rejected. Although I cannot recall if this particular instance of not having virObject parent was addressed...
I don't think it was addressed there. And honestly, I don't know how to check for this in some automated way. Compiler is not going to help - sure we can have very primitive check like sizeof(virSomeStruct) >= sizeof(virObject), but that will fail only for very small structs (which is not the case here). Also, writing script that would check for this is going to end up in a lot of pain IMO. The only thing I can think of is review. Michal
On Thu, Apr 12, 2018 at 01:27:41PM +0200, Michal Privoznik wrote:
On 04/12/2018 01:08 PM, John Ferlan wrote:
On 04/12/2018 04:44 AM, Erik Skultety wrote:
On Thu, Apr 12, 2018 at 09:14:50AM +0200, Michal Privoznik wrote:
In 2ada9ef1465f we've tried to turn virDomainChrSourceDef into virObject. Well, this requires 'virObject' member to be stored on the first position of the struct. This adjustment is missing in the original commit leading to all sorts of funny memleaks and data corruptions.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.h | 2 ++ 1 file changed, 2 insertions(+)
Doh! Thanks.... I guess not only do we need a way to detect that we're using VIR_ALLOC on an object (as Eric pointed out in my original patches), but we really should detect when we use virObjectNew without the virObject parent. For some reason, I have a recollection of altering changes during my meanderings through virObject code in order to point out more directly some misuses, but it was rejected. Although I cannot recall if this particular instance of not having virObject parent was addressed...
I don't think it was addressed there. And honestly, I don't know how to check for this in some automated way. Compiler is not going to help - sure we can have very primitive check like sizeof(virSomeStruct) >= sizeof(virObject), but that will fail only for very small structs (which is not the case here). Also, writing script that would check for this is going to end up in a lot of pain IMO. The only thing I can think of is review.
I think we could do a compile time check for it with a little cpp black magic and some naming conventions. First we declare that the class name variable must always match the name of the object struct, but with Class suffix. Second we declare that the object struct must have a field called "parent" in it. Then we rename virObjectNew to virObjectNewImpl and add a macro: #define virObjectNew(typ) \ (typ *)(&((typ *)virObjectNewImpl(typ # Class)).parent) IOW we're casting the void * return value to the object struct, then we're referencing the parent field and casting it back to our object struct. We would have to change all calls to pass the struct name instead of class variable name. This doesn't ensure that "parent" is the first field, but with a little more thinking you can probably come up with a funkier macro to validate that aspect too. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Thu, Apr 12, 2018 at 09:14 AM +0200, Michal Privoznik <mprivozn@redhat.com> wrote:
In 2ada9ef1465f we've tried to turn virDomainChrSourceDef into virObject. Well, this requires 'virObject' member to be stored on the first position of the struct. This adjustment is missing in the original commit leading to all sorts of funny memleaks and data corruptions.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.h | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 89a7131fdb..1426f115ed 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -57,6 +57,7 @@ # include "virtypedparam.h" # include "virsavecookie.h" # include "virresctrl.h" +# include "virobject.h"
/* forward declarations of all device types, required by * virDomainDeviceDef @@ -1180,6 +1181,7 @@ typedef virDomainChrSourceReconnectDef *virDomainChrSourceReconnectDefPtr;
/* The host side information for a character device. */ struct _virDomainChrSourceDef { + virObject parent; int type; /* virDomainChrType */ virObjectPtr privateData; union { -- 2.16.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Oh… :/ With the fix suggested by Erik Reviewed-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> -- Beste Grüße / Kind regards Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
participants (5)
-
Daniel P. Berrangé -
Erik Skultety -
John Ferlan -
Marc Hartmayer -
Michal Privoznik