11:36 a.m.
> 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(a)linux.vnet.ibm.com