[libvirt] [PATCH 01/15] util: Introduce VIR_DEFINE_AUTOPTR_FUNC for virStorageAuthDef

John Ferlan jferlan at redhat.com
Thu Feb 7 12:12:08 UTC 2019



On 2/7/19 4:10 AM, Erik Skultety wrote:
> On Wed, Feb 06, 2019 at 08:41:33AM -0500, John Ferlan wrote:
>> Let's make use of the auto __cleanup capabilities cleaning up any
>> now unnecessary goto paths. For virStorageAuthDefCopy use authdef
>> and ret as consistently as similar other code.
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>>  src/conf/domain_conf.c        | 29 +++++++++++------------------
>>  src/conf/storage_conf.c       |  6 ++----
>>  src/qemu/qemu_parse_command.c |  6 ++----
>>  src/util/virstoragefile.c     | 33 ++++++++++++++-------------------
>>  src/util/virstoragefile.h     |  1 +
>>  5 files changed, 30 insertions(+), 45 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 1fc4c8a35a..5699a60549 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -7578,10 +7578,9 @@ virDomainHostdevSubsysSCSIiSCSIDefParseXML(xmlNodePtr sourcenode,
>>                                             virDomainHostdevSubsysSCSIPtr def,
>>                                             xmlXPathContextPtr ctxt)
>>  {
>> -    int ret = -1;
>>      int auth_secret_usage = -1;
>>      xmlNodePtr cur;
>> -    virStorageAuthDefPtr authdef = NULL;
>> +    VIR_AUTOPTR(virStorageAuthDef) authdef = NULL;
> 
> For better readability I prefer declaring VIR_AUTO variables at the end of the
> declare block (multiple occurrences throughout the patch)...
> 
> ...
> 

I don't mind moving them. I generally just try to keep the usages together.


>> +            VIR_STEAL_PTR(iscsisrc->src->auth, authdef);
> 
> ^Unrelated change, there should be a trivial patch preceding this one taking
> care of the VIR_STEAL_PTR replacements.
> 
> ...

You'll find this sprinkled throughout - generating separate patches
could explode the series into perhaps 30+ patches which I doubt anyone
would jump at the chance to review.  I can separate before pushing and I
assume by extracting it/them I can just add your R-by too...

> 
>>
>> diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
>> index a98d5103fa..f1164dde9b 100644
>> --- a/src/util/virstoragefile.h
>> +++ b/src/util/virstoragefile.h
>> @@ -543,4 +543,5 @@ void virStorageFileReportBrokenChain(int errcode,
>>                                       virStorageSourcePtr src,
>>                                       virStorageSourcePtr parent);
>>
>> +VIR_DEFINE_AUTOPTR_FUNC(virStorageAuthDef, virStorageAuthDefFree);
> 
> ^This defines a static function, so the ';' is unnecessary, it doesn't hurt,
> but you know, consistency ;). Also, keep the original newline below.
> 

Perhaps because I had recently reviewed Cole's series about removing the
semicolon from VIR_ENUM_DECL, VIR_ENUM_IMPL, VIR_LOG_INIT, and
VIR_ONCE_GLOBAL_INIT - it was just "fresh" in my mind that macros should
have semicolons (moreso than actually looking at that definition).  I
can change them all.

Tks -

John

> Reviewed-by: Erik Skultety <eskultet at redhat.com>
> 




More information about the libvir-list mailing list