1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH 0/8] More virsecretobj changes to prepare for common object
  4. View patch

[libvirt] [PATCH 6/8] secret: Have virSecretObjNew consume newdef

John Ferlan posted 8 patches 7 years, 11 months ago
There is a newer version of this series
[libvirt] [PATCH 6/8] secret: Have virSecretObjNew consume newdef
Posted by John Ferlan 7 years, 11 months ago 
Move the consumption of @newdef into virSecretObjNew and then handle that
in the calling path.  Because on error the calling code expects to free
@newdef, unset obj->def for the creation failure error paths.

Signed-off-by: John Ferlan <jferlan@redhat.com>
---
 src/conf/virsecretobj.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
index c0bcfab..ca13cf8 100644
--- a/src/conf/virsecretobj.c
+++ b/src/conf/virsecretobj.c
@@ -87,7 +87,7 @@ virSecretObjOnceInit(void)
 VIR_ONCE_GLOBAL_INIT(virSecretObj)
 
 static virSecretObjPtr
-virSecretObjNew(void)
+virSecretObjNew(virSecretDefPtr def)
 {
     virSecretObjPtr obj;
 
@@ -98,6 +98,7 @@ virSecretObjNew(void)
         return NULL;
 
     virObjectLock(obj);
+    obj->def = def;
 
     return obj;
 }
@@ -384,20 +385,23 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
         goto error;
     }
 
-    if (!(obj = virSecretObjNew()))
+    if (!(obj = virSecretObjNew(newdef)))
         goto cleanup;
 
     /* Generate the possible configFile and base64File strings
      * using the configDir, uuidstr, and appropriate suffix
      */
     if (!(obj->configFile = virFileBuildPath(configDir, uuidstr, ".xml")) ||
-        !(obj->base64File = virFileBuildPath(configDir, uuidstr, ".base64")))
+        !(obj->base64File = virFileBuildPath(configDir, uuidstr, ".base64"))) {
+        obj->def = NULL;
         goto error;
+    }
 
-    if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0)
+    if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0) {
+        obj->def = NULL;
         goto error;
+    }
 
-    obj->def = newdef;
     virObjectRef(obj);
 
  cleanup:
-- 
2.9.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 6/8] secret: Have virSecretObjNew consume newdef
Posted by Michal Privoznik 7 years, 10 months ago 
On 06/03/2017 03:27 PM, John Ferlan wrote:
> Move the consumption of @newdef into virSecretObjNew and then handle that
> in the calling path.  Because on error the calling code expects to free
> @newdef, unset obj->def for the creation failure error paths.
> 
> Signed-off-by: John Ferlan <jferlan@redhat.com>
> ---
>  src/conf/virsecretobj.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
> index c0bcfab..ca13cf8 100644
> --- a/src/conf/virsecretobj.c
> +++ b/src/conf/virsecretobj.c
> @@ -87,7 +87,7 @@ virSecretObjOnceInit(void)
>  VIR_ONCE_GLOBAL_INIT(virSecretObj)
>  
>  static virSecretObjPtr
> -virSecretObjNew(void)
> +virSecretObjNew(virSecretDefPtr def)
>  {
>      virSecretObjPtr obj;
>  
> @@ -98,6 +98,7 @@ virSecretObjNew(void)
>          return NULL;
>  
>      virObjectLock(obj);
> +    obj->def = def;
>  
>      return obj;
>  }
> @@ -384,20 +385,23 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
>          goto error;
>      }
>  
> -    if (!(obj = virSecretObjNew()))
> +    if (!(obj = virSecretObjNew(newdef)))
>          goto cleanup;
>  
>      /* Generate the possible configFile and base64File strings
>       * using the configDir, uuidstr, and appropriate suffix
>       */
>      if (!(obj->configFile = virFileBuildPath(configDir, uuidstr, ".xml")) ||
> -        !(obj->base64File = virFileBuildPath(configDir, uuidstr, ".base64")))
> +        !(obj->base64File = virFileBuildPath(configDir, uuidstr, ".base64"))) {
> +        obj->def = NULL;
>          goto error;
> +    }

I don't quite see the value of this patch, esp. because you have to
manually unset the ->def in each error path.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 6/8] secret: Have virSecretObjNew consume newdef
Posted by John Ferlan 7 years, 10 months ago 

