On Thu, Oct 03, 2019 at 10:01:06AM +0200, Martijn van Duren wrote:
> On 10/3/19 9:21 AM, Sebastien Marie wrote:
> > On Thu, Sep 26, 2019 at 02:33:11PM +0200, Martijn van Duren wrote:
> >> On 9/26/19 9:54 AM, Martijn van Duren wrote:
> >>> Hello,
> >>>
> >>> I reckon this will be on of the last major additions.
> >>> Adding "snmp set" allows us to run snmpd's regress without installing
> >>> netsnmp. :-)
> >>>
> >>> Tested with snmpd's regress test.
> >>>
> >>> Majority of diff is moving oid/value parsing from snmp trap to a
> >>> separate function.
> >>>
> >>> OK?
> >>>
> >>> martijn@
> >
> > few comments.
> >
> > thanks.
> >
> >> Index: snmpc.c
> >> ===================================================================
> >> RCS file: /cvs/src/usr.bin/snmp/snmpc.c,v
> >> retrieving revision 1.11
> >> diff -u -p -r1.11 snmpc.c
> >> --- snmpc.c 18 Sep 2019 09:54:36 -0000 1.11
> >> +++ snmpc.c 26 Sep 2019 12:32:38 -0000
> >> @@ -666,19 +669,54 @@ snmpc_walk(int argc, char *argv[])
> >> }
> >>
> >> int
> >> +snmpc_set(int argc, char *argv[])
> >> +{
> >> + struct snmp_agent *agent;
> >> + struct ber_element *pdu, *varbind;
> >> + int errorstatus, errorindex;
> >> + int class;
> >> + unsigned type;
> >> +
> >> + if (argc < 4)
> >> + usage();
> >> + if ((agent = snmpc_connect(argv[0], "161")) == NULL)
> >> + err(1, "%s", snmp_app->name);
> >> + argc--;
> >> + argv++;
> >> +
> >> + if (argc < 3 || argc % 3 != 0)
> >> + usage();
> >> +
> >> + if (pledge("stdio", NULL) == -1)
> >> + err(1, "pledge");
> >> +
> >> + pdu = snmp_set(agent, snmpc_varbindparse(argc, argv));
> >> +
> >> + (void) ber_scanf_elements(pdu, "t{Sdd{e", &class, &type, &errorstatus,
> >> + &errorindex, &varbind);
> >> + if (errorstatus != 0)
> >> + snmpc_printerror((enum snmp_error) errorstatus,
> >> + argv[errorindex - 1]);
> >
> > is "errorindex - 1" the right index ?
>
> Yes: RFC 3416 section 4.1:
> A variable binding is identified by its index value. The first variable
> binding in a variable-binding list is index one, the second is index
> two, etc.
>
> I choose to use argv, because it's easier for the end user to see his
> own input for where he mistyped instead of a double parsed oid to -O
> based output.
>
> >
> > $ ./obj/snmp set 192.168.1.5 sysContact.0 s "test"
> > snmp: Can't parse oid 192.168.1.5: Not writable
> >
> > Note that the same pattern is used in others commands.
>
> Somehow the machines I test against don't return a not writable message.
> Tested against snmpd, net-snmpd, HP Laserjet.
> Could you show me a tcpdump -v output on this output?
> >
>
> >> +struct ber_element *
> >> +snmpc_varbindparse(int argc, char *argv[])
> >> +{
> >> + struct ber_oid oid, oidval;
> >> + struct in_addr addr4;
> >> + char *addr = (char *)&addr4;
> >> + char *str = NULL, *tmpstr, *endstr;
> >> + const char *errstr = NULL;
> >> + struct ber_element *varbind = NULL, *vblist = NULL;
> >> + int i, ret;
> >> + size_t strl, byte;
> >> + long long lval;
> >> +
> >> + if (argc % 3 != 0)
> >> + usage();
> >
> > if I don't mess myself, callers already checks that 'argc % 3 != 0'. So I
> > think
> > the condition is more a defense for programmer error that user error, and I
> > would use an assert(argc % 3 == 0) or abort().
>
> I removed these checks from snmpc_{set,trap}. Even less code. :-)
>
> Index: snmp.1
> ===================================================================
> RCS file: /cvs/src/usr.bin/snmp/snmp.1,v
> retrieving revision 1.6
> diff -u -p -r1.6 snmp.1
> --- snmp.1 18 Sep 2019 09:54:36 -0000 1.6
> +++ snmp.1 3 Oct 2019 07:59:07 -0000
> @@ -110,6 +110,28 @@
> .Ar agent
> .Op Ar oid
> .Nm
> +.Cm set
> +.Op Fl A Ar authpass
> +.Op Fl a Ar digest
> +.Op Fl c Ar community
> +.Op Fl E Ar ctxengineid
> +.Op Fl e Ar secengineid
> +.Op Fl K Ar localpriv
> +.Op Fl k Ar localauth
> +.Op Fl l Ar seclevel
> +.Op Fl n Ar ctxname
> +.Op Fl O Cm afnQqSvx
> +.Op Fl r Ar retries
> +.Op Fl t Ar timeout
> +.Op Fl u Ar user
> +.Op Fl v Ar version
> +.Op Fl X Ar privpass
> +.Op Fl x Ar cipher
> +.Op Fl Z Ar boots , Ns Ar time
> +.Ar agent
> +.Ar varoid type value
> +.Oo Ar varoid type value Oc ...
> +.Nm
> .Cm trap
> .Op Fl A Ar authpass
> .Op Fl a Ar digest
> @@ -182,6 +204,12 @@ This uses the
> subcommand internally to retrieve multiple MIBs at a time.
> This command is not available for
> .Fl v Cm 1 .
> +.It Cm set
> +Set one or more OID to a new value.
> +The triple
> +.Ar varoid , type , value
> +is described in
> +.Sx Data types .
> .It Cm trap
> Send a trap message to the
> .Ar agent .
> @@ -194,8 +222,8 @@ The
> .Ar trapoid
> is the identification OID used by the trap handler to determine its action.
> The triple
> -.Op Ar varoid , type, value
> -is described below
> +.Op Ar varoid , type , value
> +is described in
> .Sx Data types .
> This command is not available for
> .Fl v Cm 1 .
> Index: snmp.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/snmp/snmp.c,v
> retrieving revision 1.6
> diff -u -p -r1.6 snmp.c
> --- snmp.c 18 Sep 2019 09:54:36 -0000 1.6
> +++ snmp.c 3 Oct 2019 07:59:07 -0000
> @@ -249,6 +249,22 @@ fail:
> return NULL;
> }
>
> +struct ber_element *
> +snmp_set(struct snmp_agent *agent, struct ber_element *vblist)
> +{
> + struct ber_element *pdu;
> +
> + if ((pdu = ber_add_sequence(NULL)) == NULL)
> + return NULL;
> + if (ber_printf_elements(pdu, "tddd{e", BER_CLASS_CONTEXT,
> + SNMP_C_SETREQ, arc4random() & 0x7fffffff, 0, 0, vblist) == NULL) {
> + ber_free_elements(pdu);
Shouldn't this be ber_free_elements(vblist)?
If ber_printf_elements() fails pdu was already freed there.
Apart from that OK claudio@
--
:wq Claudio
