[libvirt] [PATCH 3/6] Report secret usage error message similarly

[Ján Tomko]

On 08/19/2013 11:09 PM, John Ferlan wrote:
> On 08/16/2013 12:34 PM, Ján Tomko wrote:
>> On 08/08/2013 02:43 AM, John Ferlan wrote:
>>>                                                 VIR_SECRET_GET_VALUE_INTERNAL_CALL);
>>>          if (!secret_value) {
>>> -            virReportError(VIR_ERR_INTERNAL_ERROR,
>>> -                           _("could not get the value of the secret "
>>> -                             "for username %s"), chap.username);
>>> +            if (chap.secret.uuidUsable) {
>>> +                virReportError(VIR_ERR_INTERNAL_ERROR,
>>> +                               _("could not get the value of the secret "
>>> +                                 "for username %s using uuid '%s'"),
>>> +                                 chap.username, chap.secret.uuid);
>>> +            } else {
>>> +                virReportError(VIR_ERR_INTERNAL_ERROR,
>>> +                               _("could not get the value of the secret "
>>> +                                 "for username %s using usage value '%s'"),
>>> +                                 chap.username, chap.secret.usage);
>>
>> This looks to me like the username plays a part in getting the secret value,
>> but I'm not sure how to rephrase it while still keeping the username there.
>>
> 
> This error is a failure to find the secret value (e.g. call to
> 'secretGetValue' to the secret driver) given the "secret key"
> (virSecretPtr) which was obtained by using either the UUID or Usage value.
> 
> The username is mainly for convenience in the error message since it's
> not used by the secret driver. The username would be passed to whatever
> the code is authenticating to along with the secret value pulled from
> the secret driver.
> 
> I suppose username could be removed from the message, but it just seemed
> easier to reference the username as well since it's part of the xml that
> would use the secret.
> 

Usually we don't print the surrounding elements. Maybe the username was a part
of the error message to avoid separate errors for uuid and usage? But I'm fine
either way, the only condition for my ACK was that you remove the unrelated
whitespace change.

>>> +            }
>>>              goto cleanup;
>>>          }
>>>      } else {
>>> -        virReportError(VIR_ERR_INTERNAL_ERROR,
>>> -                       _("username '%s' specified but secret not found"),
>>> -                       chap.username);
>>> +        if (chap.secret.uuidUsable) {
>>> +            virReportError(VIR_ERR_NO_SECRET,
>>> +                           _("username '%s' specified but the secret for "
>>> +                             "uuid '%s' not found"),
>>> +                           chap.username, chap.secret.uuid);
>>> +        } else {
>>> +            virReportError(VIR_ERR_NO_SECRET,
>>> +                           _("username '%s' specified but the secret for "
>>> +                             "usage value '%s' not found"),
>>> +                           chap.username, chap.secret.usage);
>>> +        }
>>>          goto cleanup;
>>
>> With VIR_ERR_NO_SECRET, this says "secret not found" twice:
>> error: Secret not found: username 'nahasapeemapetilon' specified but the
>> secret for usage value 'pwagmattasquarmsettport' not found
>>
>> But that's minor. Maybe "... but no secret matches usage '%s'"?
>>
> 
> This error message is the inability to find the "secret key"
> (virSecretPtr) given either a UUID or Usage value (the output of that
> virsh secret-list).
> 
> Is the following more appropriate?
> 
> 
> change
> 
> _("%s username '%s' specified but secret for "
>  "uuid '%s' not found")
> 
> to be:
> 
> _("no secret matches uuid '%s'")
> 
> and
> 
> _("%s username '%s' specified but secret for "
>   "usage value '%s' not found"),
> 
> to be:
> 
> _("no secret matches usage value '%s'")
> 
> 

Personally, I like the shorter versions more. But all three versions are fine.

> 
>>>      }
>>>  
>>> diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c
>>> index e3340f6..9c6d179 100644
>>> --- a/src/storage/storage_backend_rbd.c
>>> +++ b/src/storage/storage_backend_rbd.c
>> ...
>>>  
>>> -        secret_value = conn->secretDriver->secretGetValue(secret, &secret_value_size, 0,
>>> +        secret_value = conn->secretDriver->secretGetValue(secret,
>>> +                                                          &secret_value_size, 0,
>>>                                                            VIR_SECRET_GET_VALUE_INTERNAL_CALL);
>>>  
> 
> I removed this since it's unrelated - that's just my desire to have 80
> column displays.

Feel free to push it separately under the trivial rule, but it doesn't really
remove any lines longer than 80 columns :)


Jan