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@linux.vnet.ibm.com