Wojciech Macek wrote:

> 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...

That would be good, thanks!

> 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.

Yeah, looks like you're right, sorry for the confusion.

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

As for the ACL...

> > > +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)

We check it here...

> > > +        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 need it here again?

Roman Bogorodskiy