[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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/


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]