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.
> 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);
> }
>
> +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.
> 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);
>
> +#endif
We want to remove virConnectPtr from both these methods & macros
> diff --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.
> 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.h
Can we change these to org/libvirt too, or will that be an
incompatible schema change ?
Incidentally, is this fully backwards compatible from the
POV of existing libvirt-qpid client apps, or are we starting
a v2 libvirt schema here ?
> 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
> + 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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list