Re: [PATCH 06/14] secret: Use virSecretDefPtr rather than deref from virSecretObjPtr

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 04/25/2017 08:38 AM, Pavel Hrdina wrote:
> On Tue, Apr 25, 2017 at 07:58:58AM -0400, John Ferlan wrote:
>>
>>
>> On 04/25/2017 04:36 AM, Pavel Hrdina wrote:
>>> On Mon, Apr 24, 2017 at 02:00:15PM -0400, John Ferlan wrote:
>>>> Rather than dereferencing obj->def->X, create a local 'def' variable
>>>> variable that will dereference the def and use directly.
>>>>
>>>> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
>>>> ---
>>>>  src/conf/virsecretobj.c | 69 +++++++++++++++++++++++++++++++------------------
>>>>  1 file changed, 44 insertions(+), 25 deletions(-)
>>>>
>>>> diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
>>>> index 42f36c8..413955d 100644
>>>> --- a/src/conf/virsecretobj.c
>>>> +++ b/src/conf/virsecretobj.c
>>>> @@ -139,8 +139,9 @@ static void
>>>>  virSecretObjDispose(void *opaque)
>>>>  {
>>>>      virSecretObjPtr obj = opaque;
>>>> +    virSecretDefPtr def = obj->def;
>>>>  
>>>> -    virSecretDefFree(obj->def);
>>>> +    virSecretDefFree(def);
>>>
>>> Here it adds only a noise into the code, no actual improvement.
>>>
>>
>> Well.... not entirely.  Later on in patches that aren't posted yet the
>> "obj->def" is going to be replaced by virObjectPoolableDefGetDef() and
>> while yes, one could make that call inside the virSecretDefFree, it's
>> also just as simple to make it outside that call.  It's a rote exercise.
>> Besides I find it "ugly" to read functionA(functionB()). Even worse if
>> functionB now could return NULL or some value one then has to split
>> apart the code anyway.
> 
> Like I replied to your replay, these change itself doesn't improve anything
> and there is no benefit from these changes further in the series.  If you
> have some patches prepared in your branch that will benefit these changes
> it should be part of that series, not this one.
> 

You want to see a 90 patch series?  That includes all secret, nwfilter,
and interface changes plus a bunch of object changes? No one's even
commented on the object RFC I posted, but I need to make some progress;
otherwise, I end up with way too many patches to manage in my tree.

>>
>>>>      if (obj->value) {
>>>>          /* Wipe before free to ensure we don't leave a secret on the heap */
>>>>          memset(obj->value, 0, obj->value_size);
>>>> @@ -203,16 +204,18 @@ virSecretObjSearchName(const void *payload,
>>>>                         const void *opaque)
>>>>  {
>>>>      virSecretObjPtr obj = (virSecretObjPtr) payload;
>>>> +    virSecretDefPtr def;
>>>>      struct virSecretSearchData *data = (struct virSecretSearchData *) opaque;
>>>>      int found = 0;
>>>>  
>>>>      virObjectLock(obj);
>>>> +    def = obj->def;
>>>>  
>>>> -    if (obj->def->usage_type != data->usageType)
>>>> +    if (def->usage_type != data->usageType)
>>>>          goto cleanup;
>>>>  
>>>>      if (data->usageType != VIR_SECRET_USAGE_TYPE_NONE &&
>>>> -        STREQ(obj->def->usage_id, data->usageID))
>>>> +        STREQ(def->usage_id, data->usageID))
>>>>          found = 1;
>>>>  
>>>>   cleanup:
>>>> @@ -278,11 +281,13 @@ virSecretObjListRemove(virSecretObjListPtr secrets,
>>>>                         virSecretObjPtr obj)
>>>>  {
>>>>      char uuidstr[VIR_UUID_STRING_BUFLEN];
>>>> +    virSecretDefPtr def;
>>>>  
>>>>      if (!obj)
>>>>          return;
>>>> +    def = obj->def;
>>>>  
>>>> -    virUUIDFormat(obj->def->uuid, uuidstr);
>>>> +    virUUIDFormat(def->uuid, uuidstr);
>>>
>>> Same here,
>>>
>>
>> No this is "obj->def->uuid" into "def->uuid"...  Once "->def" becomes
>> part of an "obj" it won't be visible and that would require more change
>> later on to essentially perform the same action.
> 
> Once, but it's not a part of this series so without that patch it
> doesn't add any benefit to the current code.  This probably applies to
> the remaining places.
> 
> And getting def->uuid instead of obj->def->uuid doesn't seem like an
> improvement, it's used only once in this function.  If there were more
> usages of def it would make sense to do this change.
> 

Again, it's all about frame of reference.  If this patch is unacceptable
then I may as well just stop doing any of this work.  Really - there's
so much built up upon the separation of 'def' into it's own variable
that it's untenable to continue.

I think the question should be - is what I did technically wrong and not
is what I did seem unnecessary? Would the compiler actually do something
different (probably not). I personally find the code more readable to
have the separate defs. Are the "just" obj->def deref's absolutely
necessary - probably not for now and I could change those back. But for
any others where its obj->def->X, I feel the local def is a better
answer *regardless* of whether it's only used once.

I have an end goal in mind, but I can pretty much guarantee if I posted
all 90+ patches I have it'd probably mean the patches would languish on
the list because no one wants to review 90+ patches.

I contemplated splitting these up into 5-8 patch series, but that felt
way too tedious. So the two 15 patch series felt like a happy medium.

I do greatly appreciate you jumping right in right away and providing
reviews! While I may not agree exactly - I can only hope if you
understand the frame of reference it'll help understand why I think this
should be done now.

John

> Pavel
> 
>>>>      virObjectRef(obj);
>>>>      virObjectUnlock(obj);
>>>>  
>>>> @@ -315,6 +320,7 @@ virSecretObjListAddLocked(virSecretObjListPtr secrets,
>>>>                            virSecretDefPtr *oldDef)
>>>>  {
>>>>      virSecretObjPtr obj;
>>>> +    virSecretDefPtr def;
>>>>      virSecretObjPtr ret = NULL;
>>>>      char uuidstr[VIR_UUID_STRING_BUFLEN];
>>>>      char *configFile = NULL, *base64File = NULL;
>>>> @@ -325,26 +331,27 @@ virSecretObjListAddLocked(virSecretObjListPtr secrets,
>>>>      /* Is there a secret already matching this UUID */
>>>>      if ((obj = virSecretObjListFindByUUIDLocked(secrets, newdef->uuid))) {
>>>>          virObjectLock(obj);
>>>> +        def = obj->def;
>>>>  
>>>> -        if (STRNEQ_NULLABLE(obj->def->usage_id, newdef->usage_id)) {
>>>> -            virUUIDFormat(obj->def->uuid, uuidstr);
>>>> +        if (STRNEQ_NULLABLE(def->usage_id, newdef->usage_id)) {
>>>> +            virUUIDFormat(def->uuid, uuidstr);
>>>>              virReportError(VIR_ERR_INTERNAL_ERROR,
>>>>                             _("a secret with UUID %s is already defined for "
>>>>                               "use with %s"),
>>>> -                           uuidstr, obj->def->usage_id);
>>>> +                           uuidstr, def->usage_id);
>>>>              goto cleanup;
>>>>          }
>>>>  
>>>> -        if (obj->def->isprivate && !newdef->isprivate) {
>>>> +        if (def->isprivate && !newdef->isprivate) {
>>>>              virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>>>>                             _("cannot change private flag on existing secret"));
>>>>              goto cleanup;
>>>>          }
>>>>  
>>>>          if (oldDef)
>>>> -            *oldDef = obj->def;
>>>> +            *oldDef = def;
>>>>          else
>>>> -            virSecretDefFree(obj->def);
>>>> +            virSecretDefFree(def);
>>>>          obj->def = newdef;
>>>>      } else {
>>>>          /* No existing secret with same UUID,
>>>> @@ -353,7 +360,8 @@ virSecretObjListAddLocked(virSecretObjListPtr secrets,
>>>>                                                       newdef->usage_type,
>>>>                                                       newdef->usage_id))) {
>>>>              virObjectLock(obj);
>>>> -            virUUIDFormat(obj->def->uuid, uuidstr);
>>>> +            def = obj->def;
>>>> +            virUUIDFormat(def->uuid, uuidstr);
>>>>              virReportError(VIR_ERR_INTERNAL_ERROR,
>>>>                             _("a secret with UUID %s already defined for "
>>>>                               "use with %s"),
>>>> @@ -424,6 +432,7 @@ virSecretObjListGetHelper(void *payload,
>>>>  {
>>>>      struct virSecretObjListGetHelperData *data = opaque;
>>>>      virSecretObjPtr obj = payload;
>>>> +    virSecretDefPtr def;
>>>>  
>>>>      if (data->error)
>>>>          return 0;
>>>> @@ -432,8 +441,9 @@ virSecretObjListGetHelper(void *payload,
>>>>          return 0;
>>>>  
>>>>      virObjectLock(obj);
>>>> +    def = obj->def;
>>>>  
>>>> -    if (data->filter && !data->filter(data->conn, obj->def))
>>>> +    if (data->filter && !data->filter(data->conn, def))
>>>>          goto cleanup;
>>>>  
>>>>      if (data->uuids) {
>>>> @@ -444,7 +454,7 @@ virSecretObjListGetHelper(void *payload,
>>>>              goto cleanup;
>>>>          }
>>>>  
>>>> -        virUUIDFormat(obj->def->uuid, uuidstr);
>>>> +        virUUIDFormat(def->uuid, uuidstr);
>>>>          data->uuids[data->got] = uuidstr;
>>>>      }
>>>>  
>>>> @@ -478,20 +488,22 @@ static bool
>>>>  virSecretObjMatchFlags(virSecretObjPtr obj,
>>>>                         unsigned int flags)
>>>>  {
>>>> +    virSecretDefPtr def = obj->def;
>>>> +
>>>>      /* filter by whether it's ephemeral */
>>>>      if (MATCH(VIR_CONNECT_LIST_SECRETS_FILTERS_EPHEMERAL) &&
>>>>          !((MATCH(VIR_CONNECT_LIST_SECRETS_EPHEMERAL) &&
>>>> -           obj->def->isephemeral) ||
>>>> +           def->isephemeral) ||
>>>>            (MATCH(VIR_CONNECT_LIST_SECRETS_NO_EPHEMERAL) &&
>>>> -           !obj->def->isephemeral)))
>>>> +           !def->isephemeral)))
>>>>          return false;
>>>>  
>>>>      /* filter by whether it's private */
>>>>      if (MATCH(VIR_CONNECT_LIST_SECRETS_FILTERS_PRIVATE) &&
>>>>          !((MATCH(VIR_CONNECT_LIST_SECRETS_PRIVATE) &&
>>>> -           obj->def->isprivate) ||
>>>> +           def->isprivate) ||
>>>>            (MATCH(VIR_CONNECT_LIST_SECRETS_NO_PRIVATE) &&
>>>> -           !obj->def->isprivate)))
>>>> +           !def->isprivate)))
>>>>          return false;
>>>>  
>>>>      return true;
>>>> @@ -515,14 +527,16 @@ virSecretObjListPopulate(void *payload,
>>>>  {
>>>>      struct virSecretObjListData *data = opaque;
>>>>      virSecretObjPtr obj = payload;
>>>> +    virSecretDefPtr def;
>>>>      virSecretPtr secret = NULL;
>>>>  
>>>>      if (data->error)
>>>>          return 0;
>>>>  
>>>>      virObjectLock(obj);
>>>> +    def = obj->def;
>>>>  
>>>> -    if (data->filter && !data->filter(data->conn, obj->def))
>>>> +    if (data->filter && !data->filter(data->conn, def))
>>>>          goto cleanup;
>>>>  
>>>>      if (!virSecretObjMatchFlags(obj, data->flags))
>>>> @@ -533,9 +547,9 @@ virSecretObjListPopulate(void *payload,
>>>>          goto cleanup;
>>>>      }
>>>>  
>>>> -    if (!(secret = virGetSecret(data->conn, obj->def->uuid,
>>>> -                                obj->def->usage_type,
>>>> -                                obj->def->usage_id))) {
>>>> +    if (!(secret = virGetSecret(data->conn, def->uuid,
>>>> +                                def->usage_type,
>>>> +                                def->usage_id))) {
>>>>          data->error = true;
>>>>          goto cleanup;
>>>>      }
>>>> @@ -624,7 +638,9 @@ virSecretObjListGetUUIDs(virSecretObjListPtr secrets,
>>>>  int
>>>>  virSecretObjDeleteConfig(virSecretObjPtr obj)
>>>>  {
>>>> -    if (!obj->def->isephemeral &&
>>>> +    virSecretDefPtr def = obj->def;
>>>> +
>>>> +    if (!def->isephemeral &&
>>>
>>> here,
>>>
>>
>> Some concept - obj->def->isphemeral becomes just def->isephemeral
>>
>>>>          unlink(obj->configFile) < 0 && errno != ENOENT) {
>>>>          virReportSystemError(errno, _("cannot unlink '%s'"),
>>>>                               obj->configFile);
>>>> @@ -653,10 +669,11 @@ virSecretObjDeleteData(virSecretObjPtr obj)
>>>>  int
>>>>  virSecretObjSaveConfig(virSecretObjPtr obj)
>>>>  {
>>>> +    virSecretDefPtr def = obj->def;
>>>>      char *xml = NULL;
>>>>      int ret = -1;
>>>>  
>>>> -    if (!(xml = virSecretDefFormat(obj->def)))
>>>> +    if (!(xml = virSecretDefFormat(def)))
>>>>          goto cleanup;
>>>
>>> here,
>>>
>>
>> Sure it could eventually be a call instead, similar comment to above
>> though w/r/t function call within a function call for readability
>>
>>>>      if (virFileRewriteStr(obj->configFile, S_IRUSR | S_IWUSR, xml) < 0)
>>>> @@ -711,11 +728,12 @@ virSecretObjSetDef(virSecretObjPtr obj,
>>>>  unsigned char *
>>>>  virSecretObjGetValue(virSecretObjPtr obj)
>>>>  {
>>>> +    virSecretDefPtr def = obj->def;
>>>>      unsigned char *ret = NULL;
>>>>  
>>>>      if (!obj->value) {
>>>>          char uuidstr[VIR_UUID_STRING_BUFLEN];
>>>> -        virUUIDFormat(obj->def->uuid, uuidstr);
>>>> +        virUUIDFormat(def->uuid, uuidstr);
>>>
>>> here,
>>
>> obj->def->uuid is now just def->uuid
>>
>>>>          virReportError(VIR_ERR_NO_SECRET,
>>>>                         _("secret '%s' does not have a value"), uuidstr);
>>>>          goto cleanup;
>>>> @@ -735,6 +753,7 @@ virSecretObjSetValue(virSecretObjPtr obj,
>>>>                       const unsigned char *value,
>>>>                       size_t value_size)
>>>>  {
>>>> +    virSecretDefPtr def = obj->def;
>>>>      unsigned char *old_value, *new_value;
>>>>      size_t old_value_size;
>>>>  
>>>> @@ -748,7 +767,7 @@ virSecretObjSetValue(virSecretObjPtr obj,
>>>>      obj->value = new_value;
>>>>      obj->value_size = value_size;
>>>>  
>>>> -    if (!obj->def->isephemeral && virSecretObjSaveData(obj) < 0)
>>>> +    if (!def->isephemeral && virSecretObjSaveData(obj) < 0)
>>>>          goto error;
>>>
>>> and here.
>>>
>>
>> similar to previous isephemeral.
>>
>>
>> John
>>
>>> Pavel
>>>
>>
>> --
>> libvir-list mailing list
>> libvir-list@xxxxxxxxxx
>> https://www.redhat.com/mailman/listinfo/libvir-list

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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux