[libvirt] [PATCH 0/2] byve: domain management
Some enhancements and fixes with domain management. 1. Create stub for domainResume. Since bhyve does not support suspending, this one works a little different. If domain is already running, it reports success, but fails in all other cases. 2. add domainCreateXML routine 3. Fix persistency: when domain is not persistent, it should be forgotten upon domainDestroy. Wojciech Macek (2): bhyve: stub for domainResume bhyve: domainCreateXML src/bhyve/bhyve_driver.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 114 insertions(+), 1 deletion(-) -- 1.9.0
bhyve does not support suspend nor resume. To enable OpenStack procedure (create -> resume) use stub for domainResume function. If domain is running then report success; return failure in all other cases. --- src/bhyve/bhyve_driver.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index 84d1915..f7a8912 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -1041,6 +1041,39 @@ bhyveConnectCompareCPU(virConnectPtr conn, return ret; } +static int +bhyveDomainResume(virDomainPtr dom) +{ + virDomainObjPtr vm; + int ret = -1; + int state; + + if (!(vm = bhyveDomObjFromDomain(dom))) + return -1; + + if (virDomainResumeEnsureACL(dom->conn, vm->def) < 0) + goto cleanup; + + + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is not running")); + goto cleanup; + } + + /* If domain is already running, return success */ + state = virDomainObjGetState(vm, NULL); + if (state == VIR_DOMAIN_RUNNING) { + ret = 0; + } + + /*Return failure in all other cases - bhyve does not support resuming */ + cleanup: + if (vm) + virObjectUnlock(vm); + return ret; +} + static virDriver bhyveDriver = { .no = VIR_DRV_BHYVE, .name = "bhyve", @@ -1080,6 +1113,7 @@ static virDriver bhyveDriver = { .nodeSetMemoryParameters = bhyveNodeSetMemoryParameters, /* 1.2.3 */ .connectBaselineCPU = bhyveConnectBaselineCPU, /* 1.2.4 */ .connectCompareCPU = bhyveConnectCompareCPU, /* 1.2.4 */ + .domainResume = bhyveDomainResume, /* 1.2.4 */ }; -- 1.9.0
On Tue, Apr 08, 2014 at 07:01:33AM +0200, Wojciech Macek wrote:
bhyve does not support suspend nor resume. To enable OpenStack procedure (create -> resume) use stub for domainResume function. If domain is running then report success; return failure in all other cases. --- src/bhyve/bhyve_driver.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+)
If bhyve doesn't support suspend or resume, then how is the 'create' method even succeeding. OpenStack will be passing VIR_DOMAIN_START_PAUSED to that method, so if suspend isn't support create should have raised an error. As such it should never get as far as trying to resume. On that basic NACK to this patch - we shouldn't implement APIs that we know don't work because it means apps that need to know this won't get proper error reporting. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Implement bhyveDomainCreteXML function. --- 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; + } + cleanup: - virObjectUnlock(vm); + if (vm) + virObjectUnlock(vm); 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; + + 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
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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Roman Bogorodskiy
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@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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Roman Bogorodskiy
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
Oh, yep. We don't need an ACL again. I will send new patchset today. Thanks, Wojtek 2014-04-08 12:09 GMT+02:00 Roman Bogorodskiy <bogorodskiy@gmail.com>:
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
participants (3)
-
Daniel P. Berrange -
Roman Bogorodskiy -
Wojciech Macek