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 <disk> and <pool> before blindly copying the 'authType' field. This ensures virStorageAuthDefFormat will properly format the <auth> XML for a <disk>.

Signed-off-by: John Ferlan <jferlan@redhat.com> --- 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 <disk> uses secrettype, while a <pool> uses authType, so + * if we have a secrettype, then don't copy authType; otherwise, + * we will format the authType in <disk> + */ + 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 <disk> <auth username='myuser'> <secret type='ceph' usage='mypassid'/> </auth> or <auth username='myuser'> <secret type='iscsi' usage='libvirtiscsi'/> </auth> or and <pool> <auth type='chap' username='myname'> <secret usage='mycluster_myname'/> </auth> The disk secret type is conceptually the same as the pool auth type. There's also a secret mechanism involving <encryption> inside a <disk> which supports a "volume" format: <disk> <encryption format='qcow'> <secret type='passphrase' uuid='0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f'/> </encryption> Fortunately this one isn't involved with the common parsing of the <auth> 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 mean changing virStorageSecretType to virStorageUsageType as well. Then there's the similarity to virSecretUsageType that'll certainly cause some confusion. I'm personally not a fan of massive renames. The "secrettype" can be NONE, VOLUME ("volume"), CEPH ("ceph"), or ISCSI ("iscsi"). So much for similarity to authType as it's not "chap" or "ceph" like it would be for a pool... Also "volume" is special - it's only used with encryption and is never formatted into secrettype. In any case, I'll submit a slightly different patch which clears authType in the translation routine. I can also generate one to not print authdef data when there's a srcpool if desired - that's another simple change. John