[libvirt] (no subject)
Hello all, I've been working on a libvirt extension to manage IBM's Power VMs (LPARs). The Power systems are managed through management console referred to as the HMC or using a management partition (IVM). Both HMC and IVM runs an SSH, then you can reach it via command line, and an HTTP server, then you can reach it via web browser. 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. 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. Any comments are welcome. Thanks, -- Eduardo Otubo Software Engineer Linux Technology Center IBM Systems & Technology Group Mobile: +55 19 8135 0885 otubo@linux.vnet.ibm.com
Sorry all about my last email, the subject should be "[RFC] Power Hypervisor Libvirt support". There should be an error here. Thanks again, Em Sex, 2009-03-20 às 13:58 -0300, Eduardo Otubo escreveu:
Hello all,
I've been working on a libvirt extension to manage IBM's Power VMs (LPARs). The Power systems are managed through management console referred to as the HMC or using a management partition (IVM). Both HMC and IVM runs an SSH, then you can reach it via command line, and an HTTP server, then you can reach it via web browser.
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.
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.
Any comments are welcome.
Thanks,
-- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list -- Eduardo Otubo Software Engineer Linux Technology Center IBM Systems & Technology Group Mobile: +55 19 8135 0885 otubo@linux.vnet.ibm.com
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
Hello Kaitlin,
+#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?
I am still planning how I should store all driver information, so I think I should keep this for now.
+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.
Already fixed, but thanks anyway.
+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?
Good point. I should follow the patterns and start using virDomainObj to store name and uuid. But unfortunately ssh_session and the managed_system name I still need to keep them as I did.
+ + 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.
Yes, this problem will be solved when I change it to virDomainObj. Thanks again for the comments. []'s -- Eduardo Otubo Software Engineer Linux Technology Center IBM Systems & Technology Group Mobile: +55 19 8135 0885 otubo@linux.vnet.ibm.com
On Fri, Mar 20, 2009 at 01:58:50PM -0300, Eduardo Otubo wrote:
I've been working on a libvirt extension to manage IBM's Power VMs (LPARs). The Power systems are managed through management console referred to as the HMC or using a management partition (IVM). Both HMC and IVM runs an SSH, then you can reach it via command line, and an HTTP server, then you can reach it via web browser.
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
+ + sprintf(cmd, "lssyscfg -r lpar -m %s -F lpar_id", managed_system); + ret = __inner_exec_command(ssh_session, cmd, textual_return); + + 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); + ret = __inner_exec_command(ssh_session, cmd, textual_return); + + 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 :|
Hello Daniel,
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.
There is no other way of having this done. No xmlrpc or connections directly to the hypervisor. Executing commands over SSH is not beautiful, but is the only way we have.
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.
Yes, that's right. HMC, the Hardware Management Console, is a separate machine that maganes one or more servers with virtualized machines.
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.
Yes, the first model was already on my todo list and it's prettu fine to me.
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).
In fact I use libssh <http://0xbadc0de.be/wiki/libssh:libssh> and not libssh2. I choosed libssh instead of libssh2 for some reasons: libssh2 doesn't handle server side ssh or sshv1. And I am more used to work with its API then libssh2.
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
Yes, I agree with this. I'll implement this way and post it back.
+ 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.
Sure, in fact, I think IBM HMC/IVM is too high level. The final use of this feature does not need this details. Phyp is enough information we will need.
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.
Ok, this minor fixes will be at next version of this patch.
+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.
Ok, I am still getting used to your coding style. I read the HACKING file and followed all steps and details strictly, but some gone missing like this. Will fix and also will be in the next version.
+ + 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.
This improve for the authentication method is in my todo list too. I have in mind that virsh won't be the only application that will handle this, I may have many others. But thanks for the comment.
+ 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.
And this was a problem I was going to ask your comments. I really need to fix this. Thanks for the suggestion. This should be fine this way.
+ + 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
Understood, I'll check src/virterror_internal.h to see how I need to do things in the right way. I'll fix all those things and implement some more features I have in mind. I'll post the next patch soon. Thanks everyone for the comments. []'s -- Eduardo Otubo Software Engineer Linux Technology Center IBM Systems & Technology Group Mobile: +55 19 8135 0885 otubo@linux.vnet.ibm.com
On Fri, Mar 27, 2009 at 05:33:43PM -0300, Eduardo Otubo wrote:
@@ -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).
In fact I use libssh <http://0xbadc0de.be/wiki/libssh:libssh> and not libssh2. I choosed libssh instead of libssh2 for some reasons: libssh2 doesn't handle server side ssh or sshv1. And I am more used to work with its API then libssh2.
Hmm, I hadn't come across libssh before. To be honest I'm not really all that impressed with the code quality of libssh. There is a serious lack of basic error checking in system calls & libc calls they make, in particular no malloc() call is ever checked for failure. It is hardcoded to use IPv4 for sockets. It is not const-correct in its API usage, or its exposed public API. As such I don't think libssh is suitable for use in libvirt, and would rather this used libssh2. libssh2 also has the benefit that it has been ported to Win32 platform already. As for SSH v1 protocol support, this is a flawed protocol and should not be used in any apps or libraries anymore because it cannot be considered secure by modern standards. 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 :|
Hello Daniel, Look, I don't want to make big deal about this, I'll just answer these points below with some arguments they gave me. If we find out libssh2 is better, I' can change it, no problem at all.
In fact I use libssh <http://0xbadc0de.be/wiki/libssh:libssh> and not libssh2. I choosed libssh instead of libssh2 for some reasons: libssh2 doesn't handle server side ssh or sshv1. And I am more used to work with its API then libssh2.
Hmm, I hadn't come across libssh before. To be honest I'm not really all that impressed with the code quality of libssh. There is a serious lack of basic error checking in system calls & libc calls they make, in particular no malloc() call is ever checked for failure. It is
Yes, malloc() error checking is a known issue and they're already working on it.
hardcoded to use IPv4 for sockets.
They told me they did some tests with IPv6 and worked perfectly.
It is not const-correct in its API usage, or its exposed public API.
Yes, that's a point. But I think it's purely coding style.
As such I don't think libssh is suitable for use in libvirt, and would rather this used libssh2. libssh2 also has the benefit that it has been ported to Win32 platform already.
As for SSH v1 protocol support, this is a flawed protocol and should not be used in any apps or libraries anymore because it cannot be considered secure by modern standards.
I see no problem on changing my code to libssh2, I may change it and send another patch/rfc. Regards, -- Eduardo Otubo Software Engineer Linux Technology Center IBM Systems & Technology Group Mobile: +55 19 8135 0885 otubo@linux.vnet.ibm.com
Hello Daniel, I've been talking to libssh guys and they did a lot of changes trying to be more compliant. What do you think about reconsidering the use of libssh instead of libssh2?
Hmm, I hadn't come across libssh before. To be honest I'm not really all that impressed with the code quality of libssh. There is a serious lack of basic error checking in system calls & libc calls they make, in particular no malloc() call is ever checked for failure.
It's fixed in svn trunk now.
It is hardcoded to use IPv4 for sockets.
This issue is solved since at least one year ago.
It is not const-correct in its API usage, or its exposed public API.
This is being fixed and it's almost done.
As such I don't think libssh is suitable for use in libvirt, and would rather this used libssh2. libssh2 also has the benefit that it has been ported to Win32 platform already.
Libssh is ported to win32 and works very fine.
As for SSH v1 protocol support, this is a flawed protocol and should not be used in any apps or libraries anymore because it cannot be considered secure by modern standards.
In their words "can't agree more with him. SSH1 support was made initialy to support old cisco routers". All of these issues will be available in the libssh-0.3, which will be released in the end of this week, 10/april. Do you think after all these changes libssh will be compliant to be used in libvirt? What do you think? []'s -- Eduardo Otubo Software Engineer Linux Technology Center IBM Systems & Technology Group Mobile: +55 19 8135 0885 otubo@linux.vnet.ibm.com
On Mon, Apr 06, 2009 at 03:22:36PM -0300, Eduardo Otubo wrote:
Hello Daniel,
I've been talking to libssh guys and they did a lot of changes trying to be more compliant. What do you think about reconsidering the use of libssh instead of libssh2?
I've taken a look at their current SVN repo, and it looks very much improved in quality, so I've no objections to its use now. 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 :|
Hello again, I very am glad with all the feedback I got. The following attachtment is the result of the work using all the suggestions the list made. I also have a few comments over this new RFC. Here is a little Changelog: * Now the URI is recognized as phyp://user@hmc/managed_system. To use path component of the uri struct, I had to implement a little function to strip out the first slash of the string, giving me only 'path', instead of '/path'. * I implemented the option --with-libssh and --with-phyp right according to what Daniel Berrange suggested. * Now all the variable-size string buffers are manipulated with the internal API virBuffer* to make it safer. | +- This made me think and change the __inner_exec_command function. Now it returns char * with all the textual return, and I am passing int * as reference to get the exit status - all the information I need about the remote executed command. | +- And this also made me write a little auxiliar function to strip out the '\n' from the textual return. Known and unsolved issues: * Authentication - need to improve the authenticantion as Daniel Berrange said. Still in progress. * Error codes. * Logging and debugging messages. Any comments are welcome, []'s Em Sex, 2009-03-20 às 13:58 -0300, Eduardo Otubo escreveu:
Hello all,
I've been working on a libvirt extension to manage IBM's Power VMs (LPARs). The Power systems are managed through management console referred to as the HMC or using a management partition (IVM). Both HMC and IVM runs an SSH, then you can reach it via command line, and an HTTP server, then you can reach it via web browser.
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.
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.
Any comments are welcome.
Thanks,
-- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list -- Eduardo Otubo Software Engineer Linux Technology Center IBM Systems & Technology Group Mobile: +55 19 8135 0885 otubo@linux.vnet.ibm.com
On Thu, Apr 23, 2009 at 06:44:13PM -0300, Eduardo Otubo wrote:
I very am glad with all the feedback I got. The following attachtment is the result of the work using all the suggestions the list made. I also have a few comments over this new RFC. Here is a little Changelog:
[...]
--- a/include/libvirt/virterror.h +++ b/include/libvirt/virterror.h @@ -61,6 +61,7 @@ typedef enum { VIR_FROM_UML, /* Error at the UML driver */ VIR_FROM_NODEDEV, /* Error from node device monitor */ VIR_FROM_XEN_INOTIFY, /* Error from xen inotify layer */ + VIR_FROM_PHYP, /* Error from IBM power hypervisor */ VIR_FROM_SECURITY, /* Error from security framework */ VIR_FROM_VBOX, /* Error from VirtualBox driver */ } virErrorDomain;
that you can't do, you must add at the end of the enum to preserve the values for previous items in the enum. [...]
+PHYP_DRIVER_SOURCES = \ + phyp_driver.c phyp_driver.h +
Hum, maybe we should do like for vbox driver, use a subdir to limit the amount of files in the src/ directory, so I suggest using phyp/phyp_driver.c and phyp/phyp_driver.h I guess copying what was done for vbox/ files would be fine.
diff --git a/src/libvirt.c b/src/libvirt.c index 95a861e..617d957 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -55,6 +55,9 @@ #ifdef WITH_OPENVZ #include "openvz_driver.h" #endif +#ifdef WITH_PHYP +#include "phyp_driver.h" +#endif
phyp/phyp_driver.h
#ifdef WITH_VBOX #include "vbox/vbox_driver.h" #endif [...] +#if WITH_PHYP + if (STRCASEEQ(type, "Phyp"))
we should probably use lower case there but it's equivalent.
--- /dev/null +++ b/src/phyp_driver.c @@ -0,0 +1,496 @@ + +/* + * 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 + */
okay [...]
+/* this functions is the layer that manipulates the ssh channel itself + * and executes the commands on the remote machine */ +static char * +__inner_exec_command(SSH_SESSION * session, char *cmd, int *exit_status) +{ + CHANNEL *channel = channel_new(session); + BUFFER *readbuf = buffer_new(); + virBuffer tex_ret = VIR_BUFFER_INITIALIZER; + + int ret = 0; + + if (channel_open_session(channel) == SSH_ERROR) + (*exit_status) = SSH_CHANNEL_OPEN_ERR; + + if (channel_request_exec(channel, cmd) == SSH_ERROR) + (*exit_status) = SSH_CHANNEL_EXEC_ERR; + + if (channel_send_eof(channel) == SSH_ERROR) + (*exit_status) = SSH_CHANNEL_SEND_EOF_ERR;
That looks a bit weird, I think in those 3 cases I would goto an error: label directly and return the error value.
+ while (channel && channel_is_open(channel)) { + ret = channel_read(channel, readbuf, 0, 0); + if (ret < 0) { + (*exit_status) = SSH_CHANNEL_READ_ERR; + break; + } + + if (ret == 0) { + channel_send_eof(channel); + while(channel_get_exit_status(channel) == -1){ + channel_poll(channel,0); + usleep(50); + }
It's always a bit nasty to sleep like that. Is there really now way to use something like poll or select instead ? In average you're gonna be waken up multiple time while waiting for your answer while the kernel could instead wake you up exactly when you have the data.
+ if (channel_close(channel) == SSH_ERROR) + (*exit_status) = SSH_CHANNEL_CLOSE_ERR; + + (*exit_status) = channel_get_exit_status(channel); + + channel_free(channel); + channel = NULL; + break; + } + + buffer_add_u8(readbuf, 0); + virBufferStrcat(&tex_ret, buffer_get(readbuf)); + } [...]
+ +/* return the lpar_id given a name and a managed system name */ +static int +phypGetLparID(SSH_SESSION * ssh_session, const char *managed_system, + const char *name) +{ + int exit_status = 0; + virBuffer cmd = VIR_BUFFER_INITIALIZER; + + virBufferVSprintf(&cmd, + "lssyscfg -r lpar -m %s --filter lpar_names=%s -F lpar_id", + managed_system, name); + const char *tex_ret = + __inner_exec_command(ssh_session, virBufferContentAndReset(&cmd), + &exit_status); + + virBufferContentAndReset(&cmd);
Huh ? you're supposed to get the resulting char *, and then free it later once you're done with the data. Here youre just leaking memory I'm afraid same thing for most of the commands in that file.
+static int +phypNumDomains(virConnectPtr conn) +{ + int exit_status = 0; + char managed_system[strlen(conn->uri->path) - 1]; + SSH_SESSION *ssh_session = conn->privateData; + virBuffer cmd = VIR_BUFFER_INITIALIZER; + + stripPath((char *) &managed_system, conn->uri->path); + virBufferVSprintf(&cmd, + "lssyscfg -r lpar -m %s -F lpar_id|grep -c ^[0-9]*", + managed_system); + const char *ndom = + __inner_exec_command(ssh_session, virBufferContentAndReset(&cmd), + &exit_status);
This can't be a const char *really or you're using a temporary buffer I'm afraid you're loosing memory here
+ virBufferContentAndReset(&cmd); + if(exit_status < 0 || ndom == NULL) + return 0;
and if you free that memeory you would just use a freed string here,
+ return atoi(ndom); +}
I guess you really need to clean up how you build those routines, make clear error path and do more anylysis of the allocations. I would also suggest to use the virStrToLong_i routine instead of calling atoi() directly. [...]
diff --git a/src/phyp_driver.h b/src/phyp_driver.h new file mode 100644 index 0000000..c2d9c9b --- /dev/null +++ b/src/phyp_driver.h @@ -0,0 +1,21 @@ +#include <config.h> +#include <libssh/libssh.h> + +#define MAXSIZE 65536 +#define LPAR_EXEC_ERR -1 +#define SSH_CONN_ERR -2 +#define SSH_AUTH_PUBKEY_ERR -3 +#define SSH_AUTH_ERR -4 +#define SSH_CHANNEL_OPEN_ERR -5 +#define SSH_CHANNEL_EXEC_ERR -6 +#define SSH_CHANNEL_SEND_EOF_ERR -7 +#define SSH_CHANNEL_CLOSE_ERR -8 +#define SSH_CHANNEL_READ_ERR -9 +#define PHYP_NO_MEM -10
I would at least comment those constants. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
Hello DV, I agree with your suggestions and everything is fixed now. But I would like to discuss some points.
+ while (channel && channel_is_open(channel)) { + ret = channel_read(channel, readbuf, 0, 0); + if (ret < 0) { + (*exit_status) = SSH_CHANNEL_READ_ERR; + break; + } + + if (ret == 0) { + channel_send_eof(channel); + while(channel_get_exit_status(channel) == -1){ + channel_poll(channel,0); + usleep(50); + }
It's always a bit nasty to sleep like that. Is there really now way to use something like poll or select instead ? In average you're gonna be waken up multiple time while waiting for your answer while the kernel could instead wake you up exactly when you have the data.
This was a libssh issue This was the only way I could make it work. But now the libssh is fixed and this function is a little different - http://pastebin.com/fbf84426
+ +/* return the lpar_id given a name and a managed system name */ +static int +phypGetLparID(SSH_SESSION * ssh_session, const char *managed_system, + const char *name) +{ + int exit_status = 0; + virBuffer cmd = VIR_BUFFER_INITIALIZER; + + virBufferVSprintf(&cmd, + "lssyscfg -r lpar -m %s --filter lpar_names=%s -F lpar_id", + managed_system, name); + const char *tex_ret = + __inner_exec_command(ssh_session, virBufferContentAndReset(&cmd), + &exit_status); + + virBufferContentAndReset(&cmd);
Huh ? you're supposed to get the resulting char *, and then free it later once you're done with the data. Here youre just leaking memory I'm afraid
same thing for most of the commands in that file.
Here, I just would like to free the Buffer, and this was the best way I find since I couldn't find any better function to manipulate this. How do I simply free a buffer using the internal virBuffer* API? Thanks for the help, -- Eduardo Otubo Software Engineer Linux Technology Center IBM Systems & Technology Group Mobile: +55 19 8135 0885 otubo@linux.vnet.ibm.com
On Mon, May 04, 2009 at 05:50:03PM -0300, Eduardo Otubo wrote:
Hello DV,
Hi Eduardo, sorry for the delay I took a few days of vacations :-)
I agree with your suggestions and everything is fixed now. But I would like to discuss some points.
good !
+ while (channel && channel_is_open(channel)) { + ret = channel_read(channel, readbuf, 0, 0); + if (ret < 0) { + (*exit_status) = SSH_CHANNEL_READ_ERR; + break; + } + + if (ret == 0) { + channel_send_eof(channel); + while(channel_get_exit_status(channel) == -1){ + channel_poll(channel,0); + usleep(50); + }
It's always a bit nasty to sleep like that. Is there really now way to use something like poll or select instead ? In average you're gonna be waken up multiple time while waiting for your answer while the kernel could instead wake you up exactly when you have the data.
This was a libssh issue This was the only way I could make it work. But now the libssh is fixed and this function is a little different - http://pastebin.com/fbf84426
Looks fine now, but how can you make sure libssh used will have the fix, is there a new version released and the associated test in configure.in ?
+ +/* return the lpar_id given a name and a managed system name */ +static int +phypGetLparID(SSH_SESSION * ssh_session, const char *managed_system, + const char *name) +{ + int exit_status = 0; + virBuffer cmd = VIR_BUFFER_INITIALIZER; + + virBufferVSprintf(&cmd, + "lssyscfg -r lpar -m %s --filter lpar_names=%s -F lpar_id", + managed_system, name); + const char *tex_ret = + __inner_exec_command(ssh_session, virBufferContentAndReset(&cmd), + &exit_status); + + virBufferContentAndReset(&cmd);
Huh ? you're supposed to get the resulting char *, and then free it later once you're done with the data. Here youre just leaking memory I'm afraid
same thing for most of the commands in that file.
Here, I just would like to free the Buffer, and this was the best way I find since I couldn't find any better function to manipulate this. How do I simply free a buffer using the internal virBuffer* API?
Well you didn't allocate the buffer structure since it's on the heap and initilized with the VIR_BUFFER_INITIALIZER macro. So technically you don't need to free it, but you need to free the string in the buffer. That string is returned by virBufferContentAndReset(), so you need to save that to a local variable and free it once done. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
Em Qua, 2009-05-06 às 09:26 +0200, Daniel Veillard escreveu:
On Mon, May 04, 2009 at 05:50:03PM -0300, Eduardo Otubo wrote:
Hello DV,
Hi Eduardo,
sorry for the delay I took a few days of vacations :-)
Hello DV, Nice! Hope you enjoyed your vacations :)
+ while (channel && channel_is_open(channel)) { + ret = channel_read(channel, readbuf, 0, 0); + if (ret < 0) { + (*exit_status) = SSH_CHANNEL_READ_ERR; + break; + } + + if (ret == 0) { + channel_send_eof(channel); + while(channel_get_exit_status(channel) == -1){ + channel_poll(channel,0); + usleep(50); + }
It's always a bit nasty to sleep like that. Is there really now way to use something like poll or select instead ? In average you're gonna be waken up multiple time while waiting for your answer while the kernel could instead wake you up exactly when you have the data.
This was a libssh issue This was the only way I could make it work. But now the libssh is fixed and this function is a little different - http://pastebin.com/fbf84426
Looks fine now, but how can you make sure libssh used will have the fix, is there a new version released and the associated test in configure.in ?
In fact I am part of the libssh team, I help them with testing. And yes, we're planning to release a new version of libssh this week with all these fixes. But I still need to make it sure in configure.in. I'll do it right now.
+ +/* return the lpar_id given a name and a managed system name */ +static int +phypGetLparID(SSH_SESSION * ssh_session, const char *managed_system, + const char *name) +{ + int exit_status = 0; + virBuffer cmd = VIR_BUFFER_INITIALIZER; + + virBufferVSprintf(&cmd, + "lssyscfg -r lpar -m %s --filter lpar_names=%s -F lpar_id", + managed_system, name); + const char *tex_ret = + __inner_exec_command(ssh_session, virBufferContentAndReset(&cmd), + &exit_status); + + virBufferContentAndReset(&cmd);
Huh ? you're supposed to get the resulting char *, and then free it later once you're done with the data. Here youre just leaking memory I'm afraid
same thing for most of the commands in that file.
Here, I just would like to free the Buffer, and this was the best way I find since I couldn't find any better function to manipulate this. How do I simply free a buffer using the internal virBuffer* API?
Well you didn't allocate the buffer structure since it's on the heap and initilized with the VIR_BUFFER_INITIALIZER macro. So technically you don't need to free it, but you need to free the string in the buffer. That string is returned by virBufferContentAndReset(), so you need to save that to a local variable and free it once done.
All right, got it. Thanks for the tips :) []'s -- Eduardo Otubo Software Engineer Linux Technology Center IBM Systems & Technology Group Mobile: +55 19 8135 0885 otubo@linux.vnet.ibm.com
On Mon, May 04, 2009 at 05:50:03PM -0300, Eduardo Otubo wrote:
+ +/* return the lpar_id given a name and a managed system name */ +static int +phypGetLparID(SSH_SESSION * ssh_session, const char *managed_system, + const char *name) +{ + int exit_status = 0; + virBuffer cmd = VIR_BUFFER_INITIALIZER; + + virBufferVSprintf(&cmd, + "lssyscfg -r lpar -m %s --filter lpar_names=%s -F lpar_id", + managed_system, name); + const char *tex_ret = + __inner_exec_command(ssh_session, virBufferContentAndReset(&cmd), + &exit_status); + + virBufferContentAndReset(&cmd);
Huh ? you're supposed to get the resulting char *, and then free it later once you're done with the data. Here youre just leaking memory I'm afraid
same thing for most of the commands in that file.
Here, I just would like to free the Buffer, and this was the best way I find since I couldn't find any better function to manipulate this. How do I simply free a buffer using the internal virBuffer* API?
The virBufferContentAndReset() method returns you the internal char * string, and resets the virBuffer state to its inital value. You are now owner of the char * string, and are responsible for free'ing it when done. You should also check virBufferError() and report OOM error if it fails. So, in the above example. what you'd want todo is static int phypGetLparID(SSH_SESSION * ssh_session, const char *managed_system, const char *name) { int exit_status = 0; virBuffer cmd = VIR_BUFFER_INITIALIZER; char *buf; virBufferVSprintf(&cmd, "lssyscfg -r lpar -m %s --filter lpar_names=%s -F lpar_id", managed_system, name); if (virBufferError(&cmd)) { virReportOOMError(conn); return NULL; } buf = virBufferContentAndReset(&cmd); const char *tex_ret = __inner_exec_command(ssh_session, buf &exit_status); VIR_FREE(buf); } That all said, in this particular function I it is overkill to use the virBuffer APIs, since you've only got a single printf() call to make. virBuffer is more appropriate when you have 2 or more printfs() or strcats() to make. If just doing a single printf, then use virAsprintf, eg static int phypGetLparID(SSH_SESSION * ssh_session, const char *managed_system, const char *name) { int exit_status = 0; char *buf; if (virAsprintf(&buf, "lssyscfg -r lpar -m %s --filter lpar_names=%s -F lpar_id", managed_system, name) < 0) { virReportOOMError(conn); return NULL; } const char *tex_ret = __inner_exec_command(ssh_session, buf &exit_status); VIR_FREE(buf); } 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 :|
Em Qua, 2009-05-06 às 09:44 +0100, Daniel P. Berrange escreveu:
On Mon, May 04, 2009 at 05:50:03PM -0300, Eduardo Otubo wrote:
+ +/* return the lpar_id given a name and a managed system name */ +static int +phypGetLparID(SSH_SESSION * ssh_session, const char *managed_system, + const char *name) +{ + int exit_status = 0; + virBuffer cmd = VIR_BUFFER_INITIALIZER; + + virBufferVSprintf(&cmd, + "lssyscfg -r lpar -m %s --filter lpar_names=%s -F lpar_id", + managed_system, name); + const char *tex_ret = + __inner_exec_command(ssh_session, virBufferContentAndReset(&cmd), + &exit_status); + + virBufferContentAndReset(&cmd);
Huh ? you're supposed to get the resulting char *, and then free it later once you're done with the data. Here youre just leaking memory I'm afraid
same thing for most of the commands in that file.
Here, I just would like to free the Buffer, and this was the best way I find since I couldn't find any better function to manipulate this. How do I simply free a buffer using the internal virBuffer* API?
The virBufferContentAndReset() method returns you the internal char * string, and resets the virBuffer state to its inital value. You are now owner of the char * string, and are responsible for free'ing it when done.
You should also check virBufferError() and report OOM error if it fails. So, in the above example. what you'd want todo is
static int phypGetLparID(SSH_SESSION * ssh_session, const char *managed_system, const char *name) { int exit_status = 0; virBuffer cmd = VIR_BUFFER_INITIALIZER; char *buf;
virBufferVSprintf(&cmd, "lssyscfg -r lpar -m %s --filter lpar_names=%s -F lpar_id", managed_system, name); if (virBufferError(&cmd)) { virReportOOMError(conn); return NULL; }
buf = virBufferContentAndReset(&cmd); const char *tex_ret = __inner_exec_command(ssh_session, buf &exit_status); VIR_FREE(buf); }
That all said, in this particular function I it is overkill to use the virBuffer APIs, since you've only got a single printf() call to make. virBuffer is more appropriate when you have 2 or more printfs() or strcats() to make. If just doing a single printf, then use virAsprintf,
eg
static int phypGetLparID(SSH_SESSION * ssh_session, const char *managed_system, const char *name) { int exit_status = 0; char *buf;
if (virAsprintf(&buf, "lssyscfg -r lpar -m %s --filter lpar_names=%s -F lpar_id", managed_system, name) < 0) { virReportOOMError(conn); return NULL; }
const char *tex_ret = __inner_exec_command(ssh_session, buf &exit_status); VIR_FREE(buf); }
Regards, Daniel
DV and danpb, First of all, thanks for the tips. Is helping me a lot. Here is the phyp_driver.c with the memory leaks fixed with your suggestions. With those things done, do you think this code is enough and compliant to libvirt patterns to be included in the next libvirt release? The only feature we have until now is just to list the LPARs ("Logical PARtitions", the IBM virtual machines for Power). Once this code is safe and goot enough, the implementations of new commands will be much faster and easier. Here is the code: /* * 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 <config.h> #include <sys/types.h> #include <limits.h> #include <string.h> #include <strings.h> #include <stdio.h> #include <stdarg.h> #include <stdlib.h> #include <unistd.h> #include <errno.h> #include <stdio.h> #include <libssh/libssh.h> #include "internal.h" #include "util.h" #include "datatypes.h" #include "buf.h" #include "memory.h" #include "logging.h" #include "driver.h" #include "libvirt/libvirt.h" #include "virterror_internal.h" #include "phyp_driver.h" #define VIR_FROM_THIS VIR_FROM_PHYP /* * TODO: I still need to implement a way to distinguish * an HMC from an IVM * */ static virDrvOpenStatus phypOpen(virConnectPtr conn, virConnectAuthPtr auth, int flags ATTRIBUTE_UNUSED) { int ssh_auth = 0, exit_status = 0; char *banner; SSH_SESSION *session; SSH_OPTIONS *opt; if (!conn || !conn->uri) return VIR_DRV_OPEN_DECLINED; if (conn->uri->scheme == NULL || conn->uri->server == NULL || conn->uri->path == NULL) return VIR_DRV_OPEN_DECLINED; 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)) { virRaiseError(conn, NULL, NULL, 0, VIR_FROM_PHYP, VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, "%s", _("connection failed")); ssh_disconnect(session); ssh_finalize(); } /*trying to use pub key */ if ((ssh_auth = ssh_userauth_autopubkey(session, NULL)) == SSH_AUTH_ERROR) { virRaiseError(conn, NULL, NULL, 0, VIR_FROM_PHYP, VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, "%s : %s", _("authenticating with public key ailed"), ssh_get_error(session)); } if ((banner = ssh_get_issue_banner(session))) { virRaiseError(conn, NULL, NULL, 0, VIR_FROM_PHYP, VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, "%s", banner); VIR_FREE(banner); } if (ssh_auth != SSH_AUTH_SUCCESS) { int i; int hasPassphrase = 0; virConnectCredential creds[] = { {VIR_CRED_PASSPHRASE, "password", "Password", NULL, NULL, 0}, }; if (!auth || !auth->cb) { virRaiseError(conn, NULL, NULL, 0, VIR_FROM_PHYP, VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, "%s", _("no authentication callback provided")); goto err; } for (i = 0; i < auth->ncredtype; i++) { if (auth->credtype[i] == VIR_CRED_PASSPHRASE) hasPassphrase = 1; } if (!hasPassphrase) { virRaiseError(conn, NULL, NULL, 0, VIR_FROM_PHYP, VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, "%s", _("required credentials are not supported")); goto err; } int res = (auth->cb) (creds, ARRAY_CARDINALITY(creds), auth->cbdata); if (res < 0) { virRaiseError(conn, NULL, NULL, 0, VIR_FROM_PHYP, VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, "%s", _("unable to fetch credentials")); goto err; } char *password = creds[0].result; char *username = conn->uri->user; if (ssh_userauth_password(session, username, password) != SSH_AUTH_SUCCESS) { virRaiseError(conn, NULL, NULL, 0, VIR_FROM_PHYP, VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, "%s : % s", "authentication failed", ssh_get_error(session)); ssh_disconnect(session); memset(password, 0, strlen(password)); memset(username, 0, strlen(username)); goto err; } else goto exec; } else goto exec; err: exit_status = SSH_CONN_ERR; return VIR_DRV_OPEN_DECLINED; 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; } /* this functions is the layer that manipulates the ssh channel itself * and executes the commands on the remote machine */ static char * __inner_exec_command(SSH_SESSION * session, char *cmd, int *exit_status) { CHANNEL *channel = channel_new(session); char buf[4096] = { 0 }; virBuffer tex_ret = VIR_BUFFER_INITIALIZER; int ret = 0; if (channel_open_session(channel) == SSH_ERROR) goto err; if (channel_request_exec(channel, cmd) == SSH_ERROR) goto err; if (channel_send_eof(channel) == SSH_ERROR) goto err; while (channel && channel_is_open(channel)) { ret = channel_read(channel, buf, sizeof(buf), 0); if (ret < 0) goto err; if (ret == 0) { channel_send_eof(channel); if (channel_get_exit_status(channel) == -1) goto err; if (channel_close(channel) == SSH_ERROR) goto err; channel_free(channel); channel = NULL; goto exit; } virBufferAdd(&tex_ret, (const char *) &buf, sizeof(buf)); } err: (*exit_status) = SSH_CMD_ERR; return NULL; exit: return virBufferContentAndReset(&tex_ret); } /* return the lpar_id given a name and a managed system name */ static int phypGetLparID(SSH_SESSION * ssh_session, const char *managed_system, const char *name) { int exit_status = 0; int lpar_id = 0; char *char_ptr; char *cmd; if (virAsprintf(&cmd, "lssyscfg -r lpar -m %s --filter lpar_names=%s -F lpar_id", managed_system, name) < 0) return 0; const char *tex_ret = __inner_exec_command(ssh_session, cmd, &exit_status); VIR_FREE(cmd); if (exit_status < 0 || tex_ret == NULL) return 0; if (virStrToLong_i(tex_ret, &char_ptr, 10, &lpar_id) == -1) return 0; return lpar_id; } /* return the lpar name given a lpar_id and a managed system name */ static char * phypGetLparNAME(SSH_SESSION * ssh_session, const char *managed_system, unsigned int lpar_id, int *exit_status) { char *cmd; if (virAsprintf(&cmd, "lssyscfg -r lpar -m %s --filter lpar_ids=%d -F name", managed_system, lpar_id) < 0) return NULL; char *lpar_name = __inner_exec_command(ssh_session, cmd, (int *) exit_status); char *striped_lpar_name = (char *) malloc(sizeof(lpar_name) - 1); stripNewline(striped_lpar_name, lpar_name); VIR_FREE(cmd); if ((*exit_status) < 0 || lpar_name == NULL) return NULL; return striped_lpar_name; } /* return the lpar_uuid (which for now is its logical serial number) * given a lpar id and a managed system name */ static unsigned char * phypGetLparUUID(SSH_SESSION * ssh_session, const char *managed_system, unsigned int lpar_id, int *exit_status) { char *cmd; if (virAsprintf(&cmd, "lssyscfg -r lpar -m %s --filter lpar_ids=%d -F logical_serial_num", managed_system, lpar_id) < 0) return NULL; unsigned char *lpar_uuid = (unsigned char *) __inner_exec_command(ssh_session, cmd, (int *) exit_status); unsigned char *striped_lpar_uuid = (unsigned char *) malloc(sizeof(lpar_uuid) - 1); stripNewline((char *) striped_lpar_uuid, (char *) lpar_uuid); VIR_FREE(cmd); if ((*exit_status) < 0 || lpar_uuid == NULL) return NULL; return striped_lpar_uuid; } static int phypNumDomains(virConnectPtr conn) { int exit_status = 0; int ndom = 0; char *char_ptr; char *cmd; char managed_system[strlen(conn->uri->path) - 1]; SSH_SESSION *ssh_session = conn->privateData; stripPath((char *) &managed_system, conn->uri->path); if (virAsprintf(&cmd, "lssyscfg -r lpar -m %s -F lpar_id|grep -c ^[0-9]*", managed_system) < 0) { virReportOOMError(conn); return 0; } char *ret = __inner_exec_command(ssh_session, cmd, &exit_status); VIR_FREE(cmd); if (exit_status < 0 || ret == NULL) return 0; if (virStrToLong_i(ret, &char_ptr, 10, &ndom) == -1) return 0; return ndom; } static int phypListDomains(virConnectPtr conn, int *ids, int nids) { int exit_status = 0; int got = 0; char *char_ptr; unsigned int i = 0, j = 0; char id_c[10]; char *cmd; char managed_system[strlen(conn->uri->path) - 1]; SSH_SESSION *ssh_session = conn->privateData; stripPath((char *) &managed_system, conn->uri->path); memset(id_c, 0, 10); if (virAsprintf(&cmd, "lssyscfg -r lpar -m %s -F lpar_id", managed_system) < 0) { virReportOOMError(conn); return 0; } char *domains = __inner_exec_command(ssh_session, cmd, &exit_status); /* I need to parse the textual return in order to get the domains */ if (exit_status < 0 || domains == NULL) return 0; else { while (got < nids) { if (domains[i] == '\n') { if (virStrToLong_i(id_c, &char_ptr, 10, &ids[got]) == -1) return 0; memset(id_c, 0, 10); j = 0; got++; } else { id_c[j] = domains[i]; j++; } i++; } } VIR_FREE(cmd); return got; } static virDomainPtr phypDomainLookupByName(virConnectPtr conn, const char *name) { SSH_SESSION *ssh_session = conn->privateData; virDomainPtr dom = NULL; int lpar_id = 0; int exit_status = 0; char managed_system[strlen(conn->uri->path) - 1]; stripPath((char *) &managed_system, conn->uri->path); lpar_id = phypGetLparID(ssh_session, managed_system, name); if (lpar_id == PHYP_NO_MEM) return NULL; unsigned char *lpar_uuid = phypGetLparUUID(ssh_session, managed_system, lpar_id, &exit_status); if (exit_status == PHYP_NO_MEM || lpar_uuid == NULL) 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; int exit_status = 0; char managed_system[strlen(conn->uri->path) - 1]; stripPath((char *) &managed_system, conn->uri->path); char *lpar_name = phypGetLparNAME(ssh_session, managed_system, lpar_id, &exit_status); if (exit_status == PHYP_NO_MEM) return NULL; unsigned char *lpar_uuid = phypGetLparUUID(ssh_session, managed_system, lpar_id, &exit_status); if (exit_status == 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; } static virDriver phypDriver = { VIR_DRV_PHYP, "PHYP", phypOpen, /* open */ phypClose, /* close */ NULL, /* supports_feature */ NULL, /* type */ NULL, /* version */ NULL, /* getHostname */ NULL, /* getMaxVcpus */ NULL, /* nodeGetInfo */ NULL, /* getCapabilities */ phypListDomains, /* listDomains */ phypNumDomains, /* numOfDomains */ NULL, /* domainCreateXML */ phypDomainLookupByID, /* domainLookupByID */ NULL, /* domainLookupByUUID */ phypDomainLookupByName, /* domainLookupByName */ NULL, /* domainSuspend */ NULL, /* domainResume */ NULL, /* domainShutdown */ NULL, /* domainReboot */ NULL, /* domainDestroy */ NULL, /* domainGetOSType */ NULL, /* domainGetMaxMemory */ NULL, /* domainSetMaxMemory */ NULL, /* domainSetMemory */ NULL, /* domainGetInfo */ NULL, /* domainSave */ NULL, /* domainRestore */ NULL, /* domainCoreDump */ NULL, /* domainSetVcpus */ NULL, /* domainPinVcpu */ NULL, /* domainGetVcpus */ NULL, /* domainGetMaxVcpus */ NULL, /* domainGetSecurityLabel */ NULL, /* nodeGetSecurityModel */ NULL, /* domainDumpXML */ NULL, /* listDefinedDomains */ NULL, /* numOfDefinedDomains */ NULL, /* domainCreate */ NULL, /* domainDefineXML */ NULL, /* domainUndefine */ NULL, /* domainAttachDevice */ NULL, /* domainDetachDevice */ NULL, /* domainGetAutostart */ NULL, /* domainSetAutostart */ NULL, /* domainGetSchedulerType */ NULL, /* domainGetSchedulerParameters */ NULL, /* domainSetSchedulerParameters */ NULL, /* domainMigratePrepare */ NULL, /* domainMigratePerform */ NULL, /* domainMigrateFinish */ NULL, /* domainBlockStats */ NULL, /* domainInterfaceStats */ NULL, /* domainBlockPeek */ NULL, /* domainMemoryPeek */ NULL, /* nodeGetCellsFreeMemory */ NULL, /* getFreeMemory */ NULL, /* domainEventRegister */ NULL, /* domainEventDeregister */ NULL, /* domainMigratePrepare2 */ NULL, /* domainMigrateFinish2 */ NULL, /* nodeDeviceDettach */ NULL, /* nodeDeviceReAttach */ NULL, /* nodeDeviceReset */ }; int phypRegister(void) { virRegisterDriver(&phypDriver); return 0; } /* function that helps me to strip out the first slash from the * uri: phyp://user@hmc/path * * '/path' becomes 'path' * */ void stripPath(char *striped_path, char *path) { unsigned int i = 0; for (i = 1; i <= strlen(path); i++) striped_path[i - 1] = path[i]; striped_path[i] = '\0'; return; } /* function to strip out the '\n' of the end of some string */ void stripNewline(char *striped_string, char *string) { unsigned int i = 0; for (i = 0; i <= strlen(string); i++) if (string[i] != '\n') striped_string[i] = string[i]; striped_string[strlen(string) - 1] = '\0'; return; } Best regards, []'s -- Eduardo Otubo Software Engineer Linux Technology Center IBM Systems & Technology Group Mobile: +55 19 8135 0885 otubo@linux.vnet.ibm.com
On Fri, May 08, 2009 at 07:10:02PM -0300, Eduardo Otubo wrote:
Here is the phyp_driver.c with the memory leaks fixed with your suggestions. With those things done, do you think this code is enough and compliant to libvirt patterns to be included in the next libvirt release?
Yep, the virBuffer/virAsprintf usage looks good now.
The only feature we have until now is just to list the LPARs ("Logical PARtitions", the IBM virtual machines for Power). Once this code is safe and goot enough, the implementations of new commands will be much faster and easier.
I think for a initial commit of the driver I'd like to see it able to generate an XML description for each guest domain on the system. And also implement the listing of inactive domains. So these three extra driver API points:
NULL, /* domainDumpXML */ NULL, /* listDefinedDomains */ NULL, /* numOfDefinedDomains */
This would be enough to make the 2 core virsh commands work fully with the drier virsh list --all virsh dumpxml GUEST
static virDrvOpenStatus phypOpen(virConnectPtr conn, virConnectAuthPtr auth, int flags ATTRIBUTE_UNUSED) { int ssh_auth = 0, exit_status = 0; char *banner;
SSH_SESSION *session; SSH_OPTIONS *opt;
if (!conn || !conn->uri) return VIR_DRV_OPEN_DECLINED;
if (conn->uri->scheme == NULL || conn->uri->server == NULL || conn->uri->path == NULL) return VIR_DRV_OPEN_DECLINED;
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)) { virRaiseError(conn, NULL, NULL, 0, VIR_FROM_PHYP, VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, "%s", _("connection failed")); ssh_disconnect(session); ssh_finalize(); }
/*trying to use pub key */ if ((ssh_auth = ssh_userauth_autopubkey(session, NULL)) == SSH_AUTH_ERROR) { virRaiseError(conn, NULL, NULL, 0, VIR_FROM_PHYP, VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, "%s : %s", _("authenticating with public key ailed"), ssh_get_error(session)); }
if ((banner = ssh_get_issue_banner(session))) { virRaiseError(conn, NULL, NULL, 0, VIR_FROM_PHYP, VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, "%s", banner); VIR_FREE(banner); }
If you call virRaiseError in these 3 cases, then you need to also return from this function with VIR_DRV_OPEN_ERROR so caller can see it failed. If these are merely warnings, and you do intend to continue, then VIR_WARN() (from logging.h is most appropriate)
if (ssh_auth != SSH_AUTH_SUCCESS) { int i; int hasPassphrase = 0;
virConnectCredential creds[] = { {VIR_CRED_PASSPHRASE, "password", "Password", NULL, NULL, 0}, };
if (!auth || !auth->cb) { virRaiseError(conn, NULL, NULL, 0, VIR_FROM_PHYP, VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, "%s", _("no authentication callback provided")); goto err; }
for (i = 0; i < auth->ncredtype; i++) { if (auth->credtype[i] == VIR_CRED_PASSPHRASE) hasPassphrase = 1; }
if (!hasPassphrase) { virRaiseError(conn, NULL, NULL, 0, VIR_FROM_PHYP, VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, "%s", _("required credentials are not supported")); goto err; }
int res = (auth->cb) (creds, ARRAY_CARDINALITY(creds), auth->cbdata);
if (res < 0) { virRaiseError(conn, NULL, NULL, 0, VIR_FROM_PHYP, VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, "%s", _("unable to fetch credentials")); goto err; }
char *password = creds[0].result; char *username = conn->uri->user;
if (ssh_userauth_password(session, username, password) != SSH_AUTH_SUCCESS) { virRaiseError(conn, NULL, NULL, 0, VIR_FROM_PHYP, VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, "%s : % s", "authentication failed", ssh_get_error(session)); ssh_disconnect(session); memset(password, 0, strlen(password)); memset(username, 0, strlen(username)); goto err; } else goto exec;
I imagine you want to blank out the password in both branches of that if() statement. Blanking the username is redundant since its permanently stored in the xmlURIPtr
} else goto exec;
err: exit_status = SSH_CONN_ERR; return VIR_DRV_OPEN_DECLINED;
You should use OPEN_ERROR in this location. Only use OPEN_DECLINED if the initial conn->uri was not for the phy:// driver (you already handle this scenario correctly earlier on in this method)
/* this functions is the layer that manipulates the ssh channel itself * and executes the commands on the remote machine */ static char * __inner_exec_command(SSH_SESSION * session, char *cmd, int *exit_status)
Having an exit_status var here is overkill, since the caller already checks the return value for NULL and can thus detect error there.
{ CHANNEL *channel = channel_new(session); char buf[4096] = { 0 }; virBuffer tex_ret = VIR_BUFFER_INITIALIZER;
int ret = 0;
if (channel_open_session(channel) == SSH_ERROR) goto err;
if (channel_request_exec(channel, cmd) == SSH_ERROR) goto err;
if (channel_send_eof(channel) == SSH_ERROR) goto err;
It is desired to have each of these errors virRaiseError with the specific details of what went wrong.
while (channel && channel_is_open(channel)) { ret = channel_read(channel, buf, sizeof(buf), 0); if (ret < 0) goto err;
if (ret == 0) { channel_send_eof(channel); if (channel_get_exit_status(channel) == -1) goto err;
if (channel_close(channel) == SSH_ERROR) goto err;
channel_free(channel); channel = NULL; goto exit; }
virBufferAdd(&tex_ret, (const char *) &buf, sizeof(buf)); }
err: (*exit_status) = SSH_CMD_ERR; return NULL;
Here you need to free the buffer char *buf = virBufferContentAndReset(&tex_ret) VIR_FREE(buf);
exit: return virBufferContentAndReset(&tex_ret);
This also needs a check for OOM, so you can report ther error message if (virBufferError(&tex_ret)) virReportOOMError(conn); return NULL; return virBufferContentAndReset(&tex_ret)
}
/* return the lpar_id given a name and a managed system name */ static int phypGetLparID(SSH_SESSION * ssh_session, const char *managed_system, const char *name) { int exit_status = 0; int lpar_id = 0; char *char_ptr; char *cmd;
if (virAsprintf(&cmd, "lssyscfg -r lpar -m %s --filter lpar_names=%s -F lpar_id", managed_system, name) < 0) return 0;
const char *tex_ret = __inner_exec_command(ssh_session, cmd, &exit_status);
VIR_FREE(cmd);
if (exit_status < 0 || tex_ret == NULL) return 0;
if (virStrToLong_i(tex_ret, &char_ptr, 10, &lpar_id) == -1) return 0;
return lpar_id; }
/* return the lpar name given a lpar_id and a managed system name */ static char * phypGetLparNAME(SSH_SESSION * ssh_session, const char *managed_system, unsigned int lpar_id, int *exit_status) { char *cmd;
if (virAsprintf(&cmd, "lssyscfg -r lpar -m %s --filter lpar_ids=%d -F name", managed_system, lpar_id) < 0) return NULL;
char *lpar_name = __inner_exec_command(ssh_session, cmd, (int *) exit_status);
This is forgetting to check lpar_name for NULL.
char *striped_lpar_name = (char *) malloc(sizeof(lpar_name) - 1);
stripNewline(striped_lpar_name, lpar_name);
malloc()/free/calloc/realloc should all be avoided, in favour of using the VIR_ALLOC/ALLOC_N/REALLOC_N/FREE comamnds instead. In this case though, the malloc() is overkill - just modify the original string, rather than reallocating it 1 byte shorter. eg replace those 2 lines with char *nl = strchr(lpar_name, '\n'); if (nl) *nl = '\0';
VIR_FREE(cmd);
if ((*exit_status) < 0 || lpar_name == NULL) return NULL;
return striped_lpar_name; }
/* return the lpar_uuid (which for now is its logical serial number) * given a lpar id and a managed system name */ static unsigned char * phypGetLparUUID(SSH_SESSION * ssh_session, const char *managed_system, unsigned int lpar_id, int *exit_status) { char *cmd;
if (virAsprintf(&cmd, "lssyscfg -r lpar -m %s --filter lpar_ids=%d -F logical_serial_num", managed_system, lpar_id) < 0) return NULL; unsigned char *lpar_uuid = (unsigned char *) __inner_exec_command(ssh_session, cmd, (int *) exit_status);
unsigned char *striped_lpar_uuid = (unsigned char *) malloc(sizeof(lpar_uuid) - 1); stripNewline((char *) striped_lpar_uuid, (char *) lpar_uuid);
When a UUID is declared 'unsigned char*', this is intended to be the raw binary 16 byte array. So just casting from a NULL terminated string is not sufficient. Instead call into the virUUIDParse() method from src/uuid.h to convert from NULL terminated string, to raw byte array.
VIR_FREE(cmd);
if ((*exit_status) < 0 || lpar_uuid == NULL) return NULL;
return striped_lpar_uuid; }
static int phypNumDomains(virConnectPtr conn) { int exit_status = 0; int ndom = 0; char *char_ptr; char *cmd; char managed_system[strlen(conn->uri->path) - 1]; SSH_SESSION *ssh_session = conn->privateData;
stripPath((char *) &managed_system, conn->uri->path); if (virAsprintf(&cmd, "lssyscfg -r lpar -m %s -F lpar_id|grep -c ^[0-9]*", managed_system) < 0) { virReportOOMError(conn); return 0; }
char *ret = __inner_exec_command(ssh_session, cmd, &exit_status);
VIR_FREE(cmd);
if (exit_status < 0 || ret == NULL) return 0;
if (virStrToLong_i(ret, &char_ptr, 10, &ndom) == -1) return 0;
return ndom; }
static int phypListDomains(virConnectPtr conn, int *ids, int nids) { int exit_status = 0; int got = 0; char *char_ptr; unsigned int i = 0, j = 0; char id_c[10]; char *cmd; char managed_system[strlen(conn->uri->path) - 1]; SSH_SESSION *ssh_session = conn->privateData;
stripPath((char *) &managed_system, conn->uri->path);
memset(id_c, 0, 10);
if (virAsprintf(&cmd, "lssyscfg -r lpar -m %s -F lpar_id", managed_system) < 0) { virReportOOMError(conn); return 0; } char *domains = __inner_exec_command(ssh_session, cmd, &exit_status);
/* I need to parse the textual return in order to get the domains */ if (exit_status < 0 || domains == NULL) return 0; else { while (got < nids) { if (domains[i] == '\n') { if (virStrToLong_i(id_c, &char_ptr, 10, &ids[got]) == -1) return 0; memset(id_c, 0, 10); j = 0; got++; } else { id_c[j] = domains[i]; j++; } i++; } }
VIR_FREE(cmd); return got; }
static virDomainPtr phypDomainLookupByName(virConnectPtr conn, const char *name) { SSH_SESSION *ssh_session = conn->privateData; virDomainPtr dom = NULL;
int lpar_id = 0; int exit_status = 0; char managed_system[strlen(conn->uri->path) - 1];
This is allocating a variable length array on the stack which is something its best to avoid - prefer to allocate on the heap instead.
stripPath((char *) &managed_system, conn->uri->path);
lpar_id = phypGetLparID(ssh_session, managed_system, name); if (lpar_id == PHYP_NO_MEM) return NULL;
unsigned char *lpar_uuid = phypGetLparUUID(ssh_session, managed_system, lpar_id, &exit_status);
if (exit_status == PHYP_NO_MEM || lpar_uuid == NULL) 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; int exit_status = 0; char managed_system[strlen(conn->uri->path) - 1];
stripPath((char *) &managed_system, conn->uri->path);
char *lpar_name = phypGetLparNAME(ssh_session, managed_system, lpar_id, &exit_status);
if (exit_status == PHYP_NO_MEM) return NULL;
unsigned char *lpar_uuid = phypGetLparUUID(ssh_session, managed_system, lpar_id, &exit_status);
if (exit_status == 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; }
static virDriver phypDriver = { VIR_DRV_PHYP, "PHYP", phypOpen, /* open */ phypClose, /* close */ NULL, /* supports_feature */ NULL, /* type */ NULL, /* version */ NULL, /* getHostname */ NULL, /* getMaxVcpus */ NULL, /* nodeGetInfo */ NULL, /* getCapabilities */ phypListDomains, /* listDomains */ phypNumDomains, /* numOfDomains */ NULL, /* domainCreateXML */ phypDomainLookupByID, /* domainLookupByID */ NULL, /* domainLookupByUUID */ phypDomainLookupByName, /* domainLookupByName */ NULL, /* domainSuspend */ NULL, /* domainResume */ NULL, /* domainShutdown */ NULL, /* domainReboot */ NULL, /* domainDestroy */ NULL, /* domainGetOSType */ NULL, /* domainGetMaxMemory */ NULL, /* domainSetMaxMemory */ NULL, /* domainSetMemory */ NULL, /* domainGetInfo */ NULL, /* domainSave */ NULL, /* domainRestore */ NULL, /* domainCoreDump */ NULL, /* domainSetVcpus */ NULL, /* domainPinVcpu */ NULL, /* domainGetVcpus */ NULL, /* domainGetMaxVcpus */ NULL, /* domainGetSecurityLabel */ NULL, /* nodeGetSecurityModel */ NULL, /* domainDumpXML */ NULL, /* listDefinedDomains */ NULL, /* numOfDefinedDomains */ NULL, /* domainCreate */ NULL, /* domainDefineXML */ NULL, /* domainUndefine */ NULL, /* domainAttachDevice */ NULL, /* domainDetachDevice */ NULL, /* domainGetAutostart */ NULL, /* domainSetAutostart */ NULL, /* domainGetSchedulerType */ NULL, /* domainGetSchedulerParameters */ NULL, /* domainSetSchedulerParameters */ NULL, /* domainMigratePrepare */ NULL, /* domainMigratePerform */ NULL, /* domainMigrateFinish */ NULL, /* domainBlockStats */ NULL, /* domainInterfaceStats */ NULL, /* domainBlockPeek */ NULL, /* domainMemoryPeek */ NULL, /* nodeGetCellsFreeMemory */ NULL, /* getFreeMemory */ NULL, /* domainEventRegister */ NULL, /* domainEventDeregister */ NULL, /* domainMigratePrepare2 */ NULL, /* domainMigrateFinish2 */ NULL, /* nodeDeviceDettach */ NULL, /* nodeDeviceReAttach */ NULL, /* nodeDeviceReset */ };
int phypRegister(void) { virRegisterDriver(&phypDriver); return 0; }
/* function that helps me to strip out the first slash from the * uri: phyp://user@hmc/path * * '/path' becomes 'path' * */ void stripPath(char *striped_path, char *path) { unsigned int i = 0;
for (i = 1; i <= strlen(path); i++) striped_path[i - 1] = path[i]; striped_path[i] = '\0'; return; }
I'm not convinced that the compiler will optimize this loop to avoid it being O(n^2) on strlen(path). It can be simplified by just calling strcpy, or memmove()'ing the original string inplace.
/* function to strip out the '\n' of the end of some string */ void stripNewline(char *striped_string, char *string) { unsigned int i = 0;
for (i = 0; i <= strlen(string); i++) if (string[i] != '\n') striped_string[i] = string[i]; striped_string[strlen(string) - 1] = '\0'; return; }
This can also be done in-place with a simple strchr() call 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 :|
The only feature we have until now is just to list the LPARs ("Logical PARtitions", the IBM virtual machines for Power). Once this code is safe and goot enough, the implementations of new commands will be much faster and easier.
I think for a initial commit of the driver I'd like to see it able to generate an XML description for each guest domain on the system. And also implement the listing of inactive domains.
So these three extra driver API points:
NULL, /* domainDumpXML */ NULL, /* listDefinedDomains */ NULL, /* numOfDefinedDomains */
This would be enough to make the 2 core virsh commands work fully with the drier
virsh list --all virsh dumpxml GUEST
I just opened a new thread in order to discuss the usage of this function and UUID.
/*starting ssh connection */ if (ssh_connect(session)) { virRaiseError(conn, NULL, NULL, 0, VIR_FROM_PHYP, VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, "%s", _("connection failed")); ssh_disconnect(session); ssh_finalize(); }
/*trying to use pub key */ if ((ssh_auth = ssh_userauth_autopubkey(session, NULL)) == SSH_AUTH_ERROR) { virRaiseError(conn, NULL, NULL, 0, VIR_FROM_PHYP, VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, "%s : %s", _("authenticating with public key ailed"), ssh_get_error(session)); }
if ((banner = ssh_get_issue_banner(session))) { virRaiseError(conn, NULL, NULL, 0, VIR_FROM_PHYP, VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, "%s", banner); VIR_FREE(banner); }
If you call virRaiseError in these 3 cases, then you need to also return from this function with VIR_DRV_OPEN_ERROR so caller can see it failed. If these are merely warnings, and you do intend to continue, then VIR_WARN() (from logging.h is most appropriate)
I adjusted the level of problem as I thought that would be more consistent. (the fixed phyp_driver.c is attatched)
char *password = creds[0].result; char *username = conn->uri->user;
if (ssh_userauth_password(session, username, password) != SSH_AUTH_SUCCESS) { virRaiseError(conn, NULL, NULL, 0, VIR_FROM_PHYP, VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, "%s : % s", "authentication failed", ssh_get_error(session)); ssh_disconnect(session); memset(password, 0, strlen(password)); memset(username, 0, strlen(username)); goto err; } else goto exec;
I imagine you want to blank out the password in both branches of that if() statement. Blanking the username is redundant since its permanently stored in the xmlURIPtr
Agreed and fixed.
} else goto exec;
err: exit_status = SSH_CONN_ERR; return VIR_DRV_OPEN_DECLINED;
You should use OPEN_ERROR in this location. Only use OPEN_DECLINED if the initial conn->uri was not for the phy:// driver (you already handle this scenario correctly earlier on in this method)
ok.
/* this functions is the layer that manipulates the ssh channel itself * and executes the commands on the remote machine */ static char * __inner_exec_command(SSH_SESSION * session, char *cmd, int *exit_status)
Having an exit_status var here is overkill, since the caller already checks the return value for NULL and can thus detect error there.
The exit_status here refers to the exit status of the command executed remotely and not the return of the function. The return of the '__inner_exec_command' function is NULL or the textual return from the remote command.
{ CHANNEL *channel = channel_new(session); char buf[4096] = { 0 }; virBuffer tex_ret = VIR_BUFFER_INITIALIZER;
int ret = 0;
if (channel_open_session(channel) == SSH_ERROR) goto err;
if (channel_request_exec(channel, cmd) == SSH_ERROR) goto err;
if (channel_send_eof(channel) == SSH_ERROR) goto err;
It is desired to have each of these errors virRaiseError with the specific details of what went wrong.
Agreed. Actually this was at my todo list and its done now.
while (channel && channel_is_open(channel)) { ret = channel_read(channel, buf, sizeof(buf), 0); if (ret < 0) goto err;
if (ret == 0) { channel_send_eof(channel); if (channel_get_exit_status(channel) == -1) goto err;
if (channel_close(channel) == SSH_ERROR) goto err;
channel_free(channel); channel = NULL; goto exit; }
virBufferAdd(&tex_ret, (const char *) &buf, sizeof(buf)); }
err: (*exit_status) = SSH_CMD_ERR; return NULL;
Here you need to free the buffer
char *buf = virBufferContentAndReset(&tex_ret) VIR_FREE(buf);
exit: return virBufferContentAndReset(&tex_ret);
This also needs a check for OOM, so you can report ther error message
if (virBufferError(&tex_ret)) virReportOOMError(conn); return NULL; return virBufferContentAndReset(&tex_ret)
In order to check virBufferError and virReportOOMError I added 'virConnectPtr conn' as parameter to __inner_exec_command function. Everything is recorded now.
/* return the lpar name given a lpar_id and a managed system name */ static char * phypGetLparNAME(SSH_SESSION * ssh_session, const char *managed_system, unsigned int lpar_id, int *exit_status) { char *cmd;
if (virAsprintf(&cmd, "lssyscfg -r lpar -m %s --filter lpar_ids=%d -F name", managed_system, lpar_id) < 0) return NULL;
char *lpar_name = __inner_exec_command(ssh_session, cmd, (int *) exit_status);
This is forgetting to check lpar_name for NULL.
char *striped_lpar_name = (char *) malloc(sizeof(lpar_name) - 1);
stripNewline(striped_lpar_name, lpar_name);
malloc()/free/calloc/realloc should all be avoided, in favour of using the VIR_ALLOC/ALLOC_N/REALLOC_N/FREE comamnds instead.
In this case though, the malloc() is overkill - just modify the original string, rather than reallocating it 1 byte shorter.
eg replace those 2 lines with
char *nl = strchr(lpar_name, '\n'); if (nl) *nl = '\0';
These two lines opened my mind yesterday and made me remove those two ugly function. I wasn't happy with them and I was trying other ways to handle those chars. The new "stripPath" is reduced to: char *managed_system = conn->uri->path; if (managed_system[0] == '/') managed_system++; And the new "stripNewline" I did as you told me: char *char_ptr = strchr(lpar_uuid, '\n'); if (char_ptr) *char_ptr = '\0';
/* return the lpar_uuid (which for now is its logical serial number) * given a lpar id and a managed system name */ static unsigned char * phypGetLparUUID(SSH_SESSION * ssh_session, const char *managed_system, unsigned int lpar_id, int *exit_status) { char *cmd;
if (virAsprintf(&cmd, "lssyscfg -r lpar -m %s --filter lpar_ids=%d -F logical_serial_num", managed_system, lpar_id) < 0) return NULL; unsigned char *lpar_uuid = (unsigned char *) __inner_exec_command(ssh_session, cmd, (int *) exit_status);
unsigned char *striped_lpar_uuid = (unsigned char *) malloc(sizeof(lpar_uuid) - 1); stripNewline((char *) striped_lpar_uuid, (char *) lpar_uuid);
When a UUID is declared 'unsigned char*', this is intended to be the raw binary 16 byte array. So just casting from a NULL terminated string is not sufficient. Instead call into the virUUIDParse() method from src/uuid.h to convert from NULL terminated string, to raw byte array.
This is not fixed in the phyp_driver.c that is attatched here, but will be fixed as soon as we discuss that UUID issue in the thread is just opened.
static virDomainPtr phypDomainLookupByName(virConnectPtr conn, const char *name) { SSH_SESSION *ssh_session = conn->privateData; virDomainPtr dom = NULL;
int lpar_id = 0; int exit_status = 0; char managed_system[strlen(conn->uri->path) - 1];
This is allocating a variable length array on the stack which is something its best to avoid - prefer to allocate on the heap instead.
This also made me think a better way to handle managed_system without this stack allocation. Also fixed.
void stripPath(char *striped_path, char *path) { unsigned int i = 0;
for (i = 1; i <= strlen(path); i++) striped_path[i - 1] = path[i]; striped_path[i] = '\0'; return; }
I'm not convinced that the compiler will optimize this loop to avoid it being O(n^2) on strlen(path). It can be simplified by just calling strcpy, or memmove()'ing the original string inplace.
Fixed!
/* function to strip out the '\n' of the end of some string */ void stripNewline(char *striped_string, char *string) { unsigned int i = 0;
for (i = 0; i <= strlen(string); i++) if (string[i] != '\n') striped_string[i] = string[i]; striped_string[strlen(string) - 1] = '\0'; return; }
This can also be done in-place with a simple strchr() call
Fixed! -- Eduardo Otubo Software Engineer Linux Technology Center IBM Systems & Technology Group Mobile: +55 19 8135 0885 otubo@linux.vnet.ibm.com
participants (4)
-
Daniel P. Berrange -
Daniel Veillard -
Eduardo Otubo -
Kaitlin Rupert