[libvirt] [RFC] Power Hypervisor Libvirt support

[Kaitlin Rupert]

Eduardo Otubo wrote:
> Sorry all about my last email, the subject should be "[RFC] Power
> Hypervisor
> Libvirt support". There should be an error here.
> 
> Thanks again,
> 

Hello,

I don't have access to a phyp machine, so unfortunately, I'm unable to 
test this out.  However, I've done a quick read through and have a few 
comments.

 > diff --git a/src/phyp_conf.c b/src/phyp_conf.c
 > new file mode 100644
 > index 0000000..4317936
 > --- /dev/null
 > +++ b/src/phyp_conf.c
 > @@ -0,0 +1,24 @@
 > +/*
 > + * Copyright IBM Corp. 2009
 > + *
 > + * phyp_driver.c: ssh layer to access Power Hypervisors

This file is phyp_conf.c, not phyp_driver.c

 > diff --git a/src/phyp_conf.h b/src/phyp_conf.h
 > new file mode 100644
 > index 0000000..51ca65e
 > --- /dev/null
 > +++ b/src/phyp_conf.h
 > @@ -0,0 +1,44 @@
 > +/*
 > + * Copyright IBM Corp. 2009
 > + *
 > + * phyp_driver.c: ssh layer to access Power Hypervisors

This is phyp_conf.h, not phyp_driver.c


 > +#include "domain_conf.h"
 > +/* Main driver state */
 > +typedef struct __phyp_driver phyp_driver_t;
 > +struct __phyp_driver {
 > +    virMutex lock;
 > +
 > +    virCapsPtr *caps;
 > +
 > +    virDomainObjList *domains;
 > +
 > +    char *configDir;
 > +    char *autostartDir;
 > +    char *stateDir;
 > +    char *logDir;
 > +    int have_netns;
 > +};

I don't see this struct / typedef being used anywhere.  Will this be 
used for something in the future?


 > +static int
 > +phypListDomains(virConnectPtr conn, int *ids, int nids)
 > +{
 > +    SSH_SESSION *ssh_session = conn->privateData;
 > +    const char *managed_system = conn->uri->server;
 > +    int ret = 0;
 > +    char id_c[10];
 > +    unsigned int i = 0, j = 0;
 > +    char *cmd;
 > +    char *textual_return;
 > +
 > +    if (VIR_ALLOC_N(cmd, MAXSIZE+sizeof(managed_system))) {
 > +        return PHYP_NO_MEM;
 > +    }
 > +
 > +    if (VIR_ALLOC_N(textual_return, MAXSIZE)) {
 > +        return PHYP_NO_MEM;
 > +    }
 > +
 > +    nids = 0;
 > +
 > +    memset(id_c, 0, 10);
 > +    sprintf(cmd, "lssyscfg -r lpar -m %s -F lpar_id", managed_system);
 > +    ret = __inner_exec_command(ssh_session, cmd, textual_return);

You don't check ret to see if an error occured.

 > +static virDomainPtr
 > +phypDomainLookupByID(virConnectPtr conn, int lpar_id)
 > +{
 > +    SSH_SESSION *ssh_session = conn->privateData;
 > +    virDomainPtr dom = NULL;
 > +    const char *managed_system = conn->uri->server;
 > +    char *lpar_name;
 > +    unsigned char *lpar_uuid;

Any reason not to use a virDomainObjPtr to hold these values?


 > +
 > +    if (VIR_ALLOC_N(lpar_name, 100 * sizeof(char)))
 > +        return NULL;
 > +
 > +    if (VIR_ALLOC_N(lpar_uuid, 100 * sizeof(char)))
 > +        return NULL;

You then wouldn't need to use a magic number here.  You could just do an 
alloc of the virDomainObjPtr object itself.

-- 
Kaitlin Rupert
IBM Linux Technology Center
kaitlin at linux.vnet.ibm.com