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

Re: [libvirt] [RFC] Power Hypervisor Libvirt support



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 linux vnet ibm com


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