Re: [libvirt] [PATCH 5/4] storage: Use virSecretGetSecretString

[John Ferlan <jferlan redhat com>]

On 05/31/2016 09:37 AM, Peter Krempa wrote:
> On Sat, May 28, 2016 at 09:55:12 -0400, John Ferlan wrote:
>> Rather than inline code secret lookup for rbd/iscsi, use the common function.
>>
>> Signed-off-by: John Ferlan <jferlan redhat com>
>> ---
>>  This just makes the iscsi/rbd storage driver use the common function...
>>  work that was started by pkrempa in commit id '1d632c39'
>>
>>
>>  src/Makefile.am                     |  1 +
>>  src/storage/storage_backend_iscsi.c | 48 +++++--------------------------------
>>  src/storage/storage_backend_rbd.c   | 45 +++-------------------------------
>>  3 files changed, 10 insertions(+), 84 deletions(-)
> 
> [...]
> 
>> diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c
>> index bccfba3..999b610 100644
>> --- a/src/storage/storage_backend_iscsi.c
>> +++ b/src/storage/storage_backend_iscsi.c
>> @@ -1,7 +1,7 @@
>>  /*
>>   * storage_backend_iscsi.c: storage backend for iSCSI handling
>>   *
>> - * Copyright (C) 2007-2014 Red Hat, Inc.
>> + * Copyright (C) 2007-2016 Red Hat, Inc.
> 
> I'm not a fan of this noise added by the editor.
> 

I don't have some editor macro. It's done by hand if I remember to look.
As long as I've been doing this (whether here or at some other company),
when I've updated a module in a new year, then as I understand it, one
is supposed to update the copyright. Whether that's a "hard" requirement
or required legally is above my pay grade ;-).

>>   * Copyright (C) 2007-2008 Daniel P. Berrange
>>   *
>>   * This library is free software; you can redistribute it and/or
> 
> [...]
> 
>> @@ -279,9 +279,9 @@ virStorageBackendISCSISetAuth(const char *portal,
>>  {
>>      virSecretPtr secret = NULL;
> 
> This shouldn't be necessary any more.
> 
>>      unsigned char *secret_value = NULL;
>> +    size_t secret_size;
>>      virStorageAuthDefPtr authdef = source->auth;
>>      int ret = -1;
>> -    char uuidStr[VIR_UUID_STRING_BUFLEN];
>>  
>>      if (!authdef || authdef->authType == VIR_STORAGE_AUTH_TYPE_NONE)
>>          return 0;
> 
> [...]
> 
>> @@ -359,7 +323,7 @@ virStorageBackendISCSISetAuth(const char *portal,
>>  
>>   cleanup:
>>      virObjectUnref(secret);
> 
> And this could be removed too.
> 
>> -    VIR_FREE(secret_value);
>> +    VIR_DISPOSE_N(secret_value, secret_size);
>>      return ret;
>>  }
>>  
>> diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c
>> index ac46b9d..34005ce 100644
>> --- a/src/storage/storage_backend_rbd.c
>> +++ b/src/storage/storage_backend_rbd.c
> 
> [...]
> 
>> @@ -63,7 +64,6 @@ virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr,
>>      char *rados_key = NULL;
>>      virBuffer mon_host = VIR_BUFFER_INITIALIZER;
>>      virSecretPtr secret = NULL;
> 
> You also don't need @secret any more.
> 
>> -    char secretUuid[VIR_UUID_STRING_BUFLEN];
>>      size_t i;
>>      char *mon_buff = NULL;
>>      const char *client_mount_timeout = "30";
> 
> ACK with the suggested changes after the release. This patch can be
> applied stand-alone.
> 

I adjusted the two instances...

I didn't expect any of this to make it prior to the release. As noted in
a cover letter or two - it's part of a much larger series that I'm able
to grab stuff out of to lessen future review pain and ensure that the
direction this is headed is OK...

John