[libvirt] [PATCHv7 2/2] domain: conf: Fix secret type checking for network volumes

[Adam Walters]

On Thu, Jan 30, 2014 at 9:46 AM, Ján Tomko <jtomko at 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 at 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140130/d4dba795/attachment-0001.htm>