On 06/04/2010 03:40 PM, Eduardo Otubo wrote:
> Adding support for the IBM IVM Virtualization system under Power
> Hypervisor.
>
> @@ -96,12 +99,6 @@ phypOpen(virConnectPtr conn,
> return VIR_DRV_OPEN_ERROR;
> }
>
> - if (conn->uri->path == NULL) {
> - PHYP_ERROR(VIR_ERR_INTERNAL_ERROR,
> - "%s", _("Missing managed system name in phyp:// URI"));
> - return VIR_DRV_OPEN_ERROR;
> - }
> -
> if (VIR_ALLOC(phyp_driver) < 0) {
> virReportOOMError();
> goto failure;
> @@ -117,36 +114,39 @@ phypOpen(virConnectPtr conn,
> goto failure;
> }
>
> - len = strlen(conn->uri->path) + 1;
> + if (conn->uri->path) {
> + len = strlen(conn->uri->path) + 1;
>
> - if (VIR_ALLOC_N(string, len) < 0) {
> - virReportOOMError();
> - goto failure;
> - }
> + if (VIR_ALLOC_N(string, len) < 0) {
> + virReportOOMError();
> + goto failure;
> + }
These changes were easier to review with 'git diff -b', and look
reasonable up to this point...
>
> +int
> +phypGetSystemType(virConnectPtr conn)
> +{
> + ConnectionData *connection_data = conn->networkPrivateData;
> + LIBSSH2_SESSION *session = connection_data->session;
> + char *cmd = NULL;
> + char *ret = NULL;
> + int exit_status = 0;
> +
> + if (virAsprintf(&cmd, "lshmc -V") < 0) {
> + virReportOOMError();
> + goto err;
> + }
> + ret = phypExec(session, cmd, &exit_status, conn);
> +
> + VIR_FREE(cmd);
> + VIR_FREE(ret);
> + return exit_status;
> +
> + err:
> + VIR_FREE(cmd);
> + VIR_FREE(ret);
> + return -1;
The two lists of VIR_FREE seem redundant. You could prime exit_status
to -1, then blindly return exit_status in the err clause with the
success clause falling through to the err clause. Or, since the only
way to get to the err clause is after virReportOOMError, when you
already know that cmd and ret are NULL, you could get rid of the err
clause altogether and just use 'return -1' instead of 'goto err'.
>
> - if (virAsprintf(&cmd,
> - "lssyscfg -r lpar -m %s --filter lpar_names=%s -F lpar_id",
> - managed_system, name) < 0) {
> - virReportOOMError();
> - goto err;
> + if (system_type == HMC) {
> + if (virAsprintf(&cmd,
> + "lssyscfg -r lpar -m %s --filter lpar_names=%s -F lpar_id",
> + managed_system, name) < 0) {
> + virReportOOMError();
> + goto err;
> + }
> + } else {
> + if (virAsprintf(&cmd,
> + "lssyscfg -r lpar --filter lpar_names=%s -F lpar_id",
> + name) < 0) {
> + virReportOOMError();
> + goto err;
> + }
A lot of the patch is like this, adding a non-HMC version that merely
omits the -m %s part of the string. That means the executable contains
both variants of all the strings. In my opinion, you should factor this
into one virAsprintf per function, with a call out to a helper function
to decide whether to add -m %s. Something along these lines, but
without the thread-safety hole of static storage:
virAsprintf(&cmd,
"lssyscfg -r lpar %s --filter lpar_names=%s -F lpar_id",
helper(system_type, managed_system), name) < 0
where:
static char *
helper(int system_type, const char *managed_system) {
static char unsafe[1024];
if (system_type == HMC)
virSprintf(unsafe, "-m %s", managed_system);
else
unsafe[0] = 0;
return unsafe;
}
Maybe even change over to virBuffer API instead of virAsprintf. But by
your current approach of duplicating the strings, in every method, you
risk a maintenance burden of a future change affecting one, but not
both, of two similar and long command lines.
Dan Berrange's pending patches to provide an improved exec wrapper may
be handy on this front, depending on whether phypExec can be rewritten
to take advantage of the memory management features it provides.
>
> - if (virAsprintf(&cmd,
> - "lssyscfg -r lpar -m %s -F lpar_id,state %s |grep -c "
> - "^[0-9]*", managed_system, state) < 0) {
> - virReportOOMError();
> - goto err;
> + if (system_type == HMC) {
> + if (virAsprintf(&cmd,
> + "lssyscfg -r lpar -m %s -F lpar_id,state %s |grep -c "
> + "^[0-9]*", managed_system, state) < 0) {
Ouch - a pre-existing bug. Since this command is being fed to a shell,
you need to properly quote the regex being fed to grep, so that it
doesn't get interpreted as a glob matching a literal file (such as ^0)
that happens to be in the same directory. That should be fixed in an
independent patch.
> + "lssyscfg -r lpar -m %s -F lpar_id,state %s | sed -e 's/,.*$//g'",
Here, a minor optimization - you don't need the g flag to that sed s///
expression, since the regex is anchored to the end of the string (you
can't replace more than one anchored expression per line). But
independent of this patch.
> @@ -1947,14 +2196,30 @@ phypUUIDTable_Push(virConnectPtr conn)
> ConnectionData *connection_data = conn->networkPrivateData;
> LIBSSH2_SESSION *session = connection_data->session;
> LIBSSH2_CHANNEL *channel = NULL;
> + char *username = NULL;
> struct stat local_fileinfo;
> char buffer[1024];
> int rc = 0;
> FILE *fd;
> size_t nread, sent;
> char *ptr;
> - char remote_file[] = "/home/hscroot/libvirt_uuid_table";
> char local_file[] = "./uuid_table";
> + char *remote_file = NULL;
> +
> + if (conn->uri->user != NULL) {
> + username = strdup(conn->uri->user);
> +
> + if (username == NULL) {
> + virReportOOMError();
> + goto err;
> + }
> + }
> +
> + if (virAsprintf(&remote_file, "/home/%s/libvirt_uuid_table", username)
> + < 0) {
> + virReportOOMError();
> + goto err;
> + }
>
> if (stat(local_file, &local_fileinfo) == -1) {
> VIR_WARN0("Unable to stat local file.");
Memory leak? Where does username get freed? virBuffer* may be more
handy than strdup'ing username just to copy it (virAsprintf) to another
malloc'd string then free it.
> @@ -2031,6 +2296,7 @@ phypUUIDTable_Pull(virConnectPtr conn)
> ConnectionData *connection_data = conn->networkPrivateData;
> LIBSSH2_SESSION *session = connection_data->session;
> LIBSSH2_CHANNEL *channel = NULL;
> + char *username = NULL;
> struct stat fileinfo;
> char buffer[1024];
> int rc = 0;
> @@ -2039,8 +2305,23 @@ phypUUIDTable_Pull(virConnectPtr conn)
> int amount = 0;
> int total = 0;
> int sock = 0;
> - char remote_file[] = "/home/hscroot/libvirt_uuid_table";
> char local_file[] = "./uuid_table";
> + char *remote_file = NULL;
> +
> + if (conn->uri->user != NULL) {
> + username = strdup(conn->uri->user);
> +
> + if (username == NULL) {
> + virReportOOMError();
> + goto err;
> + }
> + }
> +
> + if (virAsprintf(&remote_file, "/home/%s/libvirt_uuid_table", username)
> + < 0) {
> + virReportOOMError();
> + goto err;
> + }
>
> /* Trying to stat the remote file. */
> do {
Again, where does username get freed?
> +++ b/src/phyp/phyp_driver.h
> @@ -66,11 +66,19 @@ struct _phyp_driver {
> uuid_tablePtr uuid_table;
> virCapsPtr caps;
> int vios_id;
> +
> + /* system_type:
> + * 0 = hmc
> + * 127 = ivm
> + * */
> + int system_type;
Is this worth an enum? Although I'm probably okay with an int.
> char *managed_system;
> };
>
> int phypCheckSPFreeSapce(virConnectPtr conn, int required_size, char *sp);
>
> +int phypGetSystemType(virConnectPtr conn);
> +
> int phypGetVIOSPartitionID(virConnectPtr conn);
>
> virCapsPtr phypCapsInit(void);
Looking forward to v2.
--
Eric Blake eblake@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list