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

On 06/18/2010 03:11 PM, Eduardo Otubo wrote: 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). It's a shame that git lists files alphabetically, because I like to review the headers first. I've reordered my reply accordingly ;) 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? Technically, new code should probably not be using ATTRIBUTE_UNUSED on flags; more on that at the implementation. flags); Formatting nit: no space between the * and the function name. 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? $(()) 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 $(()).) 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? 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? ... 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" 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. ... 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), ... 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, '\''); Another case of a missing string translation. How come 'make syntax-check' isn't catching it? 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. ... Another example where you could simplify to one "lslv ..." string with a little bit of refactoring. key and ret don't overlap (since ret was freshly malloc'd). Use memcpy instead for speed. 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) ... 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.) 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. 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. Again, spdef and pool don't overlap, so you should use memcpy(). 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). 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..." Another place for virCheckFlags(0, NULL) memcpy 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). ... Save a process: ...|sed '1d'|sed '1d' is equivalent to: ...|sed 2d ... Here, rather than using sed '1d' twice,... ...just subtract 2 from nvolumes here in C code. ... 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. virCheckFlags(0, NULL) (won't comment on it any more for this round of review...) ... 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"'). Missing a '|' before the grep. 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(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org