On Fri, May 08, 2009 at 07:10:02PM -0300, Eduardo Otubo wrote:
>
> Here is the phyp_driver.c with the memory leaks fixed with your
> suggestions. With those things done, do you think this code is enough
> and compliant to libvirt patterns to be included in the next libvirt
> release?
Yep, the virBuffer/virAsprintf usage looks good now.
> The only feature we have until now is just to list the LPARs ("Logical
> PARtitions", the IBM virtual machines for Power). Once this code is safe
> and goot enough, the implementations of new commands will be much faster
> and easier.
I think for a initial commit of the driver I'd like to see it able
to generate an XML description for each guest domain on the system.
And also implement the listing of inactive domains.
So these three extra driver API points:
> NULL, /* domainDumpXML */
> NULL, /* listDefinedDomains */
> NULL, /* numOfDefinedDomains */
This would be enough to make the 2 core virsh commands work
fully with the drier
virsh list --all
virsh dumpxml GUEST
> static virDrvOpenStatus
> phypOpen(virConnectPtr conn,
> virConnectAuthPtr auth, int flags ATTRIBUTE_UNUSED)
> {
> int ssh_auth = 0, exit_status = 0;
> char *banner;
>
> SSH_SESSION *session;
> SSH_OPTIONS *opt;
>
> 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;
>
> session = ssh_new();
> opt = ssh_options_new();
>
> if (!conn->uri->port)
> conn->uri->port = 22;
>
> /*setting some ssh options */
> ssh_options_set_host(opt, conn->uri->server);
> ssh_options_set_port(opt, conn->uri->port);
> ssh_options_set_username(opt, conn->uri->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();
> }
>
> /*trying to use pub key */
> if ((ssh_auth =
> ssh_userauth_autopubkey(session, NULL)) == SSH_AUTH_ERROR) {
> virRaiseError(conn, NULL, NULL, 0, VIR_FROM_PHYP, VIR_ERR_ERROR,
> NULL, NULL, NULL, 0, 0, "%s : %s",
> _("authenticating with public key ailed"),
> ssh_get_error(session));
> }
>
>
> if ((banner = ssh_get_issue_banner(session))) {
> virRaiseError(conn, NULL, NULL, 0, VIR_FROM_PHYP, VIR_ERR_ERROR,
> NULL, NULL, NULL, 0, 0, "%s", banner);
> VIR_FREE(banner);
> }
If you call virRaiseError in these 3 cases, then you need to also
return from this function with VIR_DRV_OPEN_ERROR so caller can
see it failed. If these are merely warnings, and you do intend
to continue, then VIR_WARN() (from logging.h is most appropriate)
>
> if (ssh_auth != SSH_AUTH_SUCCESS) {
> int i;
> int hasPassphrase = 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;
> char *username = conn->uri->user;
>
> if (ssh_userauth_password(session, username, password) !=
> 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);
> memset(password, 0, strlen(password));
> memset(username, 0, strlen(username));
> goto err;
> } else
> goto exec;
I imagine you want to blank out the password in both branches
of that if() statement. Blanking the username is redundant
since its permanently stored in the xmlURIPtr
> } else
> goto exec;
>
> err:
> exit_status = SSH_CONN_ERR;
> return VIR_DRV_OPEN_DECLINED;
You should use OPEN_ERROR in this location. Only use OPEN_DECLINED
if the initial conn->uri was not for the phy:// driver (you already
handle this scenario correctly earlier on in this method)
>
> /* 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)
Having an exit_status var here is overkill, since the caller
already checks the return value for NULL and can thus detect
error there.
> {
> CHANNEL *channel = channel_new(session);
> char buf[4096] = { 0 };
> virBuffer tex_ret = VIR_BUFFER_INITIALIZER;
>
> int ret = 0;
>
> if (channel_open_session(channel) == SSH_ERROR)
> goto err;
>
> if (channel_request_exec(channel, cmd) == SSH_ERROR)
> goto err;
>
> if (channel_send_eof(channel) == SSH_ERROR)
> goto err;
It is desired to have each of these errors virRaiseError
with the specific details of what went wrong.
>
> 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));
> }
>
> err:
> (*exit_status) = SSH_CMD_ERR;
> return NULL;
Here you need to free the buffer
char *buf = virBufferContentAndReset(&tex_ret)
VIR_FREE(buf);
>
> exit:
> return virBufferContentAndReset(&tex_ret);
This also needs a check for OOM, so you can report ther error
message
if (virBufferError(&tex_ret))
virReportOOMError(conn);
return NULL;
return virBufferContentAndReset(&tex_ret)
>
> }
>
> /* 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;
> int lpar_id = 0;
> char *char_ptr;
> char *cmd;
>
> if (virAsprintf(&cmd,
> "lssyscfg -r lpar -m %s --filter lpar_names=%s -F
> lpar_id",
> managed_system, name) < 0)
> return 0;
>
> const char *tex_ret =
> __inner_exec_command(ssh_session, cmd, &exit_status);
>
> VIR_FREE(cmd);
>
> if (exit_status < 0 || tex_ret == NULL)
> return 0;
>
> if (virStrToLong_i(tex_ret, &char_ptr, 10, &lpar_id) == -1)
> return 0;
>
> return lpar_id;
> }
>
> /* return the lpar name given a lpar_id and a managed system name */
> static char *
> phypGetLparNAME(SSH_SESSION * ssh_session, const char *managed_system,
> unsigned int lpar_id, int *exit_status)
> {
> char *cmd;
>
> if (virAsprintf(&cmd,
> "lssyscfg -r lpar -m %s --filter lpar_ids=%d -F
> name",
> managed_system, lpar_id) < 0)
> return NULL;
>
> char *lpar_name =
> __inner_exec_command(ssh_session, cmd, (int *) exit_status);
This is forgetting to check lpar_name for NULL.
>
> char *striped_lpar_name = (char *) malloc(sizeof(lpar_name) - 1);
>
> stripNewline(striped_lpar_name, lpar_name);
malloc()/free/calloc/realloc should all be avoided, in favour
of using the VIR_ALLOC/ALLOC_N/REALLOC_N/FREE comamnds instead.
In this case though, the malloc() is overkill - just modify
the original string, rather than reallocating it 1 byte
shorter.
eg replace those 2 lines with
char *nl = strchr(lpar_name, '\n');
if (nl) *nl = '\0';
>
> VIR_FREE(cmd);
>
> if ((*exit_status) < 0 || lpar_name == NULL)
> return NULL;
>
> return striped_lpar_name;
> }
>
> /* return the lpar_uuid (which for now is its logical serial number)
> * given a lpar id and a managed system name */
> static unsigned char *
> phypGetLparUUID(SSH_SESSION * ssh_session, const char *managed_system,
> unsigned int lpar_id, int *exit_status)
> {
> char *cmd;
>
> if (virAsprintf(&cmd,
> "lssyscfg -r lpar -m %s --filter lpar_ids=%d -F
> logical_serial_num",
> managed_system, lpar_id) < 0)
> return NULL;
> unsigned char *lpar_uuid =
> (unsigned char *) __inner_exec_command(ssh_session, cmd,
> (int *) exit_status);
>
> unsigned char *striped_lpar_uuid =
> (unsigned char *) malloc(sizeof(lpar_uuid) - 1);
> stripNewline((char *) striped_lpar_uuid, (char *) lpar_uuid);
When a UUID is declared 'unsigned char*', this is intended to
be the raw binary 16 byte array. So just casting from a NULL
terminated string is not sufficient. Instead call into the
virUUIDParse() method from src/uuid.h to convert from NULL
terminated string, to raw byte array.
>
> VIR_FREE(cmd);
>
> if ((*exit_status) < 0 || lpar_uuid == NULL)
> return NULL;
>
> return striped_lpar_uuid;
> }
>
> static int
> phypNumDomains(virConnectPtr conn)
> {
> int exit_status = 0;
> int ndom = 0;
> char *char_ptr;
> char *cmd;
> char managed_system[strlen(conn->uri->path) - 1];
> SSH_SESSION *ssh_session = conn->privateData;
>
> stripPath((char *) &managed_system, conn->uri->path);
> if (virAsprintf(&cmd,
> "lssyscfg -r lpar -m %s -F lpar_id|grep -c ^[0-9]*",
> managed_system) < 0) {
> virReportOOMError(conn);
> return 0;
> }
>
> char *ret = __inner_exec_command(ssh_session, cmd, &exit_status);
>
> VIR_FREE(cmd);
>
> if (exit_status < 0 || ret == NULL)
> return 0;
>
> if (virStrToLong_i(ret, &char_ptr, 10, &ndom) == -1)
> return 0;
>
> return ndom;
> }
>
> static int
> phypListDomains(virConnectPtr conn, int *ids, int nids)
> {
> int exit_status = 0;
> int got = 0;
> char *char_ptr;
> unsigned int i = 0, j = 0;
> char id_c[10];
> char *cmd;
> char managed_system[strlen(conn->uri->path) - 1];
> SSH_SESSION *ssh_session = conn->privateData;
>
> stripPath((char *) &managed_system, conn->uri->path);
>
> memset(id_c, 0, 10);
>
> if (virAsprintf(&cmd, "lssyscfg -r lpar -m %s -F lpar_id",
> managed_system) < 0) {
> virReportOOMError(conn);
> return 0;
> }
> char *domains = __inner_exec_command(ssh_session, cmd,
> &exit_status);
>
> /* I need to parse the textual return in order to get the domains */
> if (exit_status < 0 || domains == NULL)
> return 0;
> else {
> while (got < nids) {
> if (domains[i] == '\n') {
> if (virStrToLong_i(id_c, &char_ptr, 10, &ids[got]) ==
> -1)
> return 0;
> memset(id_c, 0, 10);
> j = 0;
> got++;
> } else {
> id_c[j] = domains[i];
> j++;
> }
> i++;
> }
> }
>
> VIR_FREE(cmd);
> return got;
> }
>
> static virDomainPtr
> phypDomainLookupByName(virConnectPtr conn, const char *name)
> {
> SSH_SESSION *ssh_session = conn->privateData;
> virDomainPtr dom = NULL;
>
> int lpar_id = 0;
> int exit_status = 0;
> char managed_system[strlen(conn->uri->path) - 1];
This is allocating a variable length array on the stack which
is something its best to avoid - prefer to allocate on the
heap instead.
>
> stripPath((char *) &managed_system, conn->uri->path);
>
> lpar_id = phypGetLparID(ssh_session, managed_system, name);
> if (lpar_id == PHYP_NO_MEM)
> return NULL;
>
> unsigned char *lpar_uuid =
> phypGetLparUUID(ssh_session, managed_system, lpar_id,
> &exit_status);
>
> if (exit_status == PHYP_NO_MEM || lpar_uuid == NULL)
> return NULL;
>
> dom = virGetDomain(conn, name, lpar_uuid);
>
> if (dom)
> dom->id = lpar_id;
>
> VIR_FREE(lpar_uuid);
> return dom;
> }
>
> static virDomainPtr
> phypDomainLookupByID(virConnectPtr conn, int lpar_id)
> {
> SSH_SESSION *ssh_session = conn->privateData;
> virDomainPtr dom = NULL;
> int exit_status = 0;
> char managed_system[strlen(conn->uri->path) - 1];
>
> stripPath((char *) &managed_system, conn->uri->path);
>
> char *lpar_name = phypGetLparNAME(ssh_session, managed_system,
> lpar_id,
> &exit_status);
>
> if (exit_status == PHYP_NO_MEM)
> return NULL;
>
> unsigned char *lpar_uuid =
> phypGetLparUUID(ssh_session, managed_system, lpar_id,
> &exit_status);
>
> if (exit_status == PHYP_NO_MEM)
> return NULL;
>
> dom = virGetDomain(conn, lpar_name, lpar_uuid);
>
> if (dom)
> dom->id = lpar_id;
>
> VIR_FREE(lpar_name);
> VIR_FREE(lpar_uuid);
> return dom;
> }
>
> static virDriver phypDriver = {
> VIR_DRV_PHYP,
> "PHYP",
> phypOpen, /* open */
> phypClose, /* close */
> NULL, /* supports_feature */
> NULL, /* type */
> NULL, /* version */
> NULL, /* getHostname */
> NULL, /* getMaxVcpus */
> NULL, /* nodeGetInfo */
> NULL, /* getCapabilities */
> phypListDomains, /* listDomains */
> phypNumDomains, /* numOfDomains */
> NULL, /* domainCreateXML */
> phypDomainLookupByID, /* domainLookupByID */
> NULL, /* domainLookupByUUID */
> phypDomainLookupByName, /* domainLookupByName */
> NULL, /* domainSuspend */
> NULL, /* domainResume */
> NULL, /* domainShutdown */
> NULL, /* domainReboot */
> NULL, /* domainDestroy */
> NULL, /* domainGetOSType */
> NULL, /* domainGetMaxMemory */
> NULL, /* domainSetMaxMemory */
> NULL, /* domainSetMemory */
> NULL, /* domainGetInfo */
> NULL, /* domainSave */
> NULL, /* domainRestore */
> NULL, /* domainCoreDump */
> NULL, /* domainSetVcpus */
> NULL, /* domainPinVcpu */
> NULL, /* domainGetVcpus */
> NULL, /* domainGetMaxVcpus */
> NULL, /* domainGetSecurityLabel */
> NULL, /* nodeGetSecurityModel */
> NULL, /* domainDumpXML */
> NULL, /* listDefinedDomains */
> NULL, /* numOfDefinedDomains */
> NULL, /* domainCreate */
> NULL, /* domainDefineXML */
> NULL, /* domainUndefine */
> NULL, /* domainAttachDevice */
> NULL, /* domainDetachDevice */
> NULL, /* domainGetAutostart */
> NULL, /* domainSetAutostart */
> NULL, /* domainGetSchedulerType */
> NULL, /* domainGetSchedulerParameters */
> NULL, /* domainSetSchedulerParameters */
> NULL, /* domainMigratePrepare */
> NULL, /* domainMigratePerform */
> NULL, /* domainMigrateFinish */
> NULL, /* domainBlockStats */
> NULL, /* domainInterfaceStats */
> NULL, /* domainBlockPeek */
> NULL, /* domainMemoryPeek */
> NULL, /* nodeGetCellsFreeMemory */
> NULL, /* getFreeMemory */
> NULL, /* domainEventRegister */
> NULL, /* domainEventDeregister */
> NULL, /* domainMigratePrepare2 */
> NULL, /* domainMigrateFinish2 */
> NULL, /* nodeDeviceDettach */
> NULL, /* nodeDeviceReAttach */
> NULL, /* nodeDeviceReset */
> };
>
> int
> phypRegister(void)
> {
> virRegisterDriver(&phypDriver);
> return 0;
> }
>
> /* function that helps me to strip out the first slash from the
> * uri: phyp://user@hmc/path
> *
> * '/path' becomes 'path'
> * */
> void
> stripPath(char *striped_path, char *path)
> {
> unsigned int i = 0;
>
> for (i = 1; i <= strlen(path); i++)
> striped_path[i - 1] = path[i];
> striped_path[i] = '\0';
> return;
> }
I'm not convinced that the compiler will optimize this loop
to avoid it being O(n^2) on strlen(path). It can be simplified
by just calling strcpy, or memmove()'ing the original string
inplace.
> /* function to strip out the '\n' of the end of some string */
> void
> stripNewline(char *striped_string, char *string)
> {
> unsigned int i = 0;
>
> for (i = 0; i <= strlen(string); i++)
> if (string[i] != '\n')
> striped_string[i] = string[i];
> striped_string[strlen(string) - 1] = '\0';
> return;
> }
This can also be done in-place with a simple strchr() call
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