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

> The protocol between the console and the partition (LPAR) is not > disclosed, therefore I propose the driver to execute commands remoetly > over an SSH connection to the consoles to manage IBM LPARs. That seems like a reasonably choice, unless the web server in the HMC/IVM provided a good formal API like xmlrpc.

> The patch attached is the first scratch of the driver that will interact > with HMC over a SSH connection. The URI model that is > used in virsh command line is: > > virsh --conect phyp://$user@$server > > Some known issues are: > * Next step is to make the URI like this: phyp://$user@ > $HMC/@managed_system. Almost finished. What it takes now is > $server = $HMC = $managed_system. > * Next features in my TODO list are "resume", "stop" and "reboot" the > LPAR. 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.

> diff --git a/configure.in b/configure.in > index 6b2bb5e..201a7dc 100644 > --- a/configure.in > +++ b/configure.in > @@ -182,6 +182,8 @@ AC_ARG_WITH([uml], > [ --with-uml add UML support (on)],[],[with_uml=yes]) > AC_ARG_WITH([openvz], > [ --with-openvz add OpenVZ support (on)],[],[with_openvz=yes]) > +AC_ARG_WITH([phyp], > +[ --with-phyp add IBM HMC/IVM support (on)],[],[with_phyp=yes]) > AC_ARG_WITH([lxc], > [ --with-lxc add Linux Container support (on)],[],[with_lxc=yes]) > AC_ARG_WITH([test], > @@ -714,6 +716,21 @@ AM_CONDITIONAL([HAVE_NUMACTL], [test "$with_numactl" != "no"]) > AC_SUBST([NUMACTL_CFLAGS]) > AC_SUBST([NUMACTL_LIBS]) > > +if test "$with_phyp" = "yes"; then > + AC_CHECK_LIB([ssh],[ssh_new],[ > + LIBSSH_LIBS="$LIBSSH_LIBS -lssh -L/usr/local/lib/" > + AC_SUBST([LIBSSH_LIBS])],[ > + AC_MSG_ERROR([You must install the libssh to compile Phype driver.])]) > + > + AC_CHECK_HEADERS([libssh/libssh.h],[ > + LIBSSH_CFLAGS="-I/usr/local/include/libssh" > + AC_SUBST([LIBSSH_CFLAGS])],[ > + AC_MSG_ERROR([Cannot find libssh headers.Is libssh installed ?])],[]) > + AC_DEFINE_UNQUOTED([WITH_PHYP], 1, > + [whether IBM HMC / IVM driver is enabled]) > +fi > +AM_CONDITIONAL([WITH_PHYP],[test "$with_phyp" = "yes"]) 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

> + > dnl virsh libraries > AC_CHECK_HEADERS([readline/readline.h]) > > @@ -1345,6 +1362,7 @@ AC_MSG_NOTICE([ QEMU: $with_qemu]) > AC_MSG_NOTICE([ UML: $with_uml]) > AC_MSG_NOTICE([ OpenVZ: $with_openvz]) > AC_MSG_NOTICE([ LXC: $with_lxc]) > +AC_MSG_NOTICE([ IBM HMC/IVM: $with_phyp]) > AC_MSG_NOTICE([ Test: $with_test]) > AC_MSG_NOTICE([ Remote: $with_remote]) > AC_MSG_NOTICE([ Network: $with_network]) 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.

> 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 > + * > + * Authors: > + * Eduardo Otubo <otubo at linux.vnet.ibm.com> > + * > + * This library is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Lesser General Public > + * License as published by the Free Software Foundation; either > + * version 2.1 of the License, or (at your option) any later version. > + * > + * This library is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * Lesser General Public License for more details. > + * > + * You should have received a copy of the GNU Lesser General Public > + * License along with this library; if not, write to the Free Software > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA > + */ > + > +#include <sys/types.h> > +#include <config.h> This should typically be done in the .c file, rather than the .h files.

> +static virDrvOpenStatus > +phypOpen(virConnectPtr conn, > + virConnectAuthPtr auth ATTRIBUTE_UNUSED, > + int flags ATTRIBUTE_UNUSED) > +{ > + if (!conn || !conn->uri) > + return VIR_DRV_OPEN_DECLINED; > + > + if (conn->uri->scheme == NULL || conn->uri->server == NULL) > + return VIR_DRV_OPEN_DECLINED; > + > + int ssh_auth = 0, exit_status = 0; > + char *banner; > + char *password; > + > + SSH_SESSION *session; > + SSH_OPTIONS *opt; 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.

> + > + session = ssh_new(); > + opt = ssh_options_new(); > + > + if (!conn->uri->port) > + conn->uri->port = 22; > + > + /*setting some ssh options */ > + ssh_options_set_host(opt, conn->uri->server); > + ssh_options_set_port(opt, conn->uri->port); > + ssh_options_set_username(opt, conn->uri->user); > + ssh_set_options(session, opt); > + > + /*starting ssh connection */ > + if (ssh_connect(session)) { > + fprintf(stderr, "Connection failed : %s\n", > + ssh_get_error(session)); > + ssh_disconnect(session); > + ssh_finalize(); > + exit_status = SSH_CONN_ERR; > + return VIR_DRV_OPEN_DECLINED; > + } > + > + /*trying to use pub key */ > + if ((ssh_auth = > + ssh_userauth_autopubkey(session, NULL)) == SSH_AUTH_ERROR) { > + fprintf(stderr, "Authenticating with pubkey: %s\n", > + ssh_get_error(session)); > + exit_status = SSH_AUTH_PUBKEY_ERR; > + return VIR_DRV_OPEN_DECLINED; > + } > + > + if ((banner = ssh_get_issue_banner(session))) { > + printf("%s\n", banner); > + VIR_FREE(banner); > + } > + > + if (ssh_auth != SSH_AUTH_SUCCESS) { > + password = getpass("Password: "); 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.

> + if (ssh_userauth_password(session, NULL, password) != > + SSH_AUTH_SUCCESS) { > + fprintf(stderr, "Authentication failed: %s\n", > + ssh_get_error(session)); > + ssh_disconnect(session); > + memset(password, 0, strlen(password)); > + exit_status = SSH_AUTH_ERR; > + return VIR_DRV_OPEN_DECLINED; > + } else > + goto exec; > + } else > + goto exec; > + > + exec: > + conn->privateData = session; > + return VIR_DRV_OPEN_SUCCESS; > +} > + > +static int > +phypClose(virConnectPtr conn) > +{ > + SSH_SESSION *ssh_session = conn->privateData; > + > + ssh_disconnect(ssh_session); > + > + return 0; > +} > + > +static int > +__inner_exec_command(SSH_SESSION * session, char *cmd, > + char *textual_return) > +{ > + CHANNEL *channel = channel_new(session); > + BUFFER *readbuf = buffer_new(); > + > + int exit_status = 0, temp_return = 0, buffer_size = 0; > + > + memset(textual_return, 0, sizeof(textual_return)); > + > + if (channel_open_session(channel) == SSH_ERROR) > + return SSH_CHANNEL_OPEN_ERR; > + > + if (channel_request_exec(channel, cmd) == SSH_ERROR) > + return SSH_CHANNEL_EXEC_ERR; > + > + while (channel && channel_is_open(channel)) { > + temp_return = channel_read(channel, readbuf, 0, 0); > + > + strncat(textual_return, buffer_get(readbuf), > + buffer_get_len(readbuf)); > + buffer_size += buffer_size + buffer_get_len(readbuf); > + > + if (temp_return == 0) { > + textual_return[buffer_size + 1] = 0; > + exit_status = channel_get_exit_status(channel); > + break; > + } > + } > + > + if (channel_send_eof(channel) == SSH_ERROR) > + return SSH_CHANNEL_SEND_EOF_ERR; > + > + if (channel_close(channel) == SSH_ERROR) > + return SSH_CHANNEL_CLOSE_ERR; > + > + return exit_status; > +} > + > +static int > +phypGetLparID(SSH_SESSION * ssh_session, const char *managed_system, > + const char *name) > +{ > + char id_c[10]; > + unsigned int i = 0; > + int ret = 0; > + char *cmd; > + char *textual_return; > + > + if (VIR_ALLOC_N(cmd, MAXSIZE+sizeof(name)+sizeof(managed_system))) { > + return PHYP_NO_MEM; > + } > + > + if (VIR_ALLOC_N(textual_return, MAXSIZE)) { > + return PHYP_NO_MEM; > + } > + > + memset(id_c, 0, sizeof(id_c)); > + > + sprintf(cmd, > + "lssyscfg -r lpar -m %s --filter lpar_names=%s -F lpar_id", > + managed_system, name); 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. > + ret = __inner_exec_command(ssh_session, cmd, textual_return); 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); > + ret = __inner_exec_command(ssh_session, cmd, textual_return); > + > + 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); > + ret = __inner_exec_command(ssh_session, cmd, textual_return); > + > + 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