On 10/3/19 10:19 AM, Claudio Jeker wrote:
> 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.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.
Nope, the pdu has already been allocated and is the root of this
endeavour. So that needs to be freed.
vblist is most likely not linked into the pdu, so we can argue if it
should be freed separately.
It's a minor leak, since we most likely are heading for the exit
path, but it's worth thinking how we want to properly handle this (same
for snmp_get, snmp_getnext, snmp_trap, and snmp_bulkget).
I'd say we leave this for a different patch.
>
> Apart from that OK claudio@
>
