Re: MfD suggestions

> DS> The other suggestion relates to the 'object_get' routines

> DS> Currently, this code includes checks on the size of the buffer
> DS> passed in, and re-allocates memory if necessary (potentially
> DS> resulting in a memory leak, since the old memory doesn't appear
> DS> to be released).
>=20
> I=C2=B4m pretty sure there isn=C2=B4t a leak - the calling function shoul=
d detect
> if the pointer changed.

Ummm... I don't think it does.
The relevant bit of the myTable_interface.c file is:

case COLUMN_MYSTRING:
var->type =3D ASN_OCTET_STR;
rc =3D myString_get(rowreq_ctx, (char **) &var->val.string,
&var->val_len);
break;


So the 'myString_get' routine is being passed the buffer from the
varbind structure. If this buffer is pointing to some dynamically
allocated memory (but not enough to hold the myString value), then
the myString_get() routine will replace this with a larger buffer,
but never try to release the original one.

Now it may well be that this situation should never arise (since the
varbind will probabably start by referring to the internal in-line
buffer anyway). But *if* this buffer should be allocated dynamically
prior to calling the MfD code, then this approach will result in a
memory leak - I'm convinced of it.


But that's not really the main issue here.



> DS> Wouldn't it be clearer to tweak this 'object_get' interface slightly
> DS> to pass in the varbind structure, [...]
>=20
> Clearer to us, yes. Clearer to them? Not likely. One of the explicit goal=
s of
> the MFD code is to require a little net-snmp specific knowledge as possib=
le.

Yes - I understand (and accept) that.
What I'm suggesting is that a single API call (even one referring to a
Net-SNMP data structure) is probably easier to understand than a whole
lot of comparisons and calculations.

Compare:

int
myString_get(myTable_rowreq_ctx * rowreq_ctx,
char **myString_val_ptr_ptr,
size_t *myString_val_ptr_len_ptr)
{
if ((NULL =3D=3D (*myString_val_ptr_ptr))
|| ((*myString_val_ptr_len_ptr) <
rowreq_ctx->data.myString_len)) {
/*
* allocate space for myString data
*/
(*myString_val_ptr_ptr) =3D
malloc(rowreq_ctx->data.myString_len *
sizeof((*myString_val_ptr_ptr)[0]));
if (NULL =3D=3D (*myString_val_ptr_ptr)) {
snmp_log(LOG_ERR, "could not allocate memory\n");
return MFD_ERROR;
}
}
(*myString_val_ptr_len_ptr) =3D rowreq_ctx->data.myString_len;
memcpy((*myString_val_ptr_ptr), rowreq_ctx->data.myString,
(*myString_val_ptr_len_ptr) *
sizeof((*myString_val_ptr_ptr)[0]));

return MFD_SUCCESS;
}


with

int
myString_get(myTable_rowreq_ctx *rowreq_ctx,
netsnmp_variable_list *var)
{
return snmp_set_var_value( var, rowreq_ctx->data.myString,
rowreq_ctx->data.myString_len);
}



> The goal isn=C2=B4t to be efficient, it=C2=B4s to be easy to understand.

Granted.
But I'd say that the second of these is *much* easier to understand.
True - it mentions the dreaded word "SNMP". But it doesn't exactly
require a large amount of knowledge of SNMP internals - particularly
since the routine is generated automatically.

The current code avoids making any mention of SNMP. But in doing so,
it reveals a whole load of detail relating to the internal memory
management. This might well be familiar to most C programmers - but
it shouldn't be necessary for them to be bothered with it. Jumping
through hoops to get rid of all trace of SNMP, ends up complicating
the code unnecessarily, IMO.


Anyway, what do other people think?

Dave


-------------------------------------------------------
SF.Net email is Sponsored by the Better Software Conference & EXPO September
19-22, 2005 * San Francisco, CA * Development Lifecycle Practices
Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA
Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf
_______________________________________________
Net-snmp-coders mailing list
Net-snmp-coders@lists.sourceforge.net
https://lists.sourceforge.net/lists/...et-snmp-coders