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>:
> 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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 619 bytes)