[libvirt] [PATCH 2/2] bhyve: domainCreateXML

[Wojciech Macek]

Sure, I will split it into two separate commits. I must have messed up sth
in commits, since even in head-email I described 3 patches...

Regarding NULL, it is a little tricky. After running
virDomainObjListRemove, the vm pointer has 0 references. Then, running
virObjectUnlock causes libvirt segfault. To minimize changes, I used the
solution from qemu: set vm to null and then check it inside cleanup. The
"if (vm)" is just aesthetic - without it there was an awful warning on the
console about null-pointer. I'd like to leave it as is, or remove
"vm=NULL;" and insert virObjectRef/virObjectUnref pair instead.

What do you mean by ACL check? Do you think the check is unnecessary, or
just should be placed earlier?


W.



2014-04-08 9:01 GMT+02:00 Roman Bogorodskiy <bogorodskiy at gmail.com>:

>   Wojciech Macek wrote:
>
> > Implement bhyveDomainCreteXML function.
>
> s/Crete/Create/
>
> >  src/bhyve/bhyve_driver.c | 81
> +++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 80 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c
> > index f7a8912..2ea2d22 100644
> > --- a/src/bhyve/bhyve_driver.c
> > +++ b/src/bhyve/bhyve_driver.c
> > @@ -756,8 +756,14 @@ bhyveDomainDestroy(virDomainPtr dom)
> >
> >      ret = virBhyveProcessStop(privconn, vm,
> VIR_DOMAIN_SHUTOFF_DESTROYED);
> >
> > +    if (!vm->persistent) {
> > +        virDomainObjListRemove(privconn->domains, vm);
> > +        vm = NULL;
> > +    }
> > +
>
> This chunk doesn't seem to be directly related to domainCreateXML(),
> should there be a reasoning and probably it should be a separate commit
> even?
>
> >   cleanup:
> > -    virObjectUnlock(vm);
> > +    if (vm)
> > +        virObjectUnlock(vm);
>
> It's safe not to check for NULL here.
>
> >      return ret;
> >  }
> >
> > @@ -1074,6 +1080,78 @@ bhyveDomainResume(virDomainPtr dom)
> >      return ret;
> >  }
> >
> > +static virDomainPtr
> > +bhyveDomainCreateXML(virConnectPtr conn,
> > +                     const char *xml,
> > +                     unsigned int flags)
> > +{
> > +    bhyveConnPtr privconn = conn->privateData;
> > +    virDomainPtr dom = NULL;
> > +    virDomainDefPtr def = NULL;
> > +    virDomainObjPtr vm = NULL;
> > +    virCapsPtr caps = NULL;
> > +    int ret;
> > +    unsigned int start_flags = 0;
> > +
> > +    virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, NULL);
> > +
> > +    if (flags & VIR_DOMAIN_START_AUTODESTROY)
> > +        start_flags |= VIR_BHYVE_PROCESS_START_AUTODESTROY;
> > +
> > +    caps = bhyveDriverGetCapabilities(privconn);
> > +    if (!caps)
> > +        return NULL;
> > +
> > +    if ((def = virDomainDefParseString(xml, caps, privconn->xmlopt,
> > +                                       1 << VIR_DOMAIN_VIRT_BHYVE,
> > +                                       VIR_DOMAIN_XML_INACTIVE)) ==
> NULL)
> > +        goto cleanup;
> > +
> > +    if (virDomainCreateXMLEnsureACL(conn, def) < 0)
> > +        goto cleanup;
> > +
> > +    if (!(vm = virDomainObjListAdd(privconn->domains, def,
> > +                                   privconn->xmlopt,
> > +                                   0, NULL)))
> > +        goto cleanup;
> > +    def = NULL;
> > +    vm->persistent = 0;
> > +
> > +    dom = virGetDomain(conn, vm->def->name, vm->def->uuid);
> > +    if (!dom)
> > +        goto cleanup;
> > +
> > +    dom->id = vm->def->id;
> > +
> > +    if (flags & VIR_DOMAIN_START_AUTODESTROY)
> > +        start_flags |= VIR_BHYVE_PROCESS_START_AUTODESTROY;
> > +
> > +    if (virDomainCreateWithFlagsEnsureACL(dom->conn, vm->def) < 0)
> > +            goto cleanup;
>
> Do we really need this ACL check here?
>
> > +    if (virDomainObjIsActive(vm)) {
> > +        virReportError(VIR_ERR_OPERATION_INVALID,
> > +                       "%s", _("Domain is already running"));
> > +        goto cleanup;
> > +    }
> > +
> > +    ret = virBhyveProcessStart(dom->conn, privconn, vm,
> > +                               VIR_DOMAIN_RUNNING_BOOTED,
> > +                               start_flags);
> > +    if (ret) {
> > +        virObjectUnref(dom);
> > +        dom = NULL;
> > +        goto cleanup;
> > +    }
> > +
> > + cleanup:
> > +    virObjectUnref(caps);
> > +    virDomainDefFree(def);
> > +    virObjectUnlock(vm);
> > +
> > +    return dom;
> > +}
> > +
> >  static virDriver bhyveDriver = {
> >      .no = VIR_DRV_BHYVE,
> >      .name = "bhyve",
> > @@ -1091,6 +1169,7 @@ static virDriver bhyveDriver = {
> >      .connectNumOfDefinedDomains = bhyveConnectNumOfDefinedDomains, /*
> 1.2.2 */
> >      .domainCreate = bhyveDomainCreate, /* 1.2.2 */
> >      .domainCreateWithFlags = bhyveDomainCreateWithFlags, /* 1.2.3 */
> > +    .domainCreateXML = bhyveDomainCreateXML, /* 1.2.4 */
> >      .domainDestroy = bhyveDomainDestroy, /* 1.2.2 */
> >      .domainLookupByUUID = bhyveDomainLookupByUUID, /* 1.2.2 */
> >      .domainLookupByName = bhyveDomainLookupByName, /* 1.2.2 */
> > --
> > 1.9.0
> >
> > --
> > libvir-list mailing list
> > libvir-list at redhat.com
> > https://www.redhat.com/mailman/listinfo/libvir-list
>
> Roman Bogorodskiy
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140408/e2c2bc4e/attachment-0001.htm>