Re: [libvirt] [PATCH] storage: Fix incorrect format for XML

[Michal Privoznik]

On 06.10.2015 23:22, John Ferlan wrote:
> 
> 
> On 10/06/2015 09:19 AM, Michal Privoznik wrote:
>> On 23.09.2015 22:54, John Ferlan wrote:
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1256999
>>>
>>> When creating a copy of the 'authdef', need to take into account the
>>> slight variation between  and  before blindly copying
>>> the 'authType' field. This ensures virStorageAuthDefFormat will
>>> properly format the  XML for a .
>>>
>>> Signed-off-by: John Ferlan 
>>> ---
>>>  src/util/virstoragefile.c | 7 ++-
>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
>>> index 2aa1d90..0b72cb3 100644
>>> --- a/src/util/virstoragefile.c
>>> +++ b/src/util/virstoragefile.c
>>> @@ -1522,7 +1522,12 @@ virStorageAuthDefCopy(const virStorageAuthDef *src)
>>>  /* Not present for storage pool, but used for disk source */
>>>  if (VIR_STRDUP(ret->secrettype, src->secrettype) < 0)
>>>  goto error;
>>> -ret->authType = src->authType;
>>> +/* A  uses secrettype, while a  uses authType, so
>>> + * if we have a secrettype, then don't copy authType; otherwise,
>>> + * we will format the authType in 
>>> + */
>>> +if (!ret->secrettype)
>>> +ret->authType = src->authType;
>>>  ret->secretType = src->secretType;
>>
>> This seems like we have a bug somewhere if authType and secretType are
>> both set at the same time. Because the way I understand the code is that
>> only one from the pair can be non-null at the time. If so, I think we
>> should fix the bug and drop this if() here. 
> 
> The short answer is the bug was written long ago when disk and pool's
> were "allowed" to use a different formatted authdef XML and now we're
> stuck with it. The longer answer and details follow.

Oh, well. ACK then. If you want to propose any follow up patch, feel
free :-)

Michal

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

Re: [libvirt] [PATCH] storage: Fix incorrect format for XML

[John Ferlan]

On 10/06/2015 09:19 AM, Michal Privoznik wrote:
> On 23.09.2015 22:54, John Ferlan wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1256999
>>
>> When creating a copy of the 'authdef', need to take into account the
>> slight variation between  and  before blindly copying
>> the 'authType' field. This ensures virStorageAuthDefFormat will
>> properly format the  XML for a .
>>
>> Signed-off-by: John Ferlan 
>> ---
>>  src/util/virstoragefile.c | 7 ++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
>> index 2aa1d90..0b72cb3 100644
>> --- a/src/util/virstoragefile.c
>> +++ b/src/util/virstoragefile.c
>> @@ -1522,7 +1522,12 @@ virStorageAuthDefCopy(const virStorageAuthDef *src)
>>  /* Not present for storage pool, but used for disk source */
>>  if (VIR_STRDUP(ret->secrettype, src->secrettype) < 0)
>>  goto error;
>> -ret->authType = src->authType;
>> +/* A  uses secrettype, while a  uses authType, so
>> + * if we have a secrettype, then don't copy authType; otherwise,
>> + * we will format the authType in 
>> + */
>> +if (!ret->secrettype)
>> +ret->authType = src->authType;
>>  ret->secretType = src->secretType;
> 
> This seems like we have a bug somewhere if authType and secretType are
> both set at the same time. Because the way I understand the code is that
> only one from the pair can be non-null at the time. If so, I think we
> should fix the bug and drop this if() here. 

The short answer is the bug was written long ago when disk and pool's
were "allowed" to use a different formatted authdef XML and now we're
stuck with it. The longer answer and details follow.

First thing to understand "secretType" is a bit of a misnomer. It's
actually the type for the secret usage field. It's not the translation
for 'secrettype'. The secretType is used for both disk and pool XML. For
getting the "type" of auth/secret data a pool uses authType, but a disk
uses secrettype.

