[libvirt] [PATCH] phyp: Adding storage management driver

[Eduardo Otubo]

>
>> index 80ff0c3..2606fe4 100644
>> --- a/src/phyp/phyp_driver.h
>> +++ b/src/phyp/phyp_driver.h
>> @@ -75,6 +75,58 @@ struct _phyp_driver {
>>       char *managed_system;
>>   };
>>
>> +
>> +/*
>> + * Storage functions
>> + * */
>> +virStorageVolPtr
>> +phypStorageVolCreateXML(virStoragePoolPtr pool, const char *xmldesc,
>> +                        unsigned int flags ATTRIBUTE_UNUSED);
>
> Do these new functions really need to be exported (by modifying
> src/libvirt_private.syms or some such)?  I'd almost rather see them just
> declared private within phyp_driver.h, since the only time they will be
> invoked outside of phyp_driver.c is via the driver context.
>
> I only found two instances of the string phypStorageVolCreateXML in your
> patch - this declaration, and its implementation.  But no one calls it,
> and you haven't yet registered it in a driver callback array.  Did you
> forget to set .volCreateXML properly in your new virStorageDriver?

Yes, I just forgot to insert these functions to the callback array.

>
>> +virStoragePoolPtr phypStoragePoolCreateXML(virConnectPtr conn,
>> +                                           const char *xml,
>> +                                           unsigned int flags
>> +                                           ATTRIBUTE_UNUSED);
>
> Technically, new code should probably not be using ATTRIBUTE_UNUSED on
> flags; more on that at the implementation.

As said further ahead in the email, I removed all the ATTRIBUTE_UNUSED
and added the virCheckFlags(0, NULL);

>
>> +char * phypGetStoragePoolXMLDesc(virStoragePoolPtr pool, unsigned int
> flags);
>
> Formatting nit: no space between the * and the function name.

The last commit on my local branch is to indent all the code with GNU
Indent, I don't know what happened here, but I'll fix it.

>
>>
>> diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c
>> index cefb8be..77a74ef 100644
>> --- a/src/phyp/phyp_driver.c
>> +++ b/src/phyp/phyp_driver.c
>> @@ -56,6 +56,7 @@
>>   #include "virterror_internal.h"
>>   #include "uuid.h"
>>   #include "domain_conf.h"
>> +#include "storage_conf.h"
>>   #include "nodeinfo.h"
>>
>>   #include "phyp_driver.h"
>> @@ -1680,6 +1681,466 @@ phypDomainSetCPU(virDomainPtr dom, unsigned int nvcpus)
>>
>>   }
>>
>> +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);
>> +    virBufferVSprintf(&buf,
>> +                      " -r prof --filter lpar_ids=%d -F name|head -n 1",
>> +                      lpar_id);
>> +    if (virBufferError(&buf)) {
>> +        virBufferFreeAndReset(&buf);
>> +        virReportOOMError();
>> +        return NULL;
>> +    }
>> +    cmd = virBufferContentAndReset(&buf);
>> +
>> +    ret = phypExec(session, cmd,&exit_status, conn);
>> +
>> +    if (exit_status<  0 || ret == NULL)
>> +        goto err;
>> +
>> +    char *char_ptr = strchr(ret, '\n');
>> +
>> +    if (char_ptr)
>> +        *char_ptr = '\0';
>> +
>
> Hmm.  The 'head -n 1' you appended to cmd will guarantee that there will
> be at most one newline, and if present, it will be the last byte in the
> returned string.  Meanwhile, some older systems didn't support 'head -n
> 1' (even though POSIX now requires it).  How much more output does
> lssyscfg typically produce?  In other words, are we saving ourselves
> some malloc overhead by trimming the stdout via head, or is it just
> simpler to avoid the portability hassle, and skip piping the results
> through head, since this strchr does the same thing anyway?

I run my tests on old versions of HMC, IVM (both run Bash) and VIOS
(which run rksh). Both versions are compatible with "head -n 1", then
newer versions should not have problem with this.

