Re: snmpd rename context to pdutype
On Fri, May 07, 2021 at 04:18:50PM +0200, Martijn van Duren wrote:
> When moving the traphandler to the snmpe process I overlooked the fact
> that "type" is being saved inside the switch statement under the
> sm_context name. RFC3411 talks about pduType, and the name context means
> something completely different in the v3 world.
>
> The diff below moves our naming closer to the RFCs, which should
> hopefully prevent further confusion in the future.
>
> While here I made the debug output print the pduType in a human readable
> format.
>
> The invalid varbind check can be simplified a simple "{}" in the
> ober_scanf_elements allowing me to just drop the type variable.
>
> OK?
I tested it and the diff looks good and legit for me.
> martijn@
>
> Index: snmp.h
> ===
> RCS file: /cvs/src/usr.sbin/snmpd/snmp.h,v
> retrieving revision 1.16
> diff -u -p -r1.16 snmp.h
> --- snmp.h30 Jun 2020 17:11:49 - 1.16
> +++ snmp.h7 May 2021 14:17:12 -
> @@ -77,7 +77,7 @@ enum snmp_version {
> SNMP_V3 = 3
> };
>
> -enum snmp_context {
> +enum snmp_pdutype {
> SNMP_C_GETREQ = 0,
> SNMP_C_GETNEXTREQ = 1,
> SNMP_C_GETRESP = 2,
> Index: snmpd.h
> ===
> RCS file: /cvs/src/usr.sbin/snmpd/snmpd.h,v
> retrieving revision 1.94
> diff -u -p -r1.94 snmpd.h
> --- snmpd.h 5 Feb 2021 10:30:45 - 1.94
> +++ snmpd.h 7 May 2021 14:17:12 -
> @@ -384,7 +384,7 @@ struct snmp_message {
> socklen_tsm_slen;
> int sm_sock_tcp;
> int sm_aflags;
> - int sm_type;
> + enum snmp_pdutypesm_pdutype;
> struct event sm_sockev;
> char sm_host[HOST_NAME_MAX+1];
> in_port_tsm_port;
> @@ -405,7 +405,6 @@ struct snmp_message {
>
> /* V1, V2c */
> char sm_community[SNMPD_MAXCOMMUNITYLEN];
> - int sm_context;
>
> /* V3 */
> long longsm_msgid;
> Index: snmpe.c
> ===
> RCS file: /cvs/src/usr.sbin/snmpd/snmpe.c,v
> retrieving revision 1.70
> diff -u -p -r1.70 snmpe.c
> --- snmpe.c 22 Feb 2021 11:31:09 - 1.70
> +++ snmpe.c 7 May 2021 14:17:12 -
> @@ -41,6 +41,7 @@
> #include "mib.h"
>
> void snmpe_init(struct privsep *, struct privsep_proc *, void *);
> +const char *snmpe_pdutype2string(enum snmp_pdutype);
> int snmpe_parse(struct snmp_message *);
> void snmpe_tryparse(int, struct snmp_message *);
> int snmpe_parsevarbinds(struct snmp_message *);
> @@ -194,6 +195,36 @@ snmpe_bind(struct address *addr)
> return (-1);
> }
>
> +const char *
> +snmpe_pdutype2string(enum snmp_pdutype pdutype)
> +{
> + static char unknown[sizeof("Unknown (4294967295)")];
> +
> + switch (pdutype) {
> + case SNMP_C_GETREQ:
> + return "GetRequest";
> + case SNMP_C_GETNEXTREQ:
> + return "GetNextRequest";
> + case SNMP_C_GETRESP:
> + return "Response";
> + case SNMP_C_SETREQ:
> + return "SetRequest";
> + case SNMP_C_TRAP:
> + return "Trap";
> + case SNMP_C_GETBULKREQ:
> + return "GetBulkRequest";
> + case SNMP_C_INFORMREQ:
> + return "InformRequest";
> + case SNMP_C_TRAPV2:
> + return "SNMPv2-Trap";
> + case SNMP_C_REPORT:
> + return "Report";
> + }
> +
> + snprintf(unknown, sizeof(unknown), "Unknown (%u)", pdutype);
> + return unknown;
> +}
> +
> int
> snmpe_parse(struct snmp_message *msg)
> {
> @@ -202,7 +233,6 @@ snmpe_parse(struct snmp_message *msg)
> struct ber_element *a;
> long longver, req;
> long longerrval, erridx;
> - unsigned int type;
> u_intclass;
> char*comn;
> char*flagstr, *ctxname;
> @@ -271,15 +301,15 @@ snmpe_parse(struct snmp_message *msg)
> goto fail;
> }
>
> - if (ober_scanf_elements(msg->sm_pdu, "t{e", , , ) != 0)
> + if (ober_scanf_elements(msg->sm_pdu, "t{e", , &(msg->sm_pdutype),
> + ) != 0)
> goto parsefail;
>
> /* SNMP PDU context */
> if (class != BER_CLASS_CONTEXT)
> goto parsefail;
>
> - msg->sm_type = type;
> - switch (type) {
> + switch (msg->sm_pdutype) {
> case SNMP_C_GETBULKREQ:
> if (msg->sm_version == SNMP_V1) {
> stats->snmp_inbadversions++;
> @@ -294,7 +324,7 @@ snmpe_parse(struct snmp_message *msg)
> /* FALLTHROUGH */
>
> case SNMP_C_GETNEXTREQ:
> - if (type ==
Re: snmpd rename context to pdutype
ping
On Fri, 2021-05-07 at 16:18 +0200, Martijn van Duren wrote:
> When moving the traphandler to the snmpe process I overlooked the fact
> that "type" is being saved inside the switch statement under the
> sm_context name. RFC3411 talks about pduType, and the name context means
> something completely different in the v3 world.
>
> The diff below moves our naming closer to the RFCs, which should
> hopefully prevent further confusion in the future.
>
> While here I made the debug output print the pduType in a human readable
> format.
>
> The invalid varbind check can be simplified a simple "{}" in the
> ober_scanf_elements allowing me to just drop the type variable.
>
> OK?
>
> martijn@
>
> Index: snmp.h
> ===
> RCS file: /cvs/src/usr.sbin/snmpd/snmp.h,v
> retrieving revision 1.16
> diff -u -p -r1.16 snmp.h
> --- snmp.h 30 Jun 2020 17:11:49 - 1.16
> +++ snmp.h 7 May 2021 14:17:12 -
> @@ -77,7 +77,7 @@ enum snmp_version {
> SNMP_V3 = 3
> };
>
> -enum snmp_context {
> +enum snmp_pdutype {
> SNMP_C_GETREQ = 0,
> SNMP_C_GETNEXTREQ = 1,
> SNMP_C_GETRESP = 2,
> Index: snmpd.h
> ===
> RCS file: /cvs/src/usr.sbin/snmpd/snmpd.h,v
> retrieving revision 1.94
> diff -u -p -r1.94 snmpd.h
> --- snmpd.h 5 Feb 2021 10:30:45 - 1.94
> +++ snmpd.h 7 May 2021 14:17:12 -
> @@ -384,7 +384,7 @@ struct snmp_message {
> socklen_t sm_slen;
> int sm_sock_tcp;
> int sm_aflags;
> - int sm_type;
> + enum snmp_pdutype sm_pdutype;
> struct event sm_sockev;
> char sm_host[HOST_NAME_MAX+1];
> in_port_t sm_port;
> @@ -405,7 +405,6 @@ struct snmp_message {
>
> /* V1, V2c */
> char sm_community[SNMPD_MAXCOMMUNITYLEN];
> - int sm_context;
>
> /* V3 */
> long long sm_msgid;
> Index: snmpe.c
> ===
> RCS file: /cvs/src/usr.sbin/snmpd/snmpe.c,v
> retrieving revision 1.70
> diff -u -p -r1.70 snmpe.c
> --- snmpe.c 22 Feb 2021 11:31:09 - 1.70
> +++ snmpe.c 7 May 2021 14:17:12 -
> @@ -41,6 +41,7 @@
> #include "mib.h"
>
> void snmpe_init(struct privsep *, struct privsep_proc *, void *);
> +const char *snmpe_pdutype2string(enum snmp_pdutype);
> int snmpe_parse(struct snmp_message *);
> void snmpe_tryparse(int, struct snmp_message *);
> int snmpe_parsevarbinds(struct snmp_message *);
> @@ -194,6 +195,36 @@ snmpe_bind(struct address *addr)
> return (-1);
> }
>
> +const char *
> +snmpe_pdutype2string(enum snmp_pdutype pdutype)
> +{
> + static char unknown[sizeof("Unknown (4294967295)")];
> +
> + switch (pdutype) {
> + case SNMP_C_GETREQ:
> + return "GetRequest";
> + case SNMP_C_GETNEXTREQ:
> + return "GetNextRequest";
> + case SNMP_C_GETRESP:
> + return "Response";
> + case SNMP_C_SETREQ:
> + return "SetRequest";
> + case SNMP_C_TRAP:
> + return "Trap";
> + case SNMP_C_GETBULKREQ:
> + return "GetBulkRequest";
> + case SNMP_C_INFORMREQ:
> + return "InformRequest";
> + case SNMP_C_TRAPV2:
> + return "SNMPv2-Trap";
> + case SNMP_C_REPORT:
> + return "Report";
> + }
> +
> + snprintf(unknown, sizeof(unknown), "Unknown (%u)", pdutype);
> + return unknown;
> +}
> +
> int
> snmpe_parse(struct snmp_message *msg)
> {
> @@ -202,7 +233,6 @@ snmpe_parse(struct snmp_message *msg)
> struct ber_element *a;
> long long ver, req;
> long long errval, erridx;
> - unsigned int type;
> u_int class;
> char*comn;
> char*flagstr, *ctxname;
> @@ -271,15 +301,15 @@ snmpe_parse(struct snmp_message *msg)
> goto fail;
> }
>
> - if (ober_scanf_elements(msg->sm_pdu, "t{e", , , ) != 0)
> + if (ober_scanf_elements(msg->sm_pdu, "t{e", ,
> &(msg->sm_pdutype),
> + ) != 0)
> goto parsefail;
>
> /* SNMP PDU context */
> if (class != BER_CLASS_CONTEXT)
> goto parsefail;
>
> - msg->sm_type = type;
> - switch (type) {
> + switch (msg->sm_pdutype) {
> case SNMP_C_GETBULKREQ:
> if (msg->sm_version == SNMP_V1) {
> stats->snmp_inbadversions++;
> @@ -294,7 +324,7 @@ snmpe_parse(struct snmp_message *msg)
>
snmpd rename context to pdutype
When moving the traphandler to the snmpe process I overlooked the fact
that "type" is being saved inside the switch statement under the
sm_context name. RFC3411 talks about pduType, and the name context means
something completely different in the v3 world.
The diff below moves our naming closer to the RFCs, which should
hopefully prevent further confusion in the future.
While here I made the debug output print the pduType in a human readable
format.
The invalid varbind check can be simplified a simple "{}" in the
ober_scanf_elements allowing me to just drop the type variable.
OK?
martijn@
Index: snmp.h
===
RCS file: /cvs/src/usr.sbin/snmpd/snmp.h,v
retrieving revision 1.16
diff -u -p -r1.16 snmp.h
--- snmp.h 30 Jun 2020 17:11:49 - 1.16
+++ snmp.h 7 May 2021 14:17:12 -
@@ -77,7 +77,7 @@ enum snmp_version {
SNMP_V3 = 3
};
-enum snmp_context {
+enum snmp_pdutype {
SNMP_C_GETREQ = 0,
SNMP_C_GETNEXTREQ = 1,
SNMP_C_GETRESP = 2,
Index: snmpd.h
===
RCS file: /cvs/src/usr.sbin/snmpd/snmpd.h,v
retrieving revision 1.94
diff -u -p -r1.94 snmpd.h
--- snmpd.h 5 Feb 2021 10:30:45 - 1.94
+++ snmpd.h 7 May 2021 14:17:12 -
@@ -384,7 +384,7 @@ struct snmp_message {
socklen_tsm_slen;
int sm_sock_tcp;
int sm_aflags;
- int sm_type;
+ enum snmp_pdutypesm_pdutype;
struct event sm_sockev;
char sm_host[HOST_NAME_MAX+1];
in_port_tsm_port;
@@ -405,7 +405,6 @@ struct snmp_message {
/* V1, V2c */
char sm_community[SNMPD_MAXCOMMUNITYLEN];
- int sm_context;
/* V3 */
long longsm_msgid;
Index: snmpe.c
===
RCS file: /cvs/src/usr.sbin/snmpd/snmpe.c,v
retrieving revision 1.70
diff -u -p -r1.70 snmpe.c
--- snmpe.c 22 Feb 2021 11:31:09 - 1.70
+++ snmpe.c 7 May 2021 14:17:12 -
@@ -41,6 +41,7 @@
#include "mib.h"
voidsnmpe_init(struct privsep *, struct privsep_proc *, void *);
+const char *snmpe_pdutype2string(enum snmp_pdutype);
int snmpe_parse(struct snmp_message *);
voidsnmpe_tryparse(int, struct snmp_message *);
int snmpe_parsevarbinds(struct snmp_message *);
@@ -194,6 +195,36 @@ snmpe_bind(struct address *addr)
return (-1);
}
+const char *
+snmpe_pdutype2string(enum snmp_pdutype pdutype)
+{
+ static char unknown[sizeof("Unknown (4294967295)")];
+
+ switch (pdutype) {
+ case SNMP_C_GETREQ:
+ return "GetRequest";
+ case SNMP_C_GETNEXTREQ:
+ return "GetNextRequest";
+ case SNMP_C_GETRESP:
+ return "Response";
+ case SNMP_C_SETREQ:
+ return "SetRequest";
+ case SNMP_C_TRAP:
+ return "Trap";
+ case SNMP_C_GETBULKREQ:
+ return "GetBulkRequest";
+ case SNMP_C_INFORMREQ:
+ return "InformRequest";
+ case SNMP_C_TRAPV2:
+ return "SNMPv2-Trap";
+ case SNMP_C_REPORT:
+ return "Report";
+ }
+
+ snprintf(unknown, sizeof(unknown), "Unknown (%u)", pdutype);
+ return unknown;
+}
+
int
snmpe_parse(struct snmp_message *msg)
{
@@ -202,7 +233,6 @@ snmpe_parse(struct snmp_message *msg)
struct ber_element *a;
long longver, req;
long longerrval, erridx;
- unsigned int type;
u_intclass;
char*comn;
char*flagstr, *ctxname;
@@ -271,15 +301,15 @@ snmpe_parse(struct snmp_message *msg)
goto fail;
}
- if (ober_scanf_elements(msg->sm_pdu, "t{e", , , ) != 0)
+ if (ober_scanf_elements(msg->sm_pdu, "t{e", , &(msg->sm_pdutype),
+ ) != 0)
goto parsefail;
/* SNMP PDU context */
if (class != BER_CLASS_CONTEXT)
goto parsefail;
- msg->sm_type = type;
- switch (type) {
+ switch (msg->sm_pdutype) {
case SNMP_C_GETBULKREQ:
if (msg->sm_version == SNMP_V1) {
stats->snmp_inbadversions++;
@@ -294,7 +324,7 @@ snmpe_parse(struct snmp_message *msg)
/* FALLTHROUGH */
case SNMP_C_GETNEXTREQ:
- if (type == SNMP_C_GETNEXTREQ)
+ if (msg->sm_pdutype == SNMP_C_GETNEXTREQ)
stats->snmp_ingetnexts++;
if (!(msg->sm_aflags & ADDRESS_FLAG_READ)) {
msg->sm_errstr = "read requests disabled";
@@ -308,7