On 07/11/2017 11:52 AM, Michal Privoznik wrote:
> On 06/03/2017 03:27 PM, John Ferlan wrote:
>> Move the consumption of @newdef into virSecretObjNew and then handle that
>> in the calling path.  Because on error the calling code expects to free
>> @newdef, unset obj->def for the creation failure error paths.
>>
>> Signed-off-by: John Ferlan <jferlan@redhat.com>
>> ---
>>  src/conf/virsecretobj.c | 14 +++++++++-----
>>  1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
>> index c0bcfab..ca13cf8 100644
>> --- a/src/conf/virsecretobj.c
>> +++ b/src/conf/virsecretobj.c
>> @@ -87,7 +87,7 @@ virSecretObjOnceInit(void)
>>  VIR_ONCE_GLOBAL_INIT(virSecretObj)
>>  
>>  static virSecretObjPtr
>> -virSecretObjNew(void)
>> +virSecretObjNew(virSecretDefPtr def)
>>  {
>>      virSecretObjPtr obj;
>>  
>> @@ -98,6 +98,7 @@ virSecretObjNew(void)
>>          return NULL;
>>  
>>      virObjectLock(obj);
>> +    obj->def = def;
>>  
>>      return obj;
>>  }
>> @@ -384,20 +385,23 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
>>          goto error;
>>      }
>>  
>> -    if (!(obj = virSecretObjNew()))
>> +    if (!(obj = virSecretObjNew(newdef)))
>>          goto cleanup;
>>  
>>      /* Generate the possible configFile and base64File strings
>>       * using the configDir, uuidstr, and appropriate suffix
>>       */
>>      if (!(obj->configFile = virFileBuildPath(configDir, uuidstr, ".xml")) ||
>> -        !(obj->base64File = virFileBuildPath(configDir, uuidstr, ".base64")))
>> +        !(obj->base64File = virFileBuildPath(configDir, uuidstr, ".base64"))) {
>> +        obj->def = NULL;
>>          goto error;
>> +    }
> 
> I don't quite see the value of this patch, esp. because you have to
> manually unset the ->def in each error path.
> 
> Michal
> 

Well that's part of that "longer term" vision thing where I was having
the @def be consumed in a new object. I've had to scale that back a bit
due to comments related to the object, but this code was already was all
being done in parallel - so that's why it's like that.

I could drop this one, although having @def consumed by vir*ObjNew() is
something that I have been doing throughout the various changes.  So
far, virInterfaceObjNew already has this, but patches for nwfilter and
nodedev also follow the same pattern.

I'm 50/50 right now on it and can drop it if you'd prefer. Yes, the
drawback is "obvious" that on failure, clearing obj->def needs to be
done to avoid the potential double free problem.

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 6/8] secret: Have virSecretObjNew consume newdef
Posted by Michal Privoznik 7 years, 10 months ago 
On 07/13/2017 06:43 PM, John Ferlan wrote:
> 
> 
> On 07/11/2017 11:52 AM, Michal Privoznik wrote:
>> On 06/03/2017 03:27 PM, John Ferlan wrote:
>>> Move the consumption of @newdef into virSecretObjNew and then handle that
>>> in the calling path.  Because on error the calling code expects to free
>>> @newdef, unset obj->def for the creation failure error paths.
>>>
>>> Signed-off-by: John Ferlan <jferlan@redhat.com>
>>> ---
>>>  src/conf/virsecretobj.c | 14 +++++++++-----
>>>  1 file changed, 9 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
>>> index c0bcfab..ca13cf8 100644
>>> --- a/src/conf/virsecretobj.c
>>> +++ b/src/conf/virsecretobj.c
>>> @@ -87,7 +87,7 @@ virSecretObjOnceInit(void)
>>>  VIR_ONCE_GLOBAL_INIT(virSecretObj)
>>>  
>>>  static virSecretObjPtr
>>> -virSecretObjNew(void)
>>> +virSecretObjNew(virSecretDefPtr def)
>>>  {
>>>      virSecretObjPtr obj;
>>>  
>>> @@ -98,6 +98,7 @@ virSecretObjNew(void)
>>>          return NULL;
>>>  
>>>      virObjectLock(obj);
>>> +    obj->def = def;
>>>  
>>>      return obj;
>>>  }
>>> @@ -384,20 +385,23 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
>>>          goto error;
>>>      }
>>>  
>>> -    if (!(obj = virSecretObjNew()))
>>> +    if (!(obj = virSecretObjNew(newdef)))
>>>          goto cleanup;
>>>  
>>>      /* Generate the possible configFile and base64File strings
>>>       * using the configDir, uuidstr, and appropriate suffix
>>>       */
>>>      if (!(obj->configFile = virFileBuildPath(configDir, uuidstr, ".xml")) ||
>>> -        !(obj->base64File = virFileBuildPath(configDir, uuidstr, ".base64")))
>>> +        !(obj->base64File = virFileBuildPath(configDir, uuidstr, ".base64"))) {
>>> +        obj->def = NULL;
>>>          goto error;
>>> +    }
>>
>> I don't quite see the value of this patch, esp. because you have to
>> manually unset the ->def in each error path.
>>
>> Michal
>>
> 
> Well that's part of that "longer term" vision thing where I was having
> the @def be consumed in a new object. I've had to scale that back a bit
> due to comments related to the object, but this code was already was all
> being done in parallel - so that's why it's like that.
> 
> I could drop this one, although having @def consumed by vir*ObjNew() is
> something that I have been doing throughout the various changes.  So
> far, virInterfaceObjNew already has this, but patches for nwfilter and
> nodedev also follow the same pattern.

I know that you're doing it in other patches, but I don't think we need
to do that. It's not like we will make obj->def private. But maybe I'm
missing big picture here. What is your reasoning why should vir*ObjNew
take def? Moreover, other object members are initialized "old way" too
(e.g. obj->base64File). So mixing approaches might be confusing IMO.

> 
> I'm 50/50 right now on it and can drop it if you'd prefer. Yes, the
> drawback is "obvious" that on failure, clearing obj->def needs to be
> done to avoid the potential double free problem.

Yeah, I'd prefer it to be dropped, but then again - maybe I'm missing
big picture. So I'm not gonna tell you to do that.

Michal

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

Patchew 2.1-devel-8c64711

© 2016 - 2025 Red Hat, Inc.