On 10/06/2015 09:19 AM, Michal Privoznik wrote: 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. 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. 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