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

[Roman Bogorodskiy]

  Daniel P. Berrange wrote:

> On Wed, Apr 09, 2014 at 07:52:12AM +0200, Wojciech Macek wrote:
> > Implement bhyveDomainCreteXML function.
> > ---
> >  src/bhyve/bhyve_driver.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 70 insertions(+)
> > 
> > diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c
> > index 23e7112..1357e07 100644
> > --- a/src/bhyve/bhyve_driver.c
> > +++ b/src/bhyve/bhyve_driver.c
> > @@ -1107,6 +1107,75 @@ bhyveConnectCompareCPU(virConnectPtr conn,
> >      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)))
> 
> You should pass VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, rather than 0.
> This ensures you don't overwrite the config of an existing runing
> guest
> 
> > +        goto cleanup;
> > +    def = NULL;
> > +    vm->persistent = 0;
> 
> Don't reset the persistent flag. The guest may already exist
> as an inactive persistent guest, and you're causing that to be
> lost.
> 
> > +
> > +    dom = virGetDomain(conn, vm->def->name, vm->def->uuid);
> > +    if (!dom)
> > +        goto cleanup;
> > +
> > +    dom->id = vm->def->id;
> 
> Err, the 'id' won't be set until you've started the guest,
> so this fnuction call & set needs to be the last thing you
> do
> 
> > +
> > +    if (flags & VIR_DOMAIN_START_AUTODESTROY)
> > +        start_flags |= VIR_BHYVE_PROCESS_START_AUTODESTROY;
> 
> You've already done this above.
> 
> > +
> > +    if (virDomainObjIsActive(vm)) {
> > +        virReportError(VIR_ERR_OPERATION_INVALID,
> > +                       "%s", _("Domain is already running"));
> > +        goto cleanup;
> > +    }
> 
> This is not required if you pass the right flags to the
> virDomainObjListAdd method
> 
> > +
> > +    ret = virBhyveProcessStart(dom->conn, privconn, vm,
> > +                               VIR_DOMAIN_RUNNING_BOOTED,
> > +                               start_flags);
> > +    if (ret) {

As discussed on IRC now, I think we need to call
virDomainObjListRemove() if the domain is not persistent in case if 
virBhyveProcessStart() fails.

Daniel, please correct me if I'm wrong.

> > +        virObjectUnref(dom);
> > +        dom = NULL;
> > +        goto cleanup;
> > +    }

Roman Bogorodskiy