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@xxxxxxxxxxxxxxxxxx
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list