Re: [libvirt] [PATCHv2 9/9] phyp: another simplification

On 04/16/2011 12:18 AM, Matthias Bolte wrote: > 2011/4/14 Eric Blake <eblake(a)redhat.com&gt;: >> Rather than copying and pasting lots of code, factor it into a >> single helper function. >> >> * src/phyp/phyp_driver.c (phypExecInt): New function. >> (phypGetVIOSPartitionID, phypNumDomainsGeneric, phypGetLparID) >> (phypGetLparMem, phypGetLparCPUGeneric, phypGetRemoteSlot) >> (phypGetVIOSNextSlotNumber, phypAttachDevice) >> (phypGetStoragePoolSize, phypStoragePoolNumOfVolumes) >> (phypNumOfStoragePools, phypInterfaceDestroy) >> (phypInterfaceDefineXML, phypInterfaceLookupByName) >> (phypInterfaceIsActive, phypNumOfInterfaces): Use it. >> --- >>  src/phyp/phyp_driver.c |  316 ++++++++++-------------------------------------- >>  1 files changed, 67 insertions(+), 249 deletions(-) > > Okay lets take a look at each instance if stricter parsing is safe or not. Thanks for doing that. > >> @@ -364,17 +368,7 @@ phypNumDomainsGeneric(virConnectPtr conn, unsigned int type) >>         virBufferVSprintf(&buf, " -m %s", managed_system); >>     virBufferVSprintf(&buf, " -F lpar_id,state %s |grep -c '^[0-9]*'", > Here the stricter parsing will be safe as the last grep returns a count. > > But I wonder about the last grep. I thought it would be there to count > the number of lines that start with a number, but it doesn't work: > > $ printf "aa\n22\n33\n" | grep -c '^[0-9]*' > 3 Oops - that's a definite bug; elsewhere in the file, we consistently use "[0-9][0-9]*" when matching a non-empty string of digits ([0-9]+ is not portable to POSIX BRE). > > In most cases we can see from the code that stricter parsing will be > safe, but in some places I'm not sure. > > So, as long as you don't have a PHYP system at hand to really test it, > I'd suggest that we stick to the relaxed parsing. Here's what I'm squashing in.  Do I need to send a v3, or is this interdiff sufficient for an ack? diff --git i/src/phyp/phyp_driver.c w/src/phyp/phyp_driver.c index fc4ad5c..6bb9b49 100644 --- i/src/phyp/phyp_driver.c +++ w/src/phyp/phyp_driver.c @@ -237,13 +237,16 @@ phypExecInt(LIBSSH2_SESSION *session, virBufferPtr buf, virConnectPtr conn,  {     char *str;     int ret; +    char *char_ptr;     str = phypExecBuffer(session, buf, &ret, conn, true);     if (!str || ret) {         VIR_FREE(str);         return -1;     } -    ret = virStrToLong_i(str, NULL, 10, result); +    ret = virStrToLong_i(str, &char_ptr, 10, result); +    if (ret == 0 && *char_ptr) +        VIR_WARN("ignoring suffix during integer parsing of '%s'", str);     VIR_FREE(str);     return ret;  } @@ -366,7 +369,7 @@ phypNumDomainsGeneric(virConnectPtr conn, unsigned int type)     virBufferAddLit(&buf, "lssyscfg -r lpar");     if (system_type == HMC)         virBufferVSprintf(&buf, " -m %s", managed_system); -    virBufferVSprintf(&buf, " -F lpar_id,state %s |grep -c '^[0-9]*'", +    virBufferVSprintf(&buf, " -F lpar_id,state %s |grep -c '^[0-9][0-9]*'",                       state);     phypExecInt(session, &buf, conn, &ndom);     return ndom;