On tor, 2007-05-17 at 11:12 +0100, Michael Granzow wrote:
> I'd therefore like to suggest the following patch (output of svn diff):
>
>
> -------------------------BEGIN PATCH-------------------------------
> Index: testing/RUNTESTS
> ===================================================================
> --- testing/RUNTESTS (revision 16373)
> +++ testing/RUNTESTS (working copy)
> @@ -1,4 +1,4 @@
> -#!/bin/sh
> +#!/bin/bash
No. This chunk is bad, but please see below.
> #
> # RUNTESTS [-h]...
> #
> Index: agent/mibgroup/ucd-snmp/proxy.c
> ===================================================================
> --- agent/mibgroup/ucd-snmp/proxy.c (revision 16373)
> +++ agent/mibgroup/ucd-snmp/proxy.c (working copy)
> @@ -549,9 +549,13 @@
> }
>
> /*
> - * update the original request varbinds with the results
> + * update the original request varbinds with the results.
> + *
> + * mg 17-May-2007: This must be done in the case of error
> + * packages as well, cf. section 4.1.2 of rfc 1067.
This is nice as a commit comment but I do not like it in the code. I
hope that would be OK with you? Also, 1067 seems to be a tad old as base
for new work, I think that at least rfc 1157 should be used.
> */
> - } else for (var = vars, request = requests;
> + }
> + for (var = vars, request = requests;
> request && var;
> request = request->next, var = var->next_variable) {
> /*
But when I look at the code it all seems a little odd.
You are quite right that it should handle errors in a better way but I
am unconvinced that this is the right way to solve this, especially if
the GetRequest the proxy got was a v2 request that contained entities
outside the proxied case. I am somewhat surprised that I can't find more
about this case in rfc 3584 but I might just fail to look hard enough.
> Index: agent/snmp_agent.c
> ===================================================================
> --- agent/snmp_agent.c (revision 16373)
> +++ agent/snmp_agent.c (working copy)
> @@ -3482,7 +3482,6 @@
> netsnmp_request_set_error_idx(netsnmp_request_info *request,
> int error_value, int idx)
> {
> - int i;
> netsnmp_request_info *req = request;
>
> if (!request || !request->agent_req_info)
> @@ -3491,10 +3490,12 @@
> /*
> * Skip to the indicated varbind
> */
> - for ( i=2; i<idx; i++) {
> - req = req->next;
> + while (1) {
> if (!req)
> return SNMPERR_NO_VARS;
> + if (req->index == idx)
> + break;
> + req = req->next;
> }
If the purpose of this chunk is to clarify, wouldn't a result of
while (req && req->index != idx)
req = req->next;
if (!req)
return SNMPERR_NO_VARS;
be even clearer?
>
> return _request_set_error(req, request->agent_req_info->mode,
> --------------------------END PATCH--------------------------------
>
> (The change to RUNTEST was needed on my Ubuntu Linux machine
> because /bin/sh is a link to /bin/dash which demands more POSIX
> compliance than bash;
Hmm. It seems as if the problematic part in RUNTEST is that SIGCHLD
causes it to terminate due to the
trap "exit 1;" ... 17
part. Does somebody remember why that change was done or could we just
remove the 17?
If I removes that 17 then I ran it succesfully with bourne_sh, dash,
bash
> the change to snmp_agent.c is just intended to
> make the code clearer -- which you may or may not agree it does...).
See comment above :-)
> This patch has been tested on Linux 2.6.17, both manually to cover the
> scenario described above and automatically using the test suite that
> comes with net-snmp.
/MF
-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
Net-snmp-coders mailing list
Net-snmp-coders@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/net-snmp-coders
