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

On Fri, Mar 20, 2009 at 01:58:50PM -0300, Eduardo Otubo wrote: That seems like a reasonably choice, unless the web server in the HMC/IVM provided a good formal API like xmlrpc. So if I'm understanding what I read on the web correctly, the HMC is a machine that is typically separate from the host running virtualization. Thus a single HMC can manage multiple hosts. For the URI scheme, I'd see two possible options Either use the path component of the URI for managed system name. phyp://$user@$HMC/$managedsystem Or use a query parameter phyp://$user@$HMC/?managedsystem=$managedsystem I reckon I'd favour the former. For this it is preferable to avoid using hardcoded paths in this way. If we are going to use libssh2 for phyp driver, then I reckon it woul dbe desirable to also use it for our existing remote RPC driver too (it currently just fork/exec's /usr/bin/ssh). Thus I'd recommend adding an explicit arg for --with-libssh2=$PREFIX. So if someone did --with-libssh2 it'd just look for it in /usr. While, if they did --with-libssh2=/usr/local then it'd search the alternate path given. Then, when processing your '$with_phyp' arg I'd recommend the following logic - If no --with-phyp arg is given - If libssh2 was detected, enable phyp driver by default - else disable the phyp driver by default - If a --with-phyp arg is given - If libssh2 was detected, then use it - Else AC_MSG_ERROR() to tell use libssh was missing & is needed for phyp - If a --without-phyp arg is given - diable the phyp driver Can you either shortern this to just 'Phyp: $with_phyp', or fix the indentation for all the other existing lines here, so everything is aligned sensibly. This should typically be done in the .c file, rather than the .h files. Our general coding style is to keep the variabl declarations at the start of the nearest enclosing {} block. So either at start of the function, or start of the while/if/for loop. Rather than calling 'getpass', you should check to see if the caller has suplied an authentication callback - eg the 'virConnectAuthPtr auth' parameter which is passed in to this open method. 'getpass' will prompt on the command line, which isn't suitable for graphical apps, so you need to use the callback for collecting credentials. virsh has a default callback implementation that knows how to prompt for both username and password if requested by then open method, You could also use this to prompt for a passphrase if the SSH private key is not unlocked. In this case you'll find it simpler to use the virAsprintf() function which is available from src/util.h. This will automatically allocate an char * buffer large enough for the data your printf'ing. I think you probably also want to make sure you quote the string args in yuour SSH commands, and possibly also worry about escaping shell magic characters. I'd also recommend against passing in a fixed allocated buffer 'char *textual_return' here. Instead, use the 'virBuffer' object (from src/buf.h) which implements grow-on-demand buffer with strong error checking for OOM. > + > + if (ret) { > + CLEANUP; > + return LPAR_EXEC_ERR; > + } > + > + for (i = 0; textual_return[i] != '\n'; i++) > + id_c[i] = textual_return[i]; > + id_c[i] = '\0'; > + > + CLEANUP; > + return atoi(id_c); > +} > + > +static int > +phypGetLparNAME(SSH_SESSION * ssh_session, const char *managed_system, > + unsigned int lpar_id, char *lpar_name) > +{ > + unsigned int i = 0; > + int ret = 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; > + } > + > + sprintf(cmd, > + "lssyscfg -r lpar -m %s --filter lpar_ids=%d -F name", > + managed_system, lpar_id); > + > + if (ret) { > + CLEANUP; > + lpar_name = NULL; > + return LPAR_EXEC_ERR; > + } > + > + for (i = 0; textual_return[i] != '\n'; i++) > + lpar_name[i] = textual_return[i]; > + lpar_name[i] = '\0'; > + > + CLEANUP; > + return ret; > +} > + > +static int > +phypGetLparUUID(SSH_SESSION * ssh_session, const char *managed_system, > + unsigned int lpar_id, unsigned char *lpar_uuid) > +{ > + unsigned int i = 0; > + int ret = 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; > + } > + > + sprintf(cmd, > + "lssyscfg -r lpar -m %s --filter lpar_ids=%d -F logical_serial_num", > + managed_system, lpar_id); > + > + if (ret) { > + CLEANUP; > + lpar_uuid = NULL; > + return LPAR_EXEC_ERR; > + } > + > + for (i = 0; textual_return[i] != '\n'; i++) > + lpar_uuid[i] = textual_return[i]; > + lpar_uuid[i] = '\0'; > + > + CLEANUP; > + return ret; > +} > + > +static int > +phypNumDomains(virConnectPtr conn) > +{ > + SSH_SESSION *ssh_session = conn->privateData; > + const char *managed_system = conn->uri->server; > + int ret = 0; > + unsigned int i = 0, ndom = 1; > + 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; > + } For errors that occur in driver APIs, you can't return an error code in this way. See the 'src/libvirt.c' docs for description of the error code for method - it is usually '-1' or 'NULL' as appropriate. To give more detail to the caller, you can use one of the error reporting functions from src/virterror_internal.h > + > + sprintf(cmd, "lssyscfg -r lpar -m %s -F lpar_id", managed_system); > + > + if (ret) { > + CLEANUP; > + return 0; > + } > + > + for (i = 0; textual_return[i] != 0; i++) > + if (textual_return[i] == '\n') > + ndom++; > + > + VIR_FREE(cmd); > + VIR_FREE(textual_return); > + return ndom; > +} > + > +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); > + > + for (i = 0; textual_return[i] != 0; i++) { > + if (textual_return[i] == '\n') { > + ids[nids] = atoi(id_c); > + memset(id_c, 0, 10); > + j = 0; > + nids++; > + continue; > + } > + id_c[j] = textual_return[i]; > + j++; > + } > + > + VIR_FREE(cmd); > + VIR_FREE(textual_return); > + return nids; > +} > + > +static virDomainPtr > +phypDomainLookupByName(virConnectPtr conn, const char *name) > +{ > + SSH_SESSION *ssh_session = conn->privateData; > + virDomainPtr dom = NULL; > + const char *managed_system = conn->uri->server; > + unsigned char *lpar_uuid; > + > + if (VIR_ALLOC_N(lpar_uuid, 100 * sizeof(char))) { > + return NULL; > + } > + int lpar_id = 0; > + > + lpar_id = phypGetLparID(ssh_session, managed_system, name); > + if (lpar_id == PHYP_NO_MEM) > + return NULL; > + > + if (phypGetLparUUID(ssh_session, managed_system, lpar_id, lpar_uuid) == > + PHYP_NO_MEM) > + return NULL; > + > + dom = virGetDomain(conn, name, lpar_uuid); > + > + if (dom) > + dom->id = lpar_id; > + > + VIR_FREE(lpar_uuid); > + return dom; > +} > + > +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; > + > + if (VIR_ALLOC_N(lpar_name, 100 * sizeof(char))) > + return NULL; > + > + if (VIR_ALLOC_N(lpar_uuid, 100 * sizeof(char))) > + return NULL; > + > + if (phypGetLparNAME(ssh_session, managed_system, lpar_id, lpar_name) == > + PHYP_NO_MEM) > + return NULL; > + > + if (phypGetLparUUID(ssh_session, managed_system, lpar_id, lpar_uuid) == > + PHYP_NO_MEM) > + return NULL; > + > + dom = virGetDomain(conn, lpar_name, lpar_uuid); > + > + if (dom) > + dom->id = lpar_id; > + > + VIR_FREE(lpar_name); > + VIR_FREE(lpar_uuid); > + return dom; > +} Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|