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/