Re: [libvirt] [PATCH] conf: Actually make virDomainChrSourceDef an object

[John Ferlan]

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 
>> ---
>>  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 
> 
> --
> 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

Re: [libvirt] [PATCH] conf: Actually make virDomainChrSourceDef an object

[Marc Hartmayer]

On Thu, Apr 12, 2018 at 09:14 AM +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 
> ---
>  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 

--
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


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] conf: Actually make virDomainChrSourceDef an object

[Daniel P . Berrangé]

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 
> >>> ---
> >>>  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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] conf: Actually make virDomainChrSourceDef an object

[Michal Privoznik]

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 
>>> ---
>>>  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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] conf: Actually make virDomainChrSourceDef an object

[Erik Skultety]

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 
> ---
>  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 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list