[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [Matahari] [libvirt-qpid PATCH 1/3] Convert to QMFv2 APIs



On Wed, Jul 13, 2011 at 2:46 AM, Daniel P. Berrange <berrange 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.

>>  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 lists fedorahosted org
> https://fedorahosted.org/mailman/listinfo/matahari
>


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]