On 06/18/2010 03:11 PM, Eduardo Otubo wrote:
> Hello Folks,
>
> This is the result of a couple of months of hard work. I added the storage
> management driver to the Power Hypervisor driver. This is a big but simple
> patch, it's just a new set of functions, nothing more. I could split it
> into multiple commits, but the feature freeze starts in some hours and I
> really reed this feature to be included in the next release.
>
> This patch includes:
> * Storage driver: The set of pool-* and vol-* functions.
> * attach-disk function.
> * Support for IVM on the new functions.
>
> I've been looking at this code for a long time, so I apologize now for the
> silly mistakes that might be present. Looking forward to see the comments.
>
> Thanks!
Yes, it's quite involved, but it's all code addition restricted to just
the phyp backend, so a single commit is acceptable to me. However,
breaking it into smaller commits may help getting a particular commit
through the review process faster (since most of your APIs don't depend
on each other, it would be reasonable to break it into 3-4 driver
additions per patch, rather than all 15 new driver callbacks in one go).
>
> ---
> src/phyp/phyp_driver.c | 1638 +++++++++++++++++++++++++++++++++++++++++++++++-
> src/phyp/phyp_driver.h | 52 ++
> 2 files changed, 1688 insertions(+), 2 deletions(-)
> diff --git a/src/phyp/phyp_driver.h b/src/phyp/phyp_driver.h
It's a shame that git lists files alphabetically, because I like to
review the headers first. I've reordered my reply accordingly ;)
> 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?
> +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.
> +char * phypGetStoragePoolXMLDesc(virStoragePoolPtr pool, unsigned int
flags);
Formatting nit: no space between the * and the function name.
>
> 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?
> +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 $(()).)
> + 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?
> + 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?
> +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"
> +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.
> +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),
...
> + 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, '\'');
> + 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.
> +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.
> + }
> + 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.
> +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)
> +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.)
> +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);
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.
> +
> + /* 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().
> + 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).
> +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..."
> +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).
> +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
> +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.
> +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.
> +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"').
> +int
> +phypNumOfStoragePools(virConnectPtr conn)
> +{
> + } else {
> + virBufferVSprintf(&buf, "lsvg");
> + }
> + virBufferVSprintf(&buf, "grep -c '^.*$'");
Missing a '|' before the grep.
> +}
> +virDrvOpenStatus
> +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.
--
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