Monday, May 28, 2018 7:23 AM
> It is hard for me to see why the code in ptpbase_mib.c needs to be in
> a seperate file from snmpd.c
The idea was to have all different MIBs in separate files. Then it is easy to
get a view on
which MIBs that are implemented and which code is MIB specific and which one is
general for the snmp and pmc.
In this first example object it is off course not that obvious but when 100 of
these inits
and handles are implemented, I think it is good to have them separated.
>> +long longObject = 0;
>> +size_t long_size = sizeof(long);
> This won't scale to many objects of different types.
No, but this was just an example. The idea was to have a number of globals of
different
types, which can be used for temp storage for the snmp requests.
> Can't see why this needs to be a callback. Just call run_pmc() directly.
There is no h-file for snmpd.c. I can off course add that but no other utility
program
file has one.
> This error is not propagated back to the caller.
No, I will look into that.
>> + case TLV_VERSION_NUMBER:
>> + mtd = (struct management_tlv_datum *) mgt->data;
>> + pr_err("VERSION_NUMBER %hhu", mtd->val);
> This is just for prototyping and debugging, right?
Yes, it is just for prototyping. This will be changed when the complete MIB
is implemented.
>> + longObject = mtd->val;
> Better to pass in a pointer to a data structure that holds the
> result. Then you can avoid the global.
Since the data type is different for the different objects, we picked this
alternative
for the first draft. It is also possible with a void pointer and then cast to
the preferred
type. Or are you talking about a struct holding all types of possible return
values?
>> +static int handle_tempMibObject(netsnmp_mib_handler *handler,
> Avoid camelCase please.
Yes, mib2c used this way. The code will be replaced when whole MIB is
implemented.
> /* no need for global: */
> long val;
> ...
> msg = pmc_cb("GET VERSION_NUMBER");
> if (!msg) {
> return SNMP_ERR_MUMBLE;
> }
> get_msg_val(msg, &val);
> msg_put(msg);
> err = snmp_set_var_typed_value(requests->requestvb,
> ASN_UNSIGNED,
> &val, sizeof(val));
> if (err) {
> ...
> }
> return SNMP_ERR_NOERROR;
The error handling will be updated and regarding the global, please look above.
>> + const oid tempMibObject_oid[] = { 1, 3, 6, 1, 2, 1, 241, 1, 2, 8, 1, 15
>> };
> Use proper defines rather than magic numbers please.
I will update
>> + netsnmp_register_scalar(netsnmp_create_handler_registration(
>> + "tempMibObject",
>> + handle_tempMibObject,
>> + tempMibObject_oid,
>> + OID_LENGTH(tempMibObject_oid),
>> + HANDLER_CAN_RONLY));
> Don't nest function calls like this. Also, this lacks error handling.
The code was generated from mib2c. I will update with errorhandling and without
the nested calls.
> +}
> +
>> +void init_ptpbaseMib(struct ptp_message* (*cb)(char *command))
>> +{
>> + pmc_cb = cb;
> Really don't see the point of this callback. Do you expect to pass in
> different values of 'cb' ?
Then, I can either add a h-file or include by external, what do you prefer?
/Anders
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linuxptp-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
