[libvirt] [PATCH 1/2] Adding Storage Management driver (semantic changes)

[Eric Blake]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20100624/8539d53e/attachment-0001.sig>