Re: [libvirt] [PATCH] Adding support for IVM

On 06/04/2010 03:40 PM, Eduardo Otubo wrote: These changes were easier to review with 'git diff -b', and look reasonable up to this point... The two lists of VIR_FREE seem redundant. You could prime exit_status to -1, then blindly return exit_status in the err clause with the success clause falling through to the err clause. Or, since the only way to get to the err clause is after virReportOOMError, when you already know that cmd and ret are NULL, you could get rid of the err clause altogether and just use 'return -1' instead of 'goto err'. A lot of the patch is like this, adding a non-HMC version that merely omits the -m %s part of the string. That means the executable contains both variants of all the strings. In my opinion, you should factor this into one virAsprintf per function, with a call out to a helper function to decide whether to add -m %s. Something along these lines, but without the thread-safety hole of static storage: virAsprintf(&cmd, "lssyscfg -r lpar %s --filter lpar_names=%s -F lpar_id", helper(system_type, managed_system), name) < 0 where: static char * helper(int system_type, const char *managed_system) { static char unsafe[1024]; if (system_type == HMC) virSprintf(unsafe, "-m %s", managed_system); else unsafe[0] = 0; return unsafe; } Maybe even change over to virBuffer API instead of virAsprintf. But by your current approach of duplicating the strings, in every method, you risk a maintenance burden of a future change affecting one, but not both, of two similar and long command lines. Dan Berrange's pending patches to provide an improved exec wrapper may be handy on this front, depending on whether phypExec can be rewritten to take advantage of the memory management features it provides. Ouch - a pre-existing bug. Since this command is being fed to a shell, you need to properly quote the regex being fed to grep, so that it doesn't get interpreted as a glob matching a literal file (such as ^0) that happens to be in the same directory. That should be fixed in an independent patch. Here, a minor optimization - you don't need the g flag to that sed s/// expression, since the regex is anchored to the end of the string (you can't replace more than one anchored expression per line). But independent of this patch. Memory leak? Where does username get freed? virBuffer* may be more handy than strdup'ing username just to copy it (virAsprintf) to another malloc'd string then free it. Again, where does username get freed? Is this worth an enum? Although I'm probably okay with an int. Looking forward to v2. -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org