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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org