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.
>> 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.
No objection at all to making these sorts of changes, lets just do
them as a separate work item.
>
>
>> 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 ?
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.
>
>> 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 :|
> _______________________________________________
> 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