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

[Eric Blake]

On 04/16/2011 12:18 AM, Matthias Bolte wrote:
> 2011/4/14 Eric Blake <eblake at redhat.com>:
>> 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;

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110418/126772b5/attachment-0001.sig>