Hi Daniel, Thanks for the very helpful (and prompt!) feedback. Responses inline. cheers, Zane. On 13/07/11 08:49, Andrew Beekhof wrote:
On Wed, Jul 13, 2011 at 2:46 AM, Daniel P. Berrange<berrange@xxxxxxxxxx> wrote:On Tue, Jul 12, 2011 at 06:28:46PM +0200, Zane Bitter wrote:diff --git a/src/DomainWrap.cpp b/src/DomainWrap.cpp index 47876de..eab6bbc 100644 --- a/src/DomainWrap.cpp +++ b/src/DomainWrap.cpp @@ -2,266 +2,310 @@ #include "NodeWrap.h" #include "DomainWrap.h" #include "Error.h" +#include "Exception.h" -#include "ArgsDomainMigrate.h" -#include "ArgsDomainRestore.h" -#include "ArgsDomainSave.h" -#include "ArgsDomainGetXMLDesc.h" +#include<cstdio> -namespace _qmf = qmf::com::redhat::libvirt; -DomainWrap::DomainWrap(ManagementAgent *agent, NodeWrap *parent, - virDomainPtr _domain_ptr, virConnectPtr _conn) : - domain_ptr(_domain_ptr), conn(_conn) +DomainWrap::DomainWrap( + NodeWrap *parent, + virDomainPtr domain_ptr, + virConnectPtr conn): + PackageOwner<NodeWrap::PackageDefinition>(parent), + ManagedObject(package().data_Domain), + _domain_ptr(domain_ptr), _conn(conn) { char dom_uuid[VIR_UUID_STRING_BUFLEN]; - domain = NULL; - - if (virDomainGetUUIDString(domain_ptr, dom_uuid)< 0) { - REPORT_ERR(conn, "DomainWrap: Unable to get UUID string of domain."); + if (virDomainGetUUIDString(_domain_ptr, dom_uuid)< 0) { + REPORT_ERR(_conn, "DomainWrap: Unable to get UUID string of domain."); throw 1;I've noticed that most (all?) objects are being passed in a virConnectPtr instance too. This should be redundant for two reasons - If you have a virDomainPtr, you can call virDomainGetConnect() to get back the associated virConnectPtr (likewise for other libvirt objects) - We should kill the 'virConnectPtr' parameter from the REPORT_ERR macro/functions, and use 'virGetLastError' exclusively. This function is thread-safe, unlike the current virConnGetLastError being used. Since this problem was pre-existing, I'll leave it upto you whether you want to incorporate such a change into this patch or do it as a later followup.Personally I'd prefer it be in a follow-up patch.
Agree, this is a good candidate for a follow-up.
void DomainWrap::update() { virDomainInfo info; int ret; - ret = virDomainGetInfo(domain_ptr,&info); + ret = virDomainGetInfo(_domain_ptr,&info); if (ret< 0) { - REPORT_ERR(conn, "Domain get info failed."); + REPORT_ERR(_conn, "Domain get info failed."); /* Next domainSync() will take care of this in the node wrapper if the domain is * indeed dead. */ return; } else { + const char *state = NULL; switch (info.state) { - case VIR_DOMAIN_NOSTATE:Add a 'default:' clause here- domain->set_state("nostate"); + state = "nostate"; break; case VIR_DOMAIN_RUNNING: - domain->set_state("running"); + state = "running"; break; case VIR_DOMAIN_BLOCKED: - domain->set_state("blocked"); + state = "blocked"; break; case VIR_DOMAIN_PAUSED: - domain->set_state("paused"); + state = "paused"; break; case VIR_DOMAIN_SHUTDOWN: - domain->set_state("shutdown"); + state = "shutdown"; break; case VIR_DOMAIN_SHUTOFF: - domain->set_state("shutoff"); + state = "shutoff"; break; case VIR_DOMAIN_CRASHED: - domain->set_state("crashed"); + state = "crashed"; break; } - domain->set_numVcpus(info.nrVirtCpu); - domain->set_maximumMemory(info.maxMem); - domain->set_memory(info.memory); - domain->set_cpuTime(info.cpuTime); + if (state) { + _data.setProperty("state", state); + }We shouldn't skip out 'NOSTATE' here, juust make this unconditional+ _data.setProperty("numVcpus", info.nrVirtCpu); + _data.setProperty("maximumMemory", info.maxMem); + _data.setProperty("memory", (uint64_t)info.memory); + _data.setProperty("cpuTime", (uint64_t)info.cpuTime); }
Fixed. Thanks!There is a similar issue with the state of a Pool (as written it will segfault if a state other than inactive/building/running/degraded is returned). What would be an appropriate default there? Just set it to "unknown"?
+bool +DomainWrap::migrate(qmf::AgentSession& session, qmf::AgentEvent& event) +{ + virConnectPtr dest_conn; + virDomainPtr rem_dom; + qpid::types::Variant::Map args(event.getArguments()); + int ret; + + // This is actually quite broken. Most setups won't allow unauthorized + // connection from one node to another directly like this. + dest_conn = virConnectOpen(args["destinationUri"].asString().c_str());Since the agent was originally written, we now have a new migration API which avoids the need for a destination virConnectPtr object. Instead you can pass the 'destinationUri' directly to virDomainMigrateToURI, and set the PEER2PEER flag. NB, this still presumes that the source libvirtd can auth with the target libvirtd directly, but it avoids the need for matahari itself to authenticate. Typically we'd expect the libvirtd's to be setup with TLS+x590 or Kerberos so they can talk to each other+ if (!dest_conn) { + std::string err = FORMAT_ERR(dest_conn, "Unable to connect to remote system for migration: virConnectOpen",&ret); + raiseException(session, event, err, STATUS_USER + ret); + return false; + } + + const std::string newDomainName_arg(args["newDomainName"]); + const char *new_dom_name = NULL; + if (newDomainName_arg.size()> 0) { + new_dom_name = newDomainName_arg.c_str(); + } + + const std::string uri_arg(args["uri"]); + const char *uri = NULL; + if (uri_arg.size()> 0) { + uri = uri_arg.c_str(); + } + + uint32_t flags(args["flags"]); + uint32_t bandwidth(args["bandwidth"]); + + printf ("calling migrate, new_dom_name: %s, uri: %s, flags: %d (live is %d)\n", + new_dom_name ? new_dom_name : "NULL", + uri ? uri : "NULL", + flags, + VIR_MIGRATE_LIVE); + + rem_dom = virDomainMigrate(_domain_ptr, dest_conn, flags, + new_dom_name, + uri, + bandwidth);Ideally you want virDomainMigrateToURI, or even better virDomainMigrateToURI2 here.No objection at all to making these sorts of changes, lets just do them as a separate work item.
Yep.
diff --git a/src/Error.h b/src/Error.h index 388b41a..5bd7397 100644 --- a/src/Error.h +++ b/src/Error.h @@ -1,11 +1,19 @@ +#ifndef ERROR_H +#define ERROR_H + +#include<libvirt/libvirt.h> +#include<libvirt/virterror.h> + #define REPORT_ERR(conn,msg) reportError(conn, msg, __func__, __LINE__, __FILE__) #define FORMAT_ERR(conn,msg,err_ret) formatError(conn, msg, err_ret, __func__, __LINE__, __FILE__) + void reportError(virConnectPtr conn, const char *msg, const char *function, int line, const char *file); std::string formatError(virConnectPtr conn, const char *msg, int *err_ret, const char *function, int line, const char *file); +#endifWe want to remove virConnectPtr from both these methods& macrosdiff --git a/src/Exception.cpp b/src/Exception.cpp new file mode 100644 index 0000000..105df27 --- /dev/null +++ b/src/Exception.cpp @@ -0,0 +1,38 @@ +#include "Exception.h" +#include "Error.h" +#include<qmf/Schema.h> +#include<qmf/SchemaProperty.h> +#include<qmf/Data.h> + + +#define ERROR_TEXT "error_text" +#define ERROR_CODE "error_code" + + +static qmf::Schema errorSchema(qmf::SCHEMA_TYPE_DATA, + "com.redhat.libvirt", "error");This should be 'org.libvirt' really.
Fixed.
generated_file_list = \ - qmf/com/redhat/libvirt/Domain.cpp\ - qmf/com/redhat/libvirt/Node.cpp\ - qmf/com/redhat/libvirt/Package.cpp\ - qmf/com/redhat/libvirt/Pool.cpp\ - qmf/com/redhat/libvirt/Volume.cpp\ - qmf/com/redhat/libvirt/ArgsDomainGetXMLDesc.h\ - qmf/com/redhat/libvirt/ArgsDomainMigrate.h\ - qmf/com/redhat/libvirt/ArgsDomainRestore.h\ - qmf/com/redhat/libvirt/ArgsDomainSave.h\ - qmf/com/redhat/libvirt/ArgsNodeDomainDefineXML.h\ - qmf/com/redhat/libvirt/ArgsNodeStoragePoolCreateXML.h\ - qmf/com/redhat/libvirt/ArgsNodeStoragePoolDefineXML.h\ - qmf/com/redhat/libvirt/ArgsPoolCreateVolumeXML.h\ - qmf/com/redhat/libvirt/ArgsPoolGetXMLDesc.h\ - qmf/com/redhat/libvirt/ArgsVolumeGetXMLDesc.h\ - qmf/com/redhat/libvirt/Domain.h\ - qmf/com/redhat/libvirt/Node.h\ - qmf/com/redhat/libvirt/Package.h\ - qmf/com/redhat/libvirt/Pool.h\ - qmf/com/redhat/libvirt/Volume.h + qmf/com/redhat/libvirt/QmfPackage.cpp\ + qmf/com/redhat/libvirt/QmfPackage.hCan we change these to org/libvirt too, or will that be an incompatible schema change ?I believe it would be. Ted would know for sure.Incidentally, is this fully backwards compatible from the POV of existing libvirt-qpid client apps, or are we starting a v2 libvirt schema here ?I believe he has some tests which both versions pass. So I'm pretty sure the API is more than backwards compatible, it should be identical.
That is correct (I will eventually post the test tool here too), and it goes some way to proving that it is, for now, possible to write a client that works with both the old and new versions. What it does not tell us is whether any client written against the previous version will work with the new matahari version.
There are definite differences you can see just by playing around with qmf-tool. For instance, with v1 you don't need to specify the package (the com.redhat.libvirt part) when you do a query, but with v2 you do. Then there's that bizarre thing where v1 appears to convert the class names to lower case, but v2 doesn't (which patch #2 fixes). There may be others. The number of hoops to jump through just to return error codes was... surprising.
From the other discussion in this thread it sounds like now is a good time to break backwards compatibility, change the package name to org.libvirt and drop that second patch (for consistency with other matahari agents).
static void @@ -521,7 +577,6 @@ print_usage() printf("\t-d | --daemon run as a daemon.\n"); printf("\t-h | --help print this help message.\n"); printf("\t-b | --broker specify broker host name..\n"); - printf("\t-g | --gssapi force GSSAPI authentication.\n"); printf("\t-u | --username username to use for authentication purproses.\n"); printf("\t-s | --service service name to use for authentication purproses.\n"); printf("\t-p | --port specify broker port.\n"); @@ -536,8 +591,7 @@ int main(int argc, char** argv) { int idx = 0; bool verbose = false; bool daemonize = false; - bool gssapi = false; - char *host = NULL; + const char *host = NULL; char *username = NULL; char *service = NULL; int port = 5672; @@ -546,14 +600,13 @@ int main(int argc, char** argv) { {"help", 0, 0, 'h'}, {"daemon", 0, 0, 'd'}, {"broker", 1, 0, 'b'}, - {"gssapi", 0, 0, 'g'}, {"username", 1, 0, 'u'}, {"service", 1, 0, 's'}, {"port", 1, 0, 'p'}, {0, 0, 0, 0} }; - while ((arg = getopt_long(argc, argv, "hdb:gu:s:p:", opt,&idx)) != -1) { + while ((arg = getopt_long(argc, argv, "hdb:u:s:p:", opt,&idx)) != -1) { switch (arg) { case 'h': case '?': @@ -582,9 +635,6 @@ int main(int argc, char** argv) { exit(1); } break; - case 'g': - gssapi = true; - break; case 'p': if (optarg) { port = atoi(optarg); @@ -620,44 +670,42 @@ int main(int argc, char** argv) { // This prevents us from dying if libvirt disconnects. signal(SIGPIPE, SIG_IGN); - // Create the management agent - ManagementAgent::Singleton singleton; - ManagementAgent* agent = singleton.getInstance(); - - // Register the schema with the agent - _qmf::Package packageInit(agent); - - // Start the agent. It will attempt to make a connection to the - // management broker. The third argument is the interval for sending - // updates to stats/properties to the broker. The last is set to 'true' - // to keep this all single threaded. Otherwise a second thread would be - // used to implement methods. - - qpid::management::ConnectionSettings settings; - settings.host = host ? host : "127.0.0.1"; - settings.port = port; + qpid::types::Variant::Map options; if (username != NULL) { - settings.username = username; + options["username"] = username; } if (service != NULL) { - settings.service = service; + options["service"] = service; } - if (gssapi == true) { - settings.mechanism = "GSSAPI"; + + if (host == NULL) { + host = "127.0.0.1"; } - agent->setName("Red Hat", "libvirt-qpid"); - agent->init(settings, 3, true); + std::stringstream url; + + url<< "amqp:"<< "tcp"<< ":"<< host<< ":"<< port; + + qpid::messaging::Connection amqp_connection(url.str(), options); + amqp_connection.open(); + + qmf::AgentSession session(amqp_connection); + session.setVendor("redhat.com");IMHO, this should really be 'org.libvirt' too
FWIW once this becomes a matahari agent this gets changed to "matahariproject.org" anyway whether we like it or not. (Perhaps matahari ought to make this configurable?)
In fact, the previous vendor name was actually "Red Hat", and I had to change it because having a space in it borked something in v2 (I forget what).
Since backward compatibility with anything relying on the agent vendor is going to be broken anyway, I've gone ahead and changed this patch to make it "libvirt.org".
+ session.setProduct("libvirt-qpid"); + session.setAttribute("hostname", host);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 :| _______________________________________________ Matahari mailing list Matahari@xxxxxxxxxxxxxxxxxxxxxx https://fedorahosted.org/mailman/listinfo/matahari
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list