4:29 a.m.
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(a)redhat.com> 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);
>>
>> +#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.
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.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.
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(a)lists.fedorahosted.org
> https://fedorahosted.org/mailman/listinfo/matahari
>