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