Re: [PATCH] SMP SNMP

I am attaching the new version.

On 01/27/2011 05:10 PM, Amos Jeffries wrote:
> On 28/01/11 00:07, Tsantilas Christos wrote:
>> The attached patch implements aggregation of SNMP responses, similar to
>> how we aggregate some cache manager stats.
>>
>> The code contains changes that allow us to share some of the classes
>> between Cache Manager and SNMP code:
>>
>> * implement the following base classes under the ipc directory/module:
>> - Ipc::Forwarder (ipc/Forwarder{.cc,.h} files)
>> - Ipc::Inquirer (ipc/Inquirer{.cc,.h} files)
>> - Ipc::Request (ipc/Request{.cc,.h} files)
>> - Ipc::Response (ipc/Response{.cc,.h} files)
>>
>> * fix the Mgr::Forwarder, Mgr::Inquirer, Mgr::Request and Mgr::Response
>> classes to be implemented as kid classes of the equivalent ICP::*
>> classes.
>>
>> Also implements for the SNMP the same mechanism used for cache manager:
>> The SNMP requests forwarder to coordinator which collects the statistics
>> from kids and aggregate them.
>>
>> This is a Measurement Factory project.
>
> Yay!
>
> minor niggle:
> please update the documentation blurb to say Ipc::* instead of ICP::*
>
>
> include/Range.h:
> * is this change able to be applied separately? it looks like a bug fix
> unrelated to the actual SMP changes.

I think it is part of this patch. This change required because the
"Range" class used in Snmp::Var and Snmp::Pdu classes

>
>
> configure.ac:
> * any particular reason for the "x" on src/snmpx/ ?
> (see makefile changes before answering)
> if not please rename. The SourceLayout plans call for all Squid SNMP
> support ending up in src/snmp/

I let it as is. We have already an libsnmp.a (the lowlevel snmp library)
under the (topdir)/snmplib directory. It is not bad idea to call the new
library libsnmpx.a

>
>
> Makefile.am:
> * could you rename the AC_CONDITIONAL() to ENABLE_SNMP from USE_SNMP
> please while altering the SNMP feature please?
>
Done

> * please also AC_DEFINE() a wrapper macro USE_SNMP and use it around all
> the new SNMP specific code.

I used the already defined SQUID_SNMP macro which is used in many other
places .

>
> * the Makefile.am common syntax for internal library variables seems to
> be name_LIBS. This will help avoid clashes with the third-party library
> definition $(SNMPLIB) when the SNMPX->SNMP conversion requested above is
> done.

I am not fully understand what you mean here.
I just rename the SNMPXLIB to SNMPX_LIBS

Also please see my comment about renaming snmpx to snmp.

>
> * please combine the SNMP_LIBS setup and SNMP_ALL_SOURCE/SNMP_SOURCE
> logics into one conditional block.

Are we sure we want it?
They are in different Makefile.am sections: Source files definitions and
SUBIRS definition....

>
>
> src/ipc/Request.cc:
> * file appears to be empty now. it can be removed.
> NP: if it is there for a global template instantiation then it is
> missing the explicit instantiation statement needed by MacOSX and some
> other BSD-derived.

I do not think it is needed any instantiation here.
I removed the Request.cc/Response.cc files

>
>
> src/ipc/Response.cc:
> * same as ipc/Request.cc.
removed

> * The operator<<() can be inlined in ipc/Request.h
OK done.

>
>
> src/snmp_core.cc:
> * CONF_ST_SWHIWM is a % and should not be combined. It is either
> identical or a set of distinct config values. We might get away with the
> min() value here, it looses some info though.

Grr... I do not know which is the best way to handle this one.

I put the min() value
>
> * same for CONF_ST_SWLOWM. we might get away with the min() value here
> as well. also with a loss of info.
And here I put the max() value.

I am expecting that in most cases the cache_swap_low/cache_swap_high
will have the same value for all kids so using min/max here will return
the correct value.

>
> * PERF_SYS_CURLRUEXP again is tricky to combine. best match would be a
> min() for oldest timestamp.

Currently it is just return "0".

> src/snmpx/Pdu.h:
> * aggrCount should be unsigned yes?
Fixed to be unsigned

> src/snmpx/Pdu.cc:
> * In Snmp::Pdu::aggregate() could you add a comment to the atAverage
> case please. Stating that Snmp::Pdu::fixAggregate() is where the
> mean-average division is done later.

done.

>
> * In Snmp::Pdu::fixAggregate() the if (!aggrCount) could become
> (aggrCount < 2) to avoid lots of loops and division by 1.

done.

> src/snmpx/Var.*:
> * "ipaddr" type for asIpAddress() and related does not appear to be
> defined anywhere. We do not use SMI_IPADDRESS within Squid anymore
> either so that whole lot of accessors can probably go.

True. Removed.

>
>
> Amos
>