On 06/23/2010 04:49 PM, Eduardo Otubo wrote:
> This is the first patch, just over the semantic changes over the code itself.
> Added the whole storage management stack and fixed the echo $(( expr)) to just
> increment the variable in the return of the function.
>
> Hope we can get it acknowledged in time for the next release :)
> Thanks for all the comments so far!
>
>
> +static char *
> +phypGetLparProfile(virConnectPtr conn, int lpar_id)
> +{
> + ConnectionData *connection_data = conn->networkPrivateData;
> + phyp_driverPtr phyp_driver = conn->privateData;
> + LIBSSH2_SESSION *session = connection_data->session;
> + char *managed_system = phyp_driver->managed_system;
> + int system_type = phyp_driver->system_type;
> + int exit_status = 0;
> + char *cmd = NULL;
> + char *ret = NULL;
> + virBuffer buf = VIR_BUFFER_INITIALIZER;
> +
> + virBufferAddLit(&buf, "lssyscfg");
> + if (system_type == HMC)
> + virBufferVSprintf(&buf, " -m %s ", managed_system);
I removed a trailing space here...
> + virBufferVSprintf(&buf,
> + " -r prof --filter lpar_ids=%d -F name|head -n 1",
...since this line always provides a space.
> +static int
> +phypGetVIOSNextSlotNumber(virConnectPtr conn)
> +{
> + ConnectionData *connection_data = conn->networkPrivateData;
> + phyp_driverPtr phyp_driver = conn->privateData;
> + LIBSSH2_SESSION *session = connection_data->session;
> + char *managed_system = phyp_driver->managed_system;
> + int system_type = phyp_driver->system_type;
> + int vios_id = phyp_driver->vios_id;
> + int exit_status = 0;
> + char *char_ptr;
> + char *cmd = NULL;
> + char *ret = NULL;
> + char *profile = NULL;
> + int slot = 0;
> + virBuffer buf = VIR_BUFFER_INITIALIZER;
> +
> + if (!(profile = phypGetLparProfile(conn, vios_id))) {
> + VIR_ERROR("%s", "Unable to get VIOS profile name.");
New code should start clean, so I added _() here (basically, I squashed
in all the formatting changes from your original 2/2 patch that were
left out when rebasing your two patches to play 2/2 first into my commit
stream). I had to add it in a lot of places, to silence 'make
syntax-check'.
> + goto err;
> + }
> +
> + virBufferAddLit(&buf, "lssyscfg");
> +
> + if (system_type == HMC)
> + virBufferVSprintf(&buf, " -m %s ", managed_system);
> +
> + virBufferVSprintf(&buf, "-r prof --filter "
Even worse, for the non-HMC case, you try to execute "lssyscfg-r prof
..." (there is no lssyscfg-r program, you meant lssyscfg with a first
argument of -r), so I did a global search and replace to clean up all of
these commands to have consistent spacing.
> + "profile_names=%s -F virtual_eth_adapters,"
> + "virtual_opti_pool_id,virtual_scsi_adapters,"
> + "virtual_serial_adapters|sed -e 's/\"//g' -e "
> + "'s/,/\\n/g'|sed -e 's/\\(^[0-9][0-9]\\*\\).*$/\\1/'"
> + "|sort|tail -n 1", profile);
> +
> + if (virBufferError(&buf)) {
> + virBufferFreeAndReset(&buf);
> + virReportOOMError();
> + return -1;
> + }
> +
> + cmd = virBufferContentAndReset(&buf);
> +
> + ret = phypExec(session, cmd, &exit_status, conn);
> +
> + if (exit_status < 0 || ret == NULL)
> + goto err;
> +
> + if (virStrToLong_i(ret, &char_ptr, 10, &slot) == -1)
> + goto err;
> +
> + VIR_FREE(cmd);
> + VIR_FREE(ret);
> + return ++slot;
I did s/++slot/slot + 1/ since there is no need to modify slot in place
- it goes out of scope after this return.
> +int
> +phypAttachDevice(virDomainPtr domain, const char *xml)
> +{
I saw no reason for this to be a public function, so I removed it (and
many like it) from phyp_driver.h, and marked it static here. If some
file outside of phyp_driver.c needs to use it in the future, we can
worry about exporting it then (and proper exporting, even if it is an
internal-use only symbol, probably involves touching some .syms files).
> virDriver phypDriver = {
> VIR_DRV_PHYP, "PHYP", phypOpen, /* open */
> phypClose, /* close */
> @@ -1725,7 +2204,7 @@ virDriver phypDriver = {
> NULL, /* domainCreateWithFlags */
> NULL, /* domainDefineXML */
> NULL, /* domainUndefine */
> - NULL, /* domainAttachDevice */
> + phypAttachDevice, /* domainAttachDevice */
> NULL, /* domainAttachDeviceFlags */
> NULL, /* domainDetachDevice */
> NULL, /* domainDetachDeviceFlags */
> @@ -1779,6 +2258,1199 @@ virDriver phypDriver = {
> NULL, /* domainSnapshotDelete */
> };
>
> +virStorageDriver phypStorageDriver = {
I moved this (and phypDriver) later in the file, and marked them static,
to match with the fact that I marked a lot of functions static as part
of reducing scope.
Then it got to be quite unwieldy, so I'm now focusing on preparing a
patch series that tackles this project in a more incremental manner.
I'll post them for review soon (it no longer looks enough like your
original for me to feel justified with acking your version but pushing
mine, so I have to wait for you to confirm that my refactoring works).
--
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