The authType is "supposed" to be only present in the pool definition;
however, when generating a "disk" definition from the source pool for a
direct iSCSI pool virStorageTranslateDiskSourcePoolAuth is used to
generate an 'authdef' structure in the disk def in order to be used to
create the command since we cannot rely upon the source pool for the
authentication. This causes the authType copy from source pool to the
disk def (which shouldn't be done), but virStorageAuthDefCopy doesn't
know if it's copying pool->pool, pool->disk, or disk->disk.

So, as an alternative, virStorageTranslateDiskSourcePoolAuth could set
"def->src->auth->authType = VIR_STORAGE_AUTH_TYPE_NONE;" since it's the
only place that knows we're copying from pool->disk. The 'secrettype'
field gets filled after returning from Translate if auth is set, so
that's where that magic happens. I could move the clear of authType
there as well, although I don't think that matters.

Alternatively, virStorageAuthDefFormat could be adjusted to not format
the authType if secrettype is set, but that's a different workaround.

Another (partially related to the bug) change that "could" happen is in
virDomainDiskDefFormat prior to calling virDomainDiskSourceFormat to add
a second condition to whether we print the auth data of "&&
!src->srcpool". That is don't print the authdef data from the source
pool. The authdef data only exists for a running domain using the
'direct'. Of course since we have been printing it already - one could
argue don't change that.

> Moreover, I'd expect a copy
> function to create the exact copy of the original image instead of
> masking a bug somewhere.

While I agree in principle about a copy function, sometimes that copy
function is better to have the logic than having to remember to "fix
things up" after the copy (or even know one has to do that). The Parse
and Format functions know the nuances, so it doesn't seem totally out of
place to have the Copy function have a bit of that knowledge too.

Rmemeber how the auth/secret data can look






or




or

and






The disk secret type is conceptually the same as the pool auth type.
There's also a secret mechanism involving  inside a 
which supports a "volume" format:






Fortunately this one isn't involved with the common parsing of the
 between the storage pool and domain xml parsing, but it is
accounted for as a disk secrettype.


> BTW isn't authType set to VIR_STORAGE_AUTH_TYPE_NONE by default?
> And secretType to VIR_STORAGE_SECRET_TYPE_NONE?
> 

The "authType" can be NONE, CHAP ("chap"), or CEPHX ("ceph"). It's used
by the iSCSI backend.

The "secretType" is not a conversion of "secrettype".  Rather it is
either NONE, UUID, or USAGE. That is it let's the code know the usage
model to lookup the secret via either UUID or USAGE.  I think it's
misnamed and could be "usageType"... Changing it would 

Re: [libvirt] [PATCH] storage: Fix incorrect format for XML

[Michal Privoznik]

On 23.09.2015 22:54, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1256999
> 
> When creating a copy of the 'authdef', need to take into account the
> slight variation between  and  before blindly copying
> the 'authType' field. This ensures virStorageAuthDefFormat will
> properly format the  XML for a .
> 
> Signed-off-by: John Ferlan 
> ---
>  src/util/virstoragefile.c | 7 ++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index 2aa1d90..0b72cb3 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -1522,7 +1522,12 @@ virStorageAuthDefCopy(const virStorageAuthDef *src)
>  /* Not present for storage pool, but used for disk source */
>  if (VIR_STRDUP(ret->secrettype, src->secrettype) < 0)
>  goto error;
> -ret->authType = src->authType;
> +/* A  uses secrettype, while a  uses authType, so
> + * if we have a secrettype, then don't copy authType; otherwise,
> + * we will format the authType in 
> + */
> +if (!ret->secrettype)
> +ret->authType = src->authType;
>  ret->secretType = src->secretType;

This seems like we have a bug somewhere if authType and secretType are
both set at the same time. Because the way I understand the code is that
only one from the pair can be non-null at the time. If so, I think we
should fix the bug and drop this if() here. Moreover, I'd expect a copy
function to create the exact copy of the original image instead of
masking a bug somewhere.
BTW isn't authType set to VIR_STORAGE_AUTH_TYPE_NONE by default?
And secretType to VIR_STORAGE_SECRET_TYPE_NONE?

Michal

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

[libvirt] [PATCH] storage: Fix incorrect format for XML

[John Ferlan]

https://bugzilla.redhat.com/show_bug.cgi?id=1256999

When creating a copy of the 'authdef', need to take into account the
slight variation between  and  before blindly copying
the 'authType' field. This ensures virStorageAuthDefFormat will
properly format the  XML for a .

Signed-off-by: John Ferlan 
---
 src/util/virstoragefile.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 2aa1d90..0b72cb3 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1522,7 +1522,12 @@ virStorageAuthDefCopy(const virStorageAuthDef *src)
 /* Not present for storage pool, but used for disk source */
 if (VIR_STRDUP(ret->secrettype, src->secrettype) < 0)
 goto error;
-ret->authType = src->authType;
+/* A  uses secrettype, while a  uses authType, so
+ * if we have a secrettype, then don't copy authType; otherwise,
+ * we will format the authType in 
+ */
+if (!ret->secrettype)
+ret->authType = src->authType;
 ret->secretType = src->secretType;
 if (ret->secretType == VIR_STORAGE_SECRET_TYPE_UUID) {
 memcpy(ret->secret.uuid, src->secret.uuid, sizeof(ret->secret.uuid));
-- 
2.1.0

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