[libvirt] [PATCH 0/4] parallels: a set of fixes
Hello, I'd like to continue working on the parallels driver. So here is a set of patches, which resolves some problems. Now Parallels has a definite goal with this driver: we want OpenStack to be working with Parallels Cloud Server (PCS) over libvirt, and we a ready to implement all needed API calls in libvirt's driver and make necessary changes in PCS. P.S. I appreciate greatly effort needed to support the driver in compilable state and a lot of refactoring made by libvirt team, thanks! Dmitry Guryanov (4): parallels: fix virDomainDef.features comparison parallels: don't enable VNC when we define a new domain parallels: don't add domain to the list twice parallels: add a set of trivial functions src/parallels/parallels_driver.c | 57 ++++++++++++++++++++++------------------ 1 file changed, 31 insertions(+), 26 deletions(-) -- 1.9.0
virDomainDef.features became an array, so now we can't simply compare one features variable to another. We need to compare each each element from the array. Signed-off-by: Dmitry Guryanov <dguryanov@parallels.com> --- src/parallels/parallels_driver.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c index 848ed9f..41bb34b 100644 --- a/src/parallels/parallels_driver.c +++ b/src/parallels/parallels_driver.c @@ -1955,6 +1955,7 @@ static int parallelsApplyChanges(virConnectPtr conn, virDomainObjPtr dom, virDomainDefPtr new) { char buf[32]; + size_t i; virDomainDefPtr old = dom->def; parallelsDomObjPtr pdom = dom->privateData; @@ -2131,11 +2132,13 @@ parallelsApplyChanges(virConnectPtr conn, virDomainObjPtr dom, virDomainDefPtr n return -1; } - if (old->features != new->features) { - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("changing features is not supported " - "by parallels driver")); - return -1; + for (i = 0; i < VIR_DOMAIN_FEATURE_LAST; i++) { + if (old->features[i] != new->features[i]) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("changing features is not supported " + "by parallels driver")); + return -1; + } } if (new->clock.offset != VIR_DOMAIN_CLOCK_OFFSET_UTC || -- 1.9.0
On Wed, Apr 23, 2014 at 06:35:00PM +0400, Dmitry Guryanov wrote:
virDomainDef.features became an array, so now we can't simply compare one features variable to another. We need to compare each each element from the array.
Signed-off-by: Dmitry Guryanov <dguryanov@parallels.com> --- src/parallels/parallels_driver.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c index 848ed9f..41bb34b 100644 --- a/src/parallels/parallels_driver.c +++ b/src/parallels/parallels_driver.c @@ -1955,6 +1955,7 @@ static int parallelsApplyChanges(virConnectPtr conn, virDomainObjPtr dom, virDomainDefPtr new) { char buf[32]; + size_t i;
virDomainDefPtr old = dom->def; parallelsDomObjPtr pdom = dom->privateData; @@ -2131,11 +2132,13 @@ parallelsApplyChanges(virConnectPtr conn, virDomainObjPtr dom, virDomainDefPtr n return -1; }
- if (old->features != new->features) { - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("changing features is not supported " - "by parallels driver")); - return -1; + for (i = 0; i < VIR_DOMAIN_FEATURE_LAST; i++) { + if (old->features[i] != new->features[i]) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("changing features is not supported " + "by parallels driver")); + return -1; + } }
if (new->clock.offset != VIR_DOMAIN_CLOCK_OFFSET_UTC ||
ACK 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 :|
I added this code year ago, instead of implementing ability to change VNC configuration, which was not trivial, I added extra call to prlctl, which sets up VNC with auto port, despite VNC configuration given by a user. Let's remove this hack, because, first, it doesn't work on the latest Parallels Cloud Server release (you have to either specify --vnc-nopasswd option or password). And also has problem with error handling. If second call to prlctl fails, VM, created by first call to prlctl, will not be removed. Signed-off-by: Dmitry Guryanov <dguryanov@parallels.com> --- src/parallels/parallels_driver.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c index 41bb34b..1c58b45 100644 --- a/src/parallels/parallels_driver.c +++ b/src/parallels/parallels_driver.c @@ -2258,9 +2258,6 @@ parallelsCreateVm(virConnectPtr conn, virDomainDefPtr def) "--uuid", uuidstr, NULL) < 0) goto error2; - if (parallelsCmdRun(PRLCTL, "set", def->name, "--vnc-mode", "auto", NULL) < 0) - goto error2; - virStoragePoolObjUnlock(pool); virObjectUnref(vol); @@ -2294,9 +2291,6 @@ parallelsCreateCt(virConnectPtr conn ATTRIBUTE_UNUSED, virDomainDefPtr def) "--ostemplate", def->fss[0]->src, NULL) < 0) goto error; - if (parallelsCmdRun(PRLCTL, "set", def->name, "--vnc-mode", "auto", NULL) < 0) - goto error; - return 0; error: -- 1.9.0
On Wed, Apr 23, 2014 at 06:35:01PM +0400, Dmitry Guryanov wrote:
I added this code year ago, instead of implementing ability to change VNC configuration, which was not trivial, I added extra call to prlctl, which sets up VNC with auto port, despite VNC configuration given by a user.
Let's remove this hack, because, first, it doesn't work on the latest Parallels Cloud Server release (you have to either specify --vnc-nopasswd option or password). And also has problem with error handling. If second call to prlctl fails, VM, created by first call to prlctl, will not be removed.
And even if we do want VNC, then we should only do it if the virDomainDefPtr graphics config actually requests VNC.
Signed-off-by: Dmitry Guryanov <dguryanov@parallels.com> --- src/parallels/parallels_driver.c | 6 ------ 1 file changed, 6 deletions(-)
diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c index 41bb34b..1c58b45 100644 --- a/src/parallels/parallels_driver.c +++ b/src/parallels/parallels_driver.c @@ -2258,9 +2258,6 @@ parallelsCreateVm(virConnectPtr conn, virDomainDefPtr def) "--uuid", uuidstr, NULL) < 0) goto error2;
- if (parallelsCmdRun(PRLCTL, "set", def->name, "--vnc-mode", "auto", NULL) < 0) - goto error2; - virStoragePoolObjUnlock(pool); virObjectUnref(vol);
@@ -2294,9 +2291,6 @@ parallelsCreateCt(virConnectPtr conn ATTRIBUTE_UNUSED, virDomainDefPtr def) "--ostemplate", def->fss[0]->src, NULL) < 0) goto error;
- if (parallelsCmdRun(PRLCTL, "set", def->name, "--vnc-mode", "auto", NULL) < 0) - goto error; - return 0;
ACK, 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 :|
There is a problem with function parallelsDomainDefineXML. If we are defining a new domain, then we need to do 2 things: aclually create a VM in PCS and add new domain to the cached list of domains _parallelsConn.domains. This is done in the function parallelsLoadDomains. So call to virDomainObjListAdd will return a error, because a domain with the same name and id will already be in the list. Signed-off-by: Dmitry Guryanov <dguryanov@parallels.com> --- src/parallels/parallels_driver.c | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c index 1c58b45..4ab3e1c 100644 --- a/src/parallels/parallels_driver.c +++ b/src/parallels/parallels_driver.c @@ -2303,7 +2303,7 @@ parallelsDomainDefineXML(virConnectPtr conn, const char *xml) parallelsConnPtr privconn = conn->privateData; virDomainPtr ret = NULL; virDomainDefPtr def; - virDomainObjPtr dom = NULL, olddom = NULL; + virDomainObjPtr olddom = NULL; parallelsDriverLock(privconn); if ((def = virDomainDefParseString(xml, privconn->caps, privconn->xmlopt, @@ -2345,24 +2345,12 @@ parallelsDomainDefineXML(virConnectPtr conn, const char *xml) } virObjectUnlock(olddom); - if (!(dom = virDomainObjListAdd(privconn->domains, def, - privconn->xmlopt, - 0, NULL))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Can't allocate domobj")); - goto cleanup; - } - - def = NULL; - - ret = virGetDomain(conn, dom->def->name, dom->def->uuid); + ret = virGetDomain(conn, def->name, def->uuid); if (ret) - ret->id = dom->def->id; + ret->id = def->id; cleanup: virDomainDefFree(def); - if (dom) - virObjectUnlock(dom); parallelsDriverUnlock(privconn); return ret; } -- 1.9.0
On Wed, Apr 23, 2014 at 06:35:02PM +0400, Dmitry Guryanov wrote:
There is a problem with function parallelsDomainDefineXML. If we are defining a new domain, then we need to do 2 things: aclually create a VM in PCS and add new domain to the cached list of domains _parallelsConn.domains.
This is done in the function parallelsLoadDomains. So call to virDomainObjListAdd will return a error, because a domain with the same name and id will already be in the list.
Signed-off-by: Dmitry Guryanov <dguryanov@parallels.com> --- src/parallels/parallels_driver.c | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-)
diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c index 1c58b45..4ab3e1c 100644 --- a/src/parallels/parallels_driver.c +++ b/src/parallels/parallels_driver.c @@ -2303,7 +2303,7 @@ parallelsDomainDefineXML(virConnectPtr conn, const char *xml) parallelsConnPtr privconn = conn->privateData; virDomainPtr ret = NULL; virDomainDefPtr def; - virDomainObjPtr dom = NULL, olddom = NULL; + virDomainObjPtr olddom = NULL;
parallelsDriverLock(privconn); if ((def = virDomainDefParseString(xml, privconn->caps, privconn->xmlopt, @@ -2345,24 +2345,12 @@ parallelsDomainDefineXML(virConnectPtr conn, const char *xml) } virObjectUnlock(olddom);
- if (!(dom = virDomainObjListAdd(privconn->domains, def, - privconn->xmlopt, - 0, NULL))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Can't allocate domobj")); - goto cleanup; - } - - def = NULL; - - ret = virGetDomain(conn, dom->def->name, dom->def->uuid); + ret = virGetDomain(conn, def->name, def->uuid); if (ret) - ret->id = dom->def->id; + ret->id = def->id;
cleanup: virDomainDefFree(def); - if (dom) - virObjectUnlock(dom); parallelsDriverUnlock(privconn); return ret; }
ACK 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 :|
Add functions parallelsIsAlive, parallelsIsEncrypted, parallelsIsSecure which are very simple to implement, but may be required by some libvirt users. Almost all other drivers have these functions. Signed-off-by: Dmitry Guryanov <dguryanov@parallels.com> --- src/parallels/parallels_driver.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c index 4ab3e1c..54d67a2 100644 --- a/src/parallels/parallels_driver.c +++ b/src/parallels/parallels_driver.c @@ -2362,6 +2362,23 @@ parallelsNodeGetInfo(virConnectPtr conn ATTRIBUTE_UNUSED, return nodeGetInfo(nodeinfo); } +static int parallelsConnectIsEncrypted(virConnectPtr conn ATTRIBUTE_UNUSED) +{ + /* Encryption is not relevant / applicable to way we talk to PCS */ + return 0; +} + +static int parallelsConnectIsSecure(virConnectPtr conn ATTRIBUTE_UNUSED) +{ + /* We run CLI tools directly so this is secure */ + return 1; +} + +static int parallelsConnectIsAlive(virConnectPtr conn ATTRIBUTE_UNUSED) +{ + return 1; +} + static virDriver parallelsDriver = { .no = VIR_DRV_PARALLELS, @@ -2392,6 +2409,9 @@ static virDriver parallelsDriver = { .domainShutdown = parallelsDomainShutdown, /* 0.10.0 */ .domainCreate = parallelsDomainCreate, /* 0.10.0 */ .domainDefineXML = parallelsDomainDefineXML, /* 0.10.0 */ + .connectIsEncrypted = parallelsConnectIsEncrypted, /* 1.2.4 */ + .connectIsSecure = parallelsConnectIsSecure, /* 1.2.4 */ + .connectIsAlive = parallelsConnectIsAlive, /* 1.2.4 */ }; /** -- 1.9.0
On Wed, Apr 23, 2014 at 06:35:03PM +0400, Dmitry Guryanov wrote:
Add functions parallelsIsAlive, parallelsIsEncrypted, parallelsIsSecure which are very simple to implement, but may be required by some libvirt users. Almost all other drivers have these functions.
Signed-off-by: Dmitry Guryanov <dguryanov@parallels.com> --- src/parallels/parallels_driver.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c index 4ab3e1c..54d67a2 100644 --- a/src/parallels/parallels_driver.c +++ b/src/parallels/parallels_driver.c @@ -2362,6 +2362,23 @@ parallelsNodeGetInfo(virConnectPtr conn ATTRIBUTE_UNUSED, return nodeGetInfo(nodeinfo); }
+static int parallelsConnectIsEncrypted(virConnectPtr conn ATTRIBUTE_UNUSED) +{ + /* Encryption is not relevant / applicable to way we talk to PCS */ + return 0; +} + +static int parallelsConnectIsSecure(virConnectPtr conn ATTRIBUTE_UNUSED) +{ + /* We run CLI tools directly so this is secure */ + return 1; +} + +static int parallelsConnectIsAlive(virConnectPtr conn ATTRIBUTE_UNUSED) +{ + return 1; +} +
static virDriver parallelsDriver = { .no = VIR_DRV_PARALLELS, @@ -2392,6 +2409,9 @@ static virDriver parallelsDriver = { .domainShutdown = parallelsDomainShutdown, /* 0.10.0 */ .domainCreate = parallelsDomainCreate, /* 0.10.0 */ .domainDefineXML = parallelsDomainDefineXML, /* 0.10.0 */ + .connectIsEncrypted = parallelsConnectIsEncrypted, /* 1.2.4 */ + .connectIsSecure = parallelsConnectIsSecure, /* 1.2.4 */ + .connectIsAlive = parallelsConnectIsAlive, /* 1.2.4 */ };
ACK, though I'll tweak the version to 1.2.5 before pushing to GIT. 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 :|
On Wednesday 07 May 2014 14:55:51 Daniel P. Berrange wrote:
On Wed, Apr 23, 2014 at 06:35:03PM +0400, Dmitry Guryanov wrote:
Add functions parallelsIsAlive, parallelsIsEncrypted, parallelsIsSecure which are very simple to implement, but may be required by some libvirt users. Almost all other drivers have these functions.
Signed-off-by: Dmitry Guryanov <dguryanov@parallels.com> ---
src/parallels/parallels_driver.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c index 4ab3e1c..54d67a2 100644 --- a/src/parallels/parallels_driver.c +++ b/src/parallels/parallels_driver.c @@ -2362,6 +2362,23 @@ parallelsNodeGetInfo(virConnectPtr conn ATTRIBUTE_UNUSED,> return nodeGetInfo(nodeinfo);
}
+static int parallelsConnectIsEncrypted(virConnectPtr conn ATTRIBUTE_UNUSED) +{ + /* Encryption is not relevant / applicable to way we talk to PCS */ + return 0; +} + +static int parallelsConnectIsSecure(virConnectPtr conn ATTRIBUTE_UNUSED) +{ + /* We run CLI tools directly so this is secure */ + return 1; +} + +static int parallelsConnectIsAlive(virConnectPtr conn ATTRIBUTE_UNUSED) +{ + return 1; +} +
static virDriver parallelsDriver = {
.no = VIR_DRV_PARALLELS,
@@ -2392,6 +2409,9 @@ static virDriver parallelsDriver = {
.domainShutdown = parallelsDomainShutdown, /* 0.10.0 */ .domainCreate = parallelsDomainCreate, /* 0.10.0 */ .domainDefineXML = parallelsDomainDefineXML, /* 0.10.0 */
+ .connectIsEncrypted = parallelsConnectIsEncrypted, /* 1.2.4 */ + .connectIsSecure = parallelsConnectIsSecure, /* 1.2.4 */ + .connectIsAlive = parallelsConnectIsAlive, /* 1.2.4 */
};
ACK, though I'll tweak the version to 1.2.5 before pushing to GIT.
Thanks!
Regards, Daniel
-- Dmitry Guryanov
On Wednesday 23 April 2014 18:34:59 Dmitry Guryanov wrote:
Hello,
I'd like to continue working on the parallels driver. So here is a set of patches, which resolves some problems.
Now Parallels has a definite goal with this driver: we want OpenStack to be working with Parallels Cloud Server (PCS) over libvirt, and we a ready to implement all needed API calls in libvirt's driver and make necessary changes in PCS.
P.S. I appreciate greatly effort needed to support the driver in compilable state and a lot of refactoring made by libvirt team, thanks!
Hello! I've sent these patches during code freeze, now new libvirt is released, so could you, please, check them?
Dmitry Guryanov (4): parallels: fix virDomainDef.features comparison parallels: don't enable VNC when we define a new domain parallels: don't add domain to the list twice parallels: add a set of trivial functions
src/parallels/parallels_driver.c | 57 ++++++++++++++++++++++------------------ 1 file changed, 31 insertions(+), 26 deletions(-)
-- Dmitry Guryanov
On Wed, May 07, 2014 at 05:29:14PM +0400, Dmitry Guryanov wrote:
On Wednesday 23 April 2014 18:34:59 Dmitry Guryanov wrote:
Hello,
I'd like to continue working on the parallels driver. So here is a set of patches, which resolves some problems.
Now Parallels has a definite goal with this driver: we want OpenStack to be working with Parallels Cloud Server (PCS) over libvirt, and we a ready to implement all needed API calls in libvirt's driver and make necessary changes in PCS.
P.S. I appreciate greatly effort needed to support the driver in compilable state and a lot of refactoring made by libvirt team, thanks!
Hello!
I've sent these patches during code freeze, now new libvirt is released, so could you, please, check them?
Thanks for the reminder. I'll aim to take a look at them soon. 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 :|
On Wed, May 07, 2014 at 02:45:09PM +0100, Daniel P. Berrange wrote:
On Wed, May 07, 2014 at 05:29:14PM +0400, Dmitry Guryanov wrote:
On Wednesday 23 April 2014 18:34:59 Dmitry Guryanov wrote:
Hello,
I'd like to continue working on the parallels driver. So here is a set of patches, which resolves some problems.
Now Parallels has a definite goal with this driver: we want OpenStack to be working with Parallels Cloud Server (PCS) over libvirt, and we a ready to implement all needed API calls in libvirt's driver and make necessary changes in PCS.
P.S. I appreciate greatly effort needed to support the driver in compilable state and a lot of refactoring made by libvirt team, thanks!
Hello!
I've sent these patches during code freeze, now new libvirt is released, so could you, please, check them?
Thanks for the reminder. I'll aim to take a look at them soon.
Ok all merged - sorry for missing these in the freeze. 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 :|
On Wed, May 07, 2014 at 05:29:14PM +0400, Dmitry Guryanov wrote:
On Wednesday 23 April 2014 18:34:59 Dmitry Guryanov wrote:
Hello,
I'd like to continue working on the parallels driver. So here is a set of patches, which resolves some problems.
Now Parallels has a definite goal with this driver: we want OpenStack to be working with Parallels Cloud Server (PCS) over libvirt, and we a ready to implement all needed API calls in libvirt's driver and make necessary changes in PCS.
Ah I hadn't noticed that mail, it's great news :-) thanks for the info ! Daniel
P.S. I appreciate greatly effort needed to support the driver in compilable state and a lot of refactoring made by libvirt team, thanks!
Hello!
I've sent these patches during code freeze, now new libvirt is released, so could you, please, check them?
Dmitry Guryanov (4): parallels: fix virDomainDef.features comparison parallels: don't enable VNC when we define a new domain parallels: don't add domain to the list twice parallels: add a set of trivial functions
src/parallels/parallels_driver.c | 57 ++++++++++++++++++++++------------------ 1 file changed, 31 insertions(+), 26 deletions(-)
-- Dmitry Guryanov
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Daniel Veillard | Open Source and Standards, Red Hat veillard@redhat.com | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Dmitry Guryanov