Truth is, the reason for me to be using "head -n 1" is because a LPAR
can have more than one profile (but at least one), and it doesn't
matter which one I modify to change the configuration, since I actually
do it. So I pick the first one just for a matter of simplicity. I could
pick the second or third, but it could get me some more trouble on
parsing.

>
>> +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.");
>> +        goto err;
>> +    }
>> +
>> +    virBufferAddLit(&buf, "echo $((`lssyscfg");
>
> $(()) is another construct guaranteed by POSIX but not portable to all
> current /bin/sh.  Are we assured that phyp can only run on systems with
> decent /bin/sh, or should we be worried about portability to Solaris (in
> which case, use expr instead of $(()).)

Since this commands run only on HMC's and IVM's (which are IBM AIX) I
don't need to worry about the portability to Solaris. I am not aware
about any HMC or IVM implememted on Solaris or Linux.

>
>> +    if (system_type == HMC)
>> +        virBufferVSprintf(&buf, " -m %s ", managed_system);
>> +    virBufferVSprintf(&buf, "-r prof --filter "
>> +                      "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` +1 ))", profile);
>
> Ouch.  If the result of the `` command substitution begins with 0, you
> have a problem with octal numbers.  Remember, $((010 + 1)) is not the
> same as $((10 + 1)).  Perhaps you can modify the sed commands used in
> your script to strip leading 0?

Yes, I can strip the leading zero using sed, but I hardly believe that
would be a such a return. But better fix this now than in the client
screen. :)

>
>> +    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;
>
> And, rather than doing $(()) (or expr) to do +1 in the shell, where you
> have to worry about octal in the first place, why not just output the
> last value as-is (that is, drop "echo $((`" and "` +1 ))" from cmd),
> then do +1 in C code?

This is an option, but I really like to isolate the most I can on the
shell side, returning the final value for the function. I've been
keeping this pattern over all the code, so did the same here


