10:22 a.m.
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/