2011/4/18 Eric Blake <eblake@xxxxxxxxxx>:
> On 04/16/2011 12:18 AM, Matthias Bolte wrote:
>> 2011/4/14 Eric Blake <eblake@xxxxxxxxxx>:
>>> 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;
>
Interdiff is fine, ACK.
Matthias
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list