>
>> +static int
>> +phypCreateServerSCSIAdapter(virConnectPtr conn)
>> +{
> ...
>> +    virBufferVSprintf(&buf, "-r prof --filter lpar_ids=%d,profile_names=%s"
>> +                      " -F virtual_scsi_adapters|sed -e s/\"//g",
>
> Won't work.  In addition to escaping " for inclusion in a C string, you
> also need to quote it from unbalanced use in shell code.  In other
> words, you want to execute
>    ...|sed -e 's/"//g'
> which requires a C string of:
>    "...|sed -e 's/\"//g'"
>
> or you want to execute
>    ...|sed -e s/\"//g
> which requires a C string of:
>    "...|sed -e s/\\\"//g"

Yes, you're right. Fixed.

>
>> +static char *
>> +phypGetVIOSFreeSCSIAdapter(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 *cmd = NULL;
>> +    char *ret = NULL;
>> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
>> +
>> +    if (system_type == HMC) {
>> +        virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ",
>> +                          managed_system, vios_id);
>> +        virBufferVSprintf(&buf, "'lsmap -all -field svsa backing -fmt ,'");
>> +    } else {
>> +        virBufferVSprintf(&buf, "lsmap -all -field svsa backing -fmt ,");
>> +    }
>> +    virBufferVSprintf(&buf, "|grep -v ',[^.*]'|head -n 1|sed -e 's/,//g'");
>
> Here, you can save a couple of processes, and avoid any portability
> problems with 'head -n 1' at the same time.
> ...|grep -v ',[^.*]'|head -n 1|sed -e 's/,//g'
> says to exclude lines that contain ,. or ,*, then print the first
> remaining line with commas removed.  But sed can do all of that:
> ...|sed -n '/,[^.*]/! {
>   s/,//g
>   p
>   q
> }'
>
> Yes, the embedded newlines are required for strict POSIX compatibility.
>   But, since this is not the critical path, and not everyone is a sed
> wizard, I'm okay with the 3-process approach instead of the
> do-it-all-in-one sed script.

Acutally, I gotta check all the versions of shell in all the versions
of HMC/VIOS and IVM to get it all in a better way.

>
>> +int
>> +phypAttachDevice(virDomainPtr domain, const char *xml)
>> +{
> ...
>> +    if (!
>> +        (vios_name =
>> +         phypGetLparNAME(session, managed_system, vios_id, conn))) {
>> +        VIR_ERROR("%s", "Unable to get VIOS name");
>
> Here, you could use VIR_ERROR0(str) instead of VIR_ERROR("%s", str),
> since your string has no % symbols.  But you do need to translate the
> string (with _() markings),

Ok, fixed!

>
> ...
>> +    if (system_type == HMC) {
>> +        virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ",
>> +                          managed_system, vios_id);
>> +        virBufferVSprintf(&buf, "'mkvdev -vdev %s -vadapter %s'",
>> +                          dev->data.disk->src, scsi_adapter);
>> +    } else {
>> +        virBufferVSprintf(&buf, "mkvdev -vdev %s -vadapter %s",
>> +                          dev->data.disk->src, scsi_adapter);
>> +    }
>
> Here's some ideas for sharing more of the code (making it easier to
> adjust the command in just one place, if you find down the road that
> mkvdev needs an argument change):
>
> char *end = "";
> if (system_type == HMC) {
>      virBufferVSprintf(&buf, "viosvrcmd ... -c '", ...);
>      end = "'";
> }
> virBufferVSprintf(&buf, "mkvdev -vdev %s -vadapter %s%s",
>                    dev->data.disk->src, scsi_adapter, end);
>
>
> OR
>
> if (system_type == HMC)
>      virBufferVSprintf(&buf, "viosvrcmd ... -c '", ...);
> virBufferVSprintf(&buf, "mkvdev ... -vadapter %s", ... scsi_adapter);
> if (system_type == HMC)
>      virBufferAddChar(&buf, '\'');

I spent some time thinking on how to optimize the calls like this one.
I think I like the second one. Fixing all the commands like this.

>
>
>> +    if (!(profile = phypGetLparProfile(conn, domain->id))) {
>> +        VIR_ERROR("%s", "Unable to get VIOS profile name.");
>> +        goto err;
>> +    }
>
> Another case of a missing string translation.  How come 'make
> syntax-check' isn't catching it?
>
>> +    virBufferVSprintf(&buf,
>> +                      "-r prof -i 'name=%s,lpar_id=%d,"
>> +                      "\"virtual_scsi_adapters=%s,%d/client/%d/%s/0\"'",
>> +                      domain->name, domain->id, ret, slot,
>
> Since we're passing a lot of things through shell, are you sure that
> domain->name (and all the other strings that you substitute via %s) will
> never contain ", $, or any other character special to shell processing
> that would throw your cmd for a loop?  I know we have various XML schema
> requirements, but I haven't checked whether a domain name is something
> that libvirt has already guaranteed to be restricted to a reasonable
> subset (alphanumeric, ., _, -) or if it allows absolute freedom in
> naming with consequences to everyone else dealing with arbitrary names.

Yes, you're right. I used escape_special_characters() to scape some
occasional trash.

>
>> +static int
>> +phypVolumeGetKey(virConnectPtr conn, char *key, const char *name)
> ...
>> +    if (system_type == HMC) {
>> +        virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ",
>> +                          managed_system, vios_id);
>> +        virBufferVSprintf(&buf, "'lslv %s -field lvid'", name);
>> +    } else {
>> +        virBufferVSprintf(&buf, "lslv %s -field lvid", name);
>
> Another example where you could simplify to one "lslv ..." string with a
> little bit of refactoring.

Yes, I refactored all of this kind.

>
>> +    }
>> +    virBufferVSprintf(&buf, "|sed -e 's/^LV IDENTIFIER://' -e 's/\\ //g'");
>> +
>> +    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;
>> +
>> +    char *char_ptr = strchr(ret, '\n');
>> +
>> +    if (char_ptr)
>> +        *char_ptr = '\0';
>> +
>> +    if (memmove(key, ret, PATH_MAX) == NULL)
>
> key and ret don't overlap (since ret was freshly malloc'd).  Use memcpy
> instead for speed.

Ok.

>
>> +static char *
>> +phypGetStoragePoolDevice(virConnectPtr conn, char *name)
>> +{
>> +    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 *cmd = NULL;
>> +    char *ret = NULL;
>> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
>> +
>> +    if (system_type == HMC) {
>> +        virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ",
>> +                          managed_system, vios_id);
>> +        virBufferVSprintf(&buf, "'lssp -detail -sp %s -field name'", name);
>> +    } else {
>> +        virBufferVSprintf(&buf, "lssp -detail -sp %s -field name", name);
>> +    }
>> +    virBufferVSprintf(&buf, "|sed '1d'|sed -e 's/\\ //g'");
>
> Two sed processes:
>    ...|sed '1d'|sed -e 's/\\ //g'
> can be simplified to one:
>    ...|sed '1d; s/\\ //g'
>
> Also, '\ ' is not a portable sed escape sequence.  Did you mean to use
> the C string "s/\\\\ //g" for the shell line 's/\\ //g', in order to
> delete backslash-space sequences from the output?  (multiple instances
> of this pattern in your patch)

No, I meant to use the C string 's/\\ //g' for the shell line 's/\ //g'
in order to delete white spaces.

>
>> +static int
>> +phypBuildVolume(virConnectPtr conn, const char *lvname, const char *spname,
>> +                unsigned int capacity, char *key)
>> +{
> ...
>> +    if (exit_status<  0) {
>> +        VIR_ERROR("%s\"%s\"", "Unable to create Volume. Reason: ", ret);
>
> Missing translation, and unusual capititalization in the middle of the
> sentence.  I'd just use:
>
> VIR_ERROR(_("unable to create volume: %s"), ret);
>
> (GNU Coding Standards demand starting with lower case; however libvirt
> is not a GNU project, so we aren't bound by that rule.  Right now, we
> aren't very consistent yet on whether error messages start with upper or
> lower case, so although I use lower, I won't reject a patch that uses
> upper.)

Ok.

>
>> +virStorageVolPtr
>> +phypStorageVolCreateXML(virStoragePoolPtr pool, const char *xml,
>> +                        unsigned int flags ATTRIBUTE_UNUSED)
>> +{
>> +
>> +    virStorageVolDefPtr voldef = NULL;
>> +    virStoragePoolDefPtr spdef = NULL;
>> +    virStorageVolPtr vol = NULL;
>> +    char *key = NULL;
>
> I'm wary of any new code that declares flags with ATTRIBUTE_UNUSED - it
> means we aren't checking for unknown flags, which makes it harder to
> actually define a new flag down the road.  To get around that, you
> should be using:
>
> virCheckFlags(0, NULL);

I removed all the occurrences of ATTRIBUTE_UNUSED and added
virCheckFlags(0, NULL);

>
> which checks that flags is explicitly 0.
>
>> +
>> +    if (VIR_ALLOC_N(key, PATH_MAX)<  0) {
>> +        virReportOOMError();
>> +        return NULL;
>> +    }
>
> Ouch.  On GNU/Hurd, PATH_MAX is unlimited, so it is not a size that you
> can safely malloc.  Is there a better bound for the maximum key size
> that you expect, even if that means adding #define MAX_KEY_SIZE
> (1024*4), or whatever value is a better reasonable limit?  At any rate,
> there's a reason that 'git grep "ALLOC.*PATH_MAX"' returns no hits, so
> this needs adjusting.

I remember that somewhere in the past I saw this PATH_MAX in the
libvirt, perhaps now you have optimized this value and git grep wont
show anything. I defined in phyp_driver.h MAX_KEY_SIZE as being
(1024*4), I believe this value is enough to handle the Power Storage
keys.

>
>> +
>> +    /* Filling spdef manually
>> +     * */
>> +    if (pool->name != NULL) {
>> +        spdef->name = pool->name;
>> +    } else {
>> +        VIR_ERROR("%s", "Unable to determine storage pool's name.");
>> +        goto err;
>> +    }
>> +
>> +    if (memmove(spdef->uuid, pool->uuid, VIR_UUID_BUFLEN) == NULL) {
>
> Again, spdef and pool don't overlap, so you should use memcpy().

Ok.

>
>> +    if ((voldef = virStorageVolDefParseString(spdef, xml)) == NULL) {
>> +        VIR_ERROR("%s", "Error parsing volume XML.");
>> +        goto err;
>> +    }
>> +
>> +    /* checking if this name already exists on this system */
>> +    if (phypVolumeLookupByName(pool, voldef->name) != NULL) {
>> +        VIR_ERROR("%s", "StoragePool name already exists.");
>
> More strings to mark for translation (I'll quit commenting on this for
> this round of review, although there's probably more instances of it
> throughout the patch).

All fixed.

>
>> +virStorageVolPtr
>> +phypVolumeLookupByPath(virConnectPtr conn, const char *volname)
>> +{
>> +    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 *cmd = NULL;
>> +    char *spname = NULL;
>> +    char *key = NULL;
>> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
>> +
>> +    if (system_type == HMC) {
>> +        virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ",
>> +                          managed_system, vios_id);
>> +        virBufferVSprintf(&buf, "'lslv %s -field vgname'", volname);
>> +    } else {
>> +        virBufferVSprintf(&buf, "lslv %s -field vgname", volname);
>> +    }
>> +    virBufferVSprintf(&buf,
>> +                      "|sed -e 's/^VOLUME\\ GROUP://g' -e 's/\\ //g'");
>
> Another instance of a non-portable sed escape '\ '.  But this time, I
> see enough context that it looks like you are trying to just delete
> spaces (and not backslash-space sequences).  In which case, use either:
>    shell: ...|sed -e s/\^VOLUME\ GROUP://g -e...
>    C-string: "...|sed -e s/\\^VOLUME\\ GROUP://g -e..."
> or
>    shell: ...|sed -e 's/^VOLUME GROUP://g' -e...
>    C-string: "...|sed -e '/^VOLUME GROUP://g' -e..."

I choose the first one, just get all calls in the same pattern.

>
>> +char *
>> +phypVolumeGetXMLDesc(virStorageVolPtr vol,
>> +                     unsigned int flags ATTRIBUTE_UNUSED)
>> +{
>> +    virStorageVolDef voldef;
>> +    memset(&voldef, 0, sizeof(virStorageVolDef));
>
> Another place for virCheckFlags(0, NULL)
>
>> +
>> +    if (memmove(pool.uuid, sp->uuid, VIR_UUID_BUFLEN) == NULL) {
>
> memcpy
>
>> +
>> +virStorageVolPtr
>> +phypVolumeLookupByName(virStoragePoolPtr pool, const char *volname)
>> +{
>> +
>> +    char key[PATH_MAX];
>
> Stack-allocating PATH_MAX bytes is just as non-portable as malloc'ing
> that many bytes; but here, you've got precedence in existing libvirt
> source (ultimately, I mean to sweep the source code to clean that all
> up).  Are you sure we don't have a better bound on the maximum key
> length?  And actually, looking at src/datatypes.c, the const char *key
> argument is a bit of a misnomer; the docs call it uuid, and we DO have a
> fixed bound for UUID length, which is much smaller than PATH_MAX (we
> also have VIR_UUID_STRING_BUFLEN defined in libvirt.h).

As you told above, I replaced all PATH_MAX for MAX_KEY_SIZE

>
>> +int
>> +phypStoragePoolListVolumes(virStoragePoolPtr pool, char **const volumes,
>> +                           int nvolumes)
>> +{
> ...
>> +    virBufferVSprintf(&buf, "|sed '1d'|sed '1d'");
>
> Save a process:
>    ...|sed '1d'|sed '1d'
> is equivalent to:
>    ...|sed 2d

This is an odd behaviour. On my bash, (Ubuntu 10.04), sed '2d' and sed
'1d'|sed '1d' removes the second line of the stream. But the on the
HMC, sed '1d'|sed '1' removes the first two lines of the stream and sed
'2d' removes the second line. And what I need is to remove the first
two lines, so keeping sed '1d'|sed '1d'.

>
>> +int
>> +phypStoragePoolNumOfVolumes(virStoragePoolPtr pool)
>> +{
> ...
>> +    virBufferVSprintf(&buf, "|sed '1d'|sed '1d'|grep -c '^.*$'");
>
> Here, rather than using sed '1d' twice,...
>
>> +
>> +    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,&nvolumes) == -1)
>> +        goto err;
>
> ...just subtract 2 from nvolumes here in C code.

Wise!

>
>> +int
>> +phypDestroyStoragePool(virStoragePoolPtr pool)
>> +{
> ...
>> +    if (system_type == HMC) {
>> +        virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ",
>> +                          managed_system, vios_id);
>> +        virBufferVSprintf(&buf, "'rmsp %s'", pool->name);
>> +    } else {
>> +        virBufferVSprintf(&buf, "'rmsp %s'", pool->name);
>> +    }
>
> Looks like you typo'd the IVM line (left the '' in, so you will fail
> trying to call the command "rmsp name" rather than the intended command
> "rmsp" with argument "name").  Another reason why refactoring your code
> to share just a single rmsp C string will help avoid mistakes like this.

Fixed as you suggested up above.

>
>> +virStoragePoolPtr
>> +phypStoragePoolCreateXML(virConnectPtr conn,
>> +                         const char *xml,
>> +                         unsigned int flags ATTRIBUTE_UNUSED)
>> +{
>> +
>> +    virStoragePoolDefPtr def = NULL;
>> +    virStoragePoolPtr sp = NULL;
>
> virCheckFlags(0, NULL) (won't comment on it any more for this round of
> review...)
>
>> +virStoragePoolPtr
>> +phypGetStoragePoolLookUpByUUID(virConnectPtr conn,
>> +                               const unsigned char *uuid)
>> +{
> ...
>> +        if (STREQLEN((char *) local_uuid, (char *) uuid, VIR_UUID_BUFLEN)) {
>
> Here, it is simpler to avoid the casts, by doing:
>
> if (!memcmp(local_uuid, uuid, VIR_UUID_BUFLEN)) {
>
> It also matches existing style in the rest of libvirt ('git grep
> "STREQLEN.*VIR_UUID"' vs. 'git grep -i "cmp.*VIR_UUID_BUFLEN"').

Ok.

>
>> +int
>> +phypNumOfStoragePools(virConnectPtr conn)
>> +{
>> +    } else {
>> +        virBufferVSprintf(&buf, "lsvg");
>> +    }
>> +    virBufferVSprintf(&buf, "grep -c '^.*$'");
>
> Missing a '|' before the grep.

Ok.

>
>> +}
>> +	
>> +phypStorageOpen(virConnectPtr conn ATTRIBUTE_UNUSED,
>> +                virConnectAuthPtr auth ATTRIBUTE_UNUSED,
>> +                int flags ATTRIBUTE_UNUSED)
>
> Add a newline between the closing } of one function and the start of
> another.
>
> Overall, my review was mainly focused on style and shell portability.  I
> don't have access to a phyp setup at the moment, so I can't really test
> if your various command lines make sense (I'm assuming they do).
> Towards the end, I kept on spotting (and ignoring) the same issues
> again, so remember to do global searches when addressing a comment,
> rather than just at the place where I made the comment.  But in general,
> this looks promising, and hopefully we can get things turned around fast
> enough to decide whether this is worth including in libvirt 0.8.2.

All the commands are tested and studied directly on the HMC/VIOS and
IVM befores inserting on C code, so they really work :) All the issues
you pointed were fixed in all worldwide code (not just the storage
driver)

Will send another patch right away after reading all the comments and
making all the properly fixes.

Thanks for all the comments!

-- 
Eduardo Otubo
Software Engineer
Linux Technology Center
IBM Systems & Technology Group
Mobile: +55 19 8135 0885
eotubo at linux.vnet.ibm.com