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
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list