Re: [libvirt] [PATCH v2 01/14] qemu: Introduce qemuDomainSecretInfoNew

[John Ferlan]

On 02/24/2017 09:00 AM, Jiri Denemark wrote:
> On Thu, Feb 23, 2017 at 13:42:03 -0500, John Ferlan wrote:
>> Create a helper which will create the secinfo used for disks, hostdevs,
>> and chardevs.
>>
>> Signed-off-by: John Ferlan 
>> ---
>>  src/qemu/qemu_domain.c | 140 
>> ++---
>>  1 file changed, 74 insertions(+), 66 deletions(-)
>>
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index c187214..b7594b3 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -1112,6 +1112,55 @@ qemuDomainSecretSetup(virConnectPtr conn,
>>  }
>>  
>>  
>> +/* qemuDomainSecretInfoNew:
>> + * @conn: Pointer to connection
>> + * @priv: pointer to domain private object
>> + * @srcAlias: Alias base to use for TLS object
>> + * @lookupType: Type of secret lookup
>> + * @username: username for plain secrets
>> + * @looupdef: lookup def describing secret
>> + * @isLuks: boolean for luks lookup
>> + * @encFmt: string for error message
>> + *
>> + * Helper function to create a secinfo to be used for secinfo consumers
>> + *
>> + * Returns @secinfo on success, NULL on failure. Caller is responsible
>> + * to eventually free @secinfo.
>> + */
>> +static qemuDomainSecretInfoPtr
>> +qemuDomainSecretInfoNew(virConnectPtr conn,
>> +qemuDomainObjPrivatePtr priv,
>> +const char *srcAlias,
>> +virSecretLookupType lookupType,
> 
> This parameter should rather be
> 

Weird I wonder what I was cut-n-paste'ing.

>virSecretUsageType usageType
> 
>> +const char *username,
>> +virSecretLookupTypeDefPtr lookupDef,
>> +bool isLuks,
>> +const char *encFmt)
>> +{
>> +qemuDomainSecretInfoPtr secinfo = NULL;
>> +
>> +if (VIR_ALLOC(secinfo) < 0)
>> +return NULL;
>> +
>> +if (qemuDomainSecretSetup(conn, priv, secinfo, srcAlias, lookupType,
>> +  username, lookupDef, isLuks) < 0)
>> +goto error;
>> +
>> +if (encFmt && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_PLAIN) {
>> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> +   _("%s requires encrypted secrets to be supported"),
>> +   encFmt);
> 
> I didn't really get the "encFmt" name, but it's just a minor issue
> compared to the way the error message is composed here. This results in
> an untranslatable string. I think returning a generic error about
> unsupported encrypted secrets would be good enough.
> 
> Jirka
> 

I know this kind of thing done elsewhere, but I can remove it. The hope
was to have some sort of message to help indicate which failed, but it's
not that important.

John

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

Re: [libvirt] [PATCH v2 01/14] qemu: Introduce qemuDomainSecretInfoNew

[Jiri Denemark]

On Thu, Feb 23, 2017 at 13:42:03 -0500, John Ferlan wrote:
> Create a helper which will create the secinfo used for disks, hostdevs,
> and chardevs.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/qemu/qemu_domain.c | 140 
> ++---
>  1 file changed, 74 insertions(+), 66 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index c187214..b7594b3 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -1112,6 +1112,55 @@ qemuDomainSecretSetup(virConnectPtr conn,
>  }
>  
>  
> +/* qemuDomainSecretInfoNew:
> + * @conn: Pointer to connection
> + * @priv: pointer to domain private object
> + * @srcAlias: Alias base to use for TLS object
> + * @lookupType: Type of secret lookup
> + * @username: username for plain secrets
> + * @looupdef: lookup def describing secret
> + * @isLuks: boolean for luks lookup
> + * @encFmt: string for error message
> + *
> + * Helper function to create a secinfo to be used for secinfo consumers
> + *
> + * Returns @secinfo on success, NULL on failure. Caller is responsible
> + * to eventually free @secinfo.
> + */
> +static qemuDomainSecretInfoPtr
> +qemuDomainSecretInfoNew(virConnectPtr conn,
> +qemuDomainObjPrivatePtr priv,
> +const char *srcAlias,
> +virSecretLookupType lookupType,

This parameter should rather be

   virSecretUsageType usageType

> +const char *username,
> +virSecretLookupTypeDefPtr lookupDef,
> +bool isLuks,
> +const char *encFmt)
> +{
> +qemuDomainSecretInfoPtr secinfo = NULL;
> +
> +if (VIR_ALLOC(secinfo) < 0)
> +return NULL;
> +
> +if (qemuDomainSecretSetup(conn, priv, secinfo, srcAlias, lookupType,
> +  username, lookupDef, isLuks) < 0)
> +goto error;
> +
> +if (encFmt && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_PLAIN) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("%s requires encrypted secrets to be supported"),
> +   encFmt);

I didn't really get the "encFmt" name, but it's just a minor issue
compared to the way the error message is composed here. This results in
an untranslatable string. I think returning a generic error about
unsupported encrypted secrets would be good enough.

Jirka

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