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

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 :| _______________________________________________ Matahari mailing list Matahari(a)lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/matahari