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

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