[libvirt] [RFC] Power Hypervisor Libvirt support
Eduardo Otubo
otubo at linux.vnet.ibm.com
Fri May 15 16:36:26 UTC 2009
> > 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
I just opened a new thread in order to discuss the usage of this
function and UUID.
> > /*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)
I adjusted the level of problem as I thought that would be more
consistent. (the fixed phyp_driver.c is attatched)
> >
> > 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
Agreed and fixed.
>
> > } 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)
ok.
>
> >
> > /* 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.
The exit_status here refers to the exit status of the command executed
remotely and not the return of the function. The return of the
'__inner_exec_command' function is NULL or the textual return from the
remote command.
>
> > {
> > 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.
Agreed. Actually this was at my todo list and its done now.
>
> >
> > 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)
In order to check virBufferError and virReportOOMError I added
'virConnectPtr conn' as parameter to __inner_exec_command function.
Everything is recorded now.
> >
> > /* 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';
These two lines opened my mind yesterday and made me remove those two
ugly function. I wasn't happy with them and I was trying other ways to
handle those chars. The new "stripPath" is reduced to:
char *managed_system = conn->uri->path;
if (managed_system[0] == '/')
managed_system++;
And the new "stripNewline" I did as you told me:
char *char_ptr = strchr(lpar_uuid, '\n');
if (char_ptr)
*char_ptr = '\0';
> >
> > /* 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.
This is not fixed in the phyp_driver.c that is attatched here, but will
be fixed as soon as we discuss that UUID issue in the thread is just
opened.
> >
> > 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.
This also made me think a better way to handle managed_system without
this stack allocation. Also fixed.
> > 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.
Fixed!
>
> > /* 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
Fixed!
--
Eduardo Otubo
Software Engineer
Linux Technology Center
IBM Systems & Technology Group
Mobile: +55 19 8135 0885
otubo at linux.vnet.ibm.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: phyp_driver.c
Type: text/x-csrc
Size: 17603 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20090515/34fac75c/attachment-0001.bin>
More information about the libvir-list
mailing list