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

Re: [libvirt] [PATCH] Power Hypervisor Support for libvirt - minimum set of features



And all the other comments you did are now fixed. Thanks.
(and sorry for the top posting)

[]'s

On Mon, 2009-06-29 at 10:44 +0100, Daniel P. Berrange wrote:
> On Mon, Jun 22, 2009 at 06:57:19PM -0300, Eduardo Otubo wrote:
> > Hello all,
> > 
> > This is the initial patch for the driver for IBM Power Hypervisors. The
> > minimum set of features are now implemented: list, list --all and
> > dumpxml. Here is the Changeset since last PATCH I sent:
> > 
> > * The URI has changed to: phyp://user [hmc|ivm]/managed_system. If the
> > system is a HMC+VIOS based, only an HMC authentication will be required.
> > Commands will be sent to VIOS trough HMC command line. And if the system
> > is an IVM based, then just provide the username and password for IVM.
> > 
> > * Since the Power Hypervisor has no information about UUID's, I built a
> > little database (uuid_db) to store and associate LPAR ID's with UUID
> > randomly generated by the API.
> 
> I might be missing something, but this database doesn't appear to be
> persistent at all - it just lives for the duration of the virConnectPtr
> object's lifetime. So if you create  two connections you'd get two
> different UUIDs for the same VM.
> 
> > * The command dumpxml is implemented, but there are some informations
> > missing. Fetching informations like fstab, os type, uptime, IP addr and
> > so on, will only be available in a future versions of the HMC system.
> 
> That's fine - starting simple is the way to go.
> 
> > +/*
> > + * URI: phyp://user [hmc|ivm]/managed_system
> > + * */
> > +
> > +static virDrvOpenStatus
> > +phypOpen(virConnectPtr conn,
> > +         virConnectAuthPtr auth, int flags ATTRIBUTE_UNUSED)
> > +{
> > +    SSH_SESSION *session;
> > +    ConnectionData *connection_data;
> > +
> > +    uuid_dbPtr uuid_db = NULL;
> > +
> > +    if (VIR_ALLOC(uuid_db) < 0)
> > +        virReportOOMError(conn);
> > +
> > +    if (VIR_ALLOC(connection_data) < 0)
> > +        virReportOOMError(conn);
> > +
> > +    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;
> 
> Here you need to check that the 'scheme' really is for your 
> driver, before continuing. If the scheme matches your driver,
> then if there are any further errors such as missing server
> or path, then you need to return VIR_DRV_OPEN_ERROR instead
> of DECLINED. So this block should look like:
> 
>      if (conn->uri->scheme == NULL || 
>          STRNEQ(conn->uri->scheme, "phyp")
>        return VIR_DRV_OPEN_DECLINED;
> 
> 
>      if (conn->uri->server == NULL) {
>          virRaiseError(conn, NULL, NULL, 0, VIR_FROM_PHYP,
>                        VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, "%s",
>                        _("Missing server name in phyp:// URI"));
>         return VIR_DRV_OPEN_ERROR;
>      }
>      if (conn->uri->path == NULL) {
>          virRaiseError(conn, NULL, NULL, 0, VIR_FROM_PHYP,
>                        VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, "%s",
>                        _("Missing path name in phyp:// URI"));
>         return VIR_DRV_OPEN_ERROR;
>      }
> 
> > +
> > +SSH_SESSION *
> > +openSSHSession(virConnectPtr conn, virConnectAuthPtr auth)
> > +{
> > +    SSH_SESSION *session;
> > +    SSH_OPTIONS *opt;
> > +    char *user = conn->uri->user;
> > +    char *host = conn->uri->server;
> > +    int ssh_auth = 0;
> > +    char *banner;
> > +    int port = 22;
> > +
> > +    if (conn->uri->port)
> > +        port = conn->uri->port;
> > +
> > +    session = ssh_new();
> > +    opt = ssh_options_new();
> > +
> > +    /*setting some ssh options */
> > +    ssh_options_set_host(opt, host);
> > +    ssh_options_set_port(opt, port);
> > +    ssh_options_set_username(opt, 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();
> > +        goto err;
> > +    }
> > +
> > +    /*trying to use pub key */
> > +    if ((ssh_auth =
> > +         ssh_userauth_autopubkey(session, NULL)) == SSH_AUTH_ERROR) {
> > +        VIR_WARN("%s", "Authentication with public key failed.");
> > +    }
> > +
> > +    if ((banner = ssh_get_issue_banner(session))) {
> > +        VIR_WARN("%s", banner);
> > +        VIR_FREE(banner);
> 
> Just tweak this to VIR_INFO(), rather than WARN.
> 
> > +    }
> > +
> > +    if (ssh_auth != SSH_AUTH_SUCCESS) {
> > +        int i;
> > +        int hasPassphrase = 0;
> > +        int auth_check = 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;
> 
> If it possible for creds[0].result to be NULL, even if 'res == 0'
> from the callback, so to be safe you should check for that before
> using the data.
> 
> > +
> > +        char *username = user;
> > +
> > +        auth_check = ssh_userauth_password(session, username, password);
> > +        memset(password, 0, strlen(password));
> > +
> > +        if (auth_check != 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);
> > +            goto err;
> > +        } else
> > +            goto exit;
> > +    } else
> > +        goto exit;
> > +
> > +  err:
> > +    return NULL;
> > +
> > +  exit:
> > +    return session;
> > +}
> > +
> > +/* this functions is the layer that manipulates the ssh channel itself
> > + * and executes the commands on the remote machine */
> > +static char *
> > +exec(SSH_SESSION * session, char *cmd, int *exit_status,
> > +     virConnectPtr conn)
> 
> I'd recommend renaming this to something that is not simply called
> 'exec' to avoid readers getting confused with the 'exec' system
> calls. Perhaps  something like  phypRemoteExec()  
> 
> > +{
> > +    CHANNEL *channel = channel_new(session);
> > +    virBuffer tex_ret = VIR_BUFFER_INITIALIZER;
> > +    char buf[4096] = { 0 };
> > +    int ret = 0;
> > +
> > +    if (channel_open_session(channel) == SSH_ERROR) {
> > +        virRaiseError(NULL, NULL, NULL, 0, VIR_FROM_PHYP,
> > +                      VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, "%s",
> > +                      _("Unable to open a SSH channel."));
> > +        goto err;
> > +    }
> > +
> > +    if (channel_request_exec(channel, cmd) == SSH_ERROR) {
> > +        virRaiseError(NULL, NULL, NULL, 0, VIR_FROM_PHYP,
> > +                      VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, "%s",
> > +                      _("Unable to execute remote command."));
> > +        goto err;
> > +    }
> > +
> > +    if (channel_send_eof(channel) == SSH_ERROR) {
> > +        virRaiseError(NULL, NULL, NULL, 0, VIR_FROM_PHYP,
> > +                      VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, "%s",
> > +                      _("Unable to send EOF."));
> > +        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));
> 
> I'm thinking that you should be passing 'ret' rather than
> sizeof(buf) here, since I imagine its possible for the
> channel_read()  command to return less than a full buffer
> worth of data.
> 
> 
> > +    }
> > +
> > +  err:
> > +    (*exit_status) = SSH_CMD_ERR;
> > +    char *cleanup_buf = virBufferContentAndReset(&tex_ret);
> > +
> > +    VIR_FREE(cleanup_buf);
> > +    return NULL;
> > +
> > +  exit:
> > +    if (virBufferError(&tex_ret)) {
> > +        virReportOOMError(conn);
> > +        return NULL;
> > +    }
> > +    return virBufferContentAndReset(&tex_ret);
> > +}
> > +
> > +
> > +/* This is a generic function that won't be used directly by
> > + * libvirt api. The function returns the number of domains
> > + * in different states: Running, Not Activated and all:
> > + *
> > + * type: 0 - Running
> > + *       1 - Not Activated
> > + *       * - All
> > + * */
> > +static int
> > +phypNumDomainsGeneric(virConnectPtr conn, unsigned int type)
> > +{
> > +    ConnectionData *connection_data = conn->networkPrivateData;
> > +    SSH_SESSION *ssh_session = connection_data->session;
> > +    int exit_status = 0;
> > +    int ndom = 0;
> > +    char *char_ptr;
> > +    char *cmd;
> > +    char *managed_system = conn->uri->path;
> > +    virBuffer state = VIR_BUFFER_INITIALIZER;
> > +
> > +    if (type == 0)
> > +        virBufferAddLit(&state, "|grep Running");
> > +    else if (type == 1)
> > +        virBufferAddLit(&state, "|grep \"Not Activated\"");
> > +    else
> > +        virBufferAddLit(&state, " ");
> > +
> > +    /* need to shift one byte in order to remove the first "/" of URI component */
> > +    if (managed_system[0] == '/')
> > +        managed_system++;
> > +
> > +    /* here we are handling only the first component of the path,
> > +     * so skipping the second:
> > +     * */
> > +
> > +    char_ptr = strchr(managed_system, '/');
> > +
> > +    if (char_ptr)
> > +        *char_ptr = '\0';
> > +
> > +    if (virAsprintf(&cmd,
> > +                    "lssyscfg -r lpar -m %s -F lpar_id,state %s |grep -c ^[0-9]*",
> > +                    managed_system,
> > +                    virBufferContentAndReset(&state)) < 0) {
> > +        virReportOOMError(conn);
> > +        goto err;
> > +    }
> 
> This has a small memory leak - you need to free the pointer returned
> by a virBufferContentAndReset() call. It is  alittle overkill to
> use a virBuffer here, since you only initialize it with a const
> string. It would thus be simpler to just do
> 
>      const char *state;
> 
>      if (type == 0)
>         state = "|grep Running";
>      else if (type == 1)
>         state = "|grep \"Not Activated\"";
>      else
>         state = " ";
> 
> 
> > +
> > +    char *ret = exec(ssh_session, cmd, &exit_status, conn);
> > +
> > +    if (exit_status < 0 || ret == NULL)
> > +        goto err;
> > +
> > +    if (virStrToLong_i(ret, &char_ptr, 10, &ndom) == -1)
> > +        goto err;
> > +
> > +    VIR_FREE(cmd);
> > +    return ndom;
> > +
> > +  err:
> > +    VIR_FREE(cmd);
> > +    return 0;
> > +}
> > +
> > +static int
> > + * */
> > +static int
> > +phypListDomainsGeneric(virConnectPtr conn, int *ids, int nids,
> > +                       unsigned int type)
> > +{
> > +    ConnectionData *connection_data = conn->networkPrivateData;
> > +    SSH_SESSION *ssh_session = connection_data->session;
> > +    virBuffer state = VIR_BUFFER_INITIALIZER;
> > +    char *managed_system = conn->uri->path;
> > +    int exit_status = 0;
> > +    int got = 0;
> > +    char *char_ptr;
> > +    unsigned int i = 0, j = 0;
> > +    char id_c[10];
> > +    char *cmd;
> > +
> > +    if (type == 0)
> > +        virBufferAddLit(&state, "|grep Running");
> > +    else
> > +        virBufferAddLit(&state, " ");
> > +
> > +    /* need to shift one byte in order to remove the first "/" of URI component */
> > +    if (managed_system[0] == '/')
> > +        managed_system++;
> > +
> > +    /* here we are handling only the first component of the path,
> > +     * so skipping the second:
> > +     * */
> > +    char_ptr = strchr(managed_system, '/');
> > +
> > +    if (char_ptr)
> > +        *char_ptr = '\0';
> > +
> > +    memset(id_c, 0, 10);
> > +
> > +    if (virAsprintf
> > +        (&cmd,
> > +         "lssyscfg -r lpar -m %s -F lpar_id,state %s | sed -e 's/,.*$//g'",
> > +         managed_system, virBufferContentAndReset(&state)) < 0) {
> > +        virReportOOMError(conn);
> > +        goto err;
> > +    }
> 
> Same recommendation here - just use a const char * string for the last
> 'state' field.
> 
> > +    return phypListDomainsGeneric(conn, ids, nids, 0);
> > +}
> > +
> > +static int
> > +phypListDefinedDomains(virConnectPtr conn, char **const names, int nnames)
> > +{
> > +    ConnectionData *connection_data = conn->networkPrivateData;
> > +    SSH_SESSION *ssh_session = connection_data->session;
> > +    char *managed_system = conn->uri->path;
> > +    int exit_status = 0;
> > +    int got = 0;
> > +    char *char_ptr = NULL;
> > +    char *cmd;
> > +
> > +    /* need to shift one byte in order to remove the first "/" of URI component */
> > +    if (managed_system[0] == '/')
> > +        managed_system++;
> > +
> > +    /* here we are handling only the first component of the path,
> > +     * so skipping the second:
> > +     * */
> > +    char_ptr = strchr(managed_system, '/');
> > +
> > +    if (char_ptr)
> > +        *char_ptr = '\0';
> > +
> > +    if (virAsprintf
> > +        (&cmd,
> > +         "lssyscfg -r lpar -m %s -F name,state | grep \"Not Activated\" | sed -e 's/,.*$//g'",
> > +         managed_system) < 0) {
> > +        virReportOOMError(conn);
> > +        goto err;
> > +    }
> > +    char *ret = exec(ssh_session, cmd, &exit_status, conn);
> > +    char *domains = malloc(sizeof(ret));
> 
> malloc() is on our banned list - switch to VIR_ALLOC() here.
> 
> > +    domains = strdup(ret);
> > +
> > +    char *char_ptr2 = NULL;
> > +    /* I need to parse the textual return in order to get the domains */
> > +    if (exit_status < 0 || domains == NULL)
> > +        goto err;
> > +    else {
> > +        while (got < nnames) {
> > +            char_ptr2 = strchr(domains, '\n');
> > +
> > +            if (char_ptr2) {
> > +                *char_ptr2 = '\0';
> > +                names[got] = strdup(domains);
> 
> Need to check for strdup() returning NULL, and cleanup  and return an
> OOM error code.
> 
> > +                char_ptr2++;
> > +                domains = char_ptr2;
> > +                got++;
> > +            }
> > +        }
> > +    }
> > +
> > +    VIR_FREE(cmd);
> > +    VIR_FREE(ret);
> > +    return got;
> > +
> > +  err:
> > +    VIR_FREE(cmd);
> > +    VIR_FREE(ret);
> > +    return 0;
> > +}
> > +
> > +
> > +static char *
> > +phypDomainDumpXML(virDomainPtr dom, int flags)
> > +{
> > +    ConnectionData *connection_data = dom->conn->networkPrivateData;
> > +    SSH_SESSION *ssh_session = connection_data->session;
> > +    virDomainDefPtr def = NULL;
> > +    char *ret = NULL;
> > +    char *managed_system = dom->conn->uri->path;
> > +    unsigned char *lpar_uuid = NULL;
> > +
> > +    if (VIR_ALLOC_N(lpar_uuid, VIR_UUID_BUFLEN) < 0)
> > +        virReportOOMError(dom->conn);
> > +
> > +    if (VIR_ALLOC(def) < 0)
> > +        virReportOOMError(dom->conn);
> > +
> > +    /* need to shift one byte in order to remove the first "/" of uri component */
> > +    if (managed_system[0] == '/')
> > +        managed_system++;
> > +
> > +    /* here we are handling only the first component of the path,
> > +     * so skipping the second:
> > +     * */
> > +    char *char_ptr = strchr(managed_system, '/');
> > +
> > +    if (char_ptr)
> > +        *char_ptr = '\0';
> > +
> > +    def->virtType = VIR_DOMAIN_VIRT_PHYP;
> > +    def->id = dom->id;
> > +
> > +    char *lpar_name = phypGetLparNAME(ssh_session, managed_system, def->id,
> > +                                      dom->conn);
> > +
> > +    if (lpar_name == NULL)
> > +        VIR_WARN("%s", "Unable to determine domain's name.");
> > +
> > +    if (phypGetLparUUID(lpar_uuid, dom->id, dom->conn) == -1)
> > +        VIR_WARN("%s", "Unable to generate random uuid.");
> > +
> > +    if (!memcpy(def->uuid, lpar_uuid, VIR_UUID_BUFLEN))
> > +        VIR_WARN("%s", "Unable to generate random uuid.");
> > +
> > +    if ((def->maxmem =
> > +         phypGetLparMem(dom->conn, managed_system, dom->id, 0)) == 0)
> > +        VIR_WARN("%s", "Unable to determine domain's max memory.");
> > +
> > +    if ((def->memory =
> > +         phypGetLparMem(dom->conn, managed_system, dom->id, 1)) == 0)
> > +        VIR_WARN("%s", "Unable to determine domain's memory.");
> > +
> > +    if ((def->vcpus =
> > +         phypGetLparCPU(dom->conn, managed_system, dom->id)) == 0)
> > +        VIR_WARN("%s", "Unable to determine domain's CPU.");
> 
> 
> I'm thinking some, if not all of these should probably be treated as
> fatal errors, rather than just warnings.
> 
> Regards,
> Daniel


-- 
Eduardo Otubo
Software Engineer
Linux Technology Center
IBM Systems & Technology Group
Mobile: +55 19 8135 0885 
otubo linux vnet ibm com


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