On Thu, Jan 30, 2014 at 9:46 AM, Ján Tomko <jtomko@redhat.com> wrote:

On 01/23/2014 08:45 PM, Adam Walters wrote:
> This patch fixes the secret type checking done in the
> virDomainDiskDefParseXML function. Previously, it would not allow any
> volumes that utilized a secret. This patch is a simple bypass of the
> checking code for volumes.
>
> Signed-off-by: Adam Walters <adam@pandorasboxen.com>
> ---
> src/conf/domain_conf.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 28e24f9..773dc26 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -5418,7 +5418,8 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
> cur = cur->next;
> }
>
> - if (auth_secret_usage != -1 && auth_secret_usage != expected_secret_usage) {
> + if (auth_secret_usage != -1 && auth_secret_usage != expected_secret_usage &&
> + def->type != VIR_DOMAIN_DISK_TYPE_VOLUME) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("invalid secret type '%s'"),
> virSecretUsageTypeTypeToString(auth_secret_usage));

So an rbd volume can have a secret when the pool has auth set to none?
Otherwise it seems the volume's secret data might get overwritten by
qemuTranslateDiskSourcePoolAuth.

The purpose of this is to bypass the secret type checking for volumes (not just RBD volumes).

Above this section of code, but in the same function, there is some code that populates the

expected_secret_usage variable. Looking back on it now, I think I may have an alternative solution.

I think I might be able to set expected_secret_usage properly by referencing def->srcpool->pooltype

above this to check it like is done for non-storage pool RBD disks.

Without this particular patch hunk, any RBD volume used would throw an error in the logs:

"invalid secret type 'ceph'". If you didn't use a storage pool, but defined the RBD disk in the

domain XML directly with the secret instead, it worked just fine.

And this could also be added to qemuxml2argvtest.

I'll look into adding this into qemuxml2argvtest, as well.

> @@ -18214,7 +18215,8 @@ virDomainDiskDefForeachPath(virDomainDiskDefPtr disk,
> if (!disk->src || disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK ||
> (disk->type == VIR_DOMAIN_DISK_TYPE_VOLUME &&
> disk->srcpool &&
> - disk->srcpool->mode == VIR_DOMAIN_DISK_SOURCE_POOL_MODE_DIRECT))
> + (disk->srcpool->mode == VIR_DOMAIN_DISK_SOURCE_POOL_MODE_DIRECT ||
> + disk->srcpool->mode == VIR_DOMAIN_DISK_TYPE_NETWORK)))

What is the purpose of this? You are comparing the source pool mode against a
disk type constant. It seems this can never be true in this case.

That was a typo. The second line should be disk->srcpool->actualtype == VIR_DOMAIN_DISK_TYPE_NETWORK))).

I'll fix and submit a new version.

Jan