tasklwIP - A Lightweight TCP/IP stack - Tasks: task #15072, SNMP support for notifications and...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

task #15072: SNMP support for notifications and informs (v2c)

Submitter:  Ognjen Bjelica <ognjenbjelica>
Submitted:  Tue 23 Oct 2018 01:11:19 PM UTC
   
 
Category:  None Should Start On:  Tue 23 Oct 2018 12:00:00 AM UTC
Should be Finished on:  Tue 23 Oct 2018 12:00:00 AM UTC Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  None Percent Complete:  0%
Open/Closed:  Closed Planned Release:  None
Effort:  0.00

Jump to the original submission

Wed 07 Nov 2018 08:06:25 PM UTC, comment #16: 

Pushed your patch. If you implement a statemachine for informs, please open a new patch entry.
Thanks for providing your implementation!

Dirk Ziegelmeier <dziegel>
Group administrator
Fri 26 Oct 2018 06:14:58 PM UTC, comment #15: 

I cleaned up your patch, but I do not have the time to test it at the moment. I add it here as a basis for further discussion.

Note the patch also misses "real" parsing of the GET response frame, the internal SNMP message struct (struct snmp_request) is simply passed to the user.

There is no retry handling implemented. A statemachine that tries to deliver an inform, implements a response timeout and retry handling would be very useful.


(file #45309)

Dirk Ziegelmeier <dziegel>
Group administrator
Thu 25 Oct 2018 06:43:17 AM UTC, comment #14: 

The struct snmp_msg_trap should remain in snmp_traps.c - it is not a good idea to export an internal structure just to transport "isInform" and "snmpVersion". I think the API needs to be changed. There should be similar functions as for traps, e.g.

err_t snmp_send_trap_generic(s32_t generic_trap);
err_t snmp_send_inform_generic(s32_t generic_trap);

err_t snmp_send_trap_specific(s32_t specific_trap, struct snmp_varbind *varbinds);
err_t snmp_send_inform_specific(s32_t specific_trap, struct snmp_varbind *varbinds, s32_t *ptr_request_id);

err_t snmp_send_trap(const struct snmp_obj_id* oid, s32_t generic_trap, s32_t specific_trap, struct snmp_varbind *varbinds);
err_t snmp_send_inform(const struct snmp_obj_id* oid, s32_t generic_trap, s32_t specific_trap, struct snmp_varbind *varbinds, s32_t *ptr_request_id);


Dirk Ziegelmeier <dziegel>
Group administrator
Wed 24 Oct 2018 05:38:09 PM UTC, comment #13: 

In snmp.h, you declare a function

err_t snmp_send_trap_or_notification_or_inform_generic(struct snmp_msg_trap trap_msg, const struct snmp_obj_id eoid, s32_t generic_trap, s32_t specific_trap, struct snmp_varbind *varbinds);

which takes "struct snmp_msg_trap" as a parameter. This struct is defined inside snmp_traps.c, so how is this supposed to work???

Dirk Ziegelmeier <dziegel>
Group administrator
Wed 24 Oct 2018 02:23:30 PM UTC, comment #12: 

On a second look, you could add a goto to the cleanup end, or you could try to skip snmp_prepare_outbound_frame() for informs and then just return?

Anyway, is it correct to call snmp_inform_callback for all GetResponse PDUs?

Plus, if snmp_inform_callback is NULL, you continue to handle the GetResponse, which seems really wrong.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 24 Oct 2018 01:21:33 PM UTC, comment #11: 

I still don't know why you need ERR_SKIP. If you never test for it, you can just return any other error? If I don't understand it, how can I propose anything?

Simon Goldschmidt <goldsimon>
Group administrator
Wed 24 Oct 2018 01:04:59 PM UTC, comment #10: 

Please let me know your proposal for ERR_SKIP.
Other 2 issues are fixed now.

(file #45281)

Ognjen Bjelica <ognjenbjelica>
Wed 24 Oct 2018 12:38:47 PM UTC, comment #9: 

Sorry I didn't see you latest response in time. Now I understand your point regarding err.h. I agree, it will be removed. I will send you new version of patch tomorrow.
Regarding ERR_SKIP, I have introduced it in order to skip rest of the processing inside snmp_receive() - snmp_msg.h Do you have better suggestion?
ERR_IS_FATAL is artifact of a manual upgrade from 2.0.3 to 2.1.0. I will remove it.

Ognjen Bjelica <ognjenbjelica>
Wed 24 Oct 2018 12:27:25 PM UTC, comment #8: 

- You cannot define new errors in err.h just to implement SNMP informs (plus, why do you add 'ERR_SKIP' if you never test for it?).
- ERR_IS_FATAL has been removed quite a while ago.
- 'err_status_t' seems to be a 'private' type to implement SNMP things. This doesn't belong to err.h.

In other words, you should not have to touch err.h to implement what you want. And I don't think I will accept a patch that does touch it :-)

Simon Goldschmidt <goldsimon>
Group administrator
Wed 24 Oct 2018 12:19:26 PM UTC, comment #7: 

You are right regarding ignoring results. While porting our changes from 2.0.3 to 2.1.0 I have failed to realize that BUILD_EXEC will affect our modifications as well.
Regarding the unused stuff in err.h, it's no longer unused. Please see:

struct snmp_msg_trap
{
  /* source enterprise ID (sysObjectID) */
  const struct snmp_obj_id *enterprise;
  /* source IP address, raw network order format */
  ip_addr_t sip;
  /* generic trap code */
  u32_t gen_trap;
  /* specific trap code */
  u32_t spc_trap;
  /* timestamp */
  u32_t ts;
  /* snmp_version */
  u32_t snmp_version;

  /* output trap lengths used in ASN encoding */
  /* encoding pdu length */
  u16_t pdulen;
  /* encoding community length */
  u16_t comlen;
  /* encoding sequence length */
  u16_t seqlen;
  /* encoding varbinds sequence length */
  u16_t vbseqlen;

  /* error status */
  err_status_t error_status;
  /* error index */
  s32_t error_index;
  /* inform */
  u8_t is_inform;
};


  /* error status */
  err_status_t error_status;


I will implement new patch soon.

Ognjen Bjelica <ognjenbjelica>
Wed 24 Oct 2018 11:56:47 AM UTC, comment #6: 

Ok, now the functions using BUILD_EXEC() have return values, but you ignore them. As a result, e.g. snmp_send_msg() will send out a broken frame if something goes wrong in snmp_trap_header_enc() or snmp_trap_header_enc_pdu().

Plus, the unused stuff in err.h is still present.

Dirk Ziegelmeier <dziegel>
Group administrator
Wed 24 Oct 2018 11:46:21 AM UTC, comment #5: 

Please try new patch. I wasn't able to find exactly the same settings as ones for gcc in the compiler we are using, but I've found similar ones. I think that all the issues should be fixed now. If you detect any more warnings please let me know.

(file #45279)

Ognjen Bjelica <ognjenbjelica>
Wed 24 Oct 2018 08:26:21 AM UTC, comment #4: 

OK, I will do that. Currently we are using proprietary compiler.

Ognjen Bjelica <ognjenbjelica>
Wed 24 Oct 2018 08:11:49 AM UTC, comment #3: 

Please check you patch again and read/check all the issues I reported. Most of them are not fixed!

- usage of BUILD_EXEC() in functions returning void
- unused stuff in err.h
- instantiation of local variables in code (struct snmp_obj_id snmp_trap_oid =  { 0 }) in snmp_send_trap_or_notification_or_inform_generic()

I recommend you to crank up your warning settings (gcc: -Wall, -pedantic) and use warnings->errors when compiling (gcc: -Werror) it will seriously improve code quality!

Dirk Ziegelmeier <dziegel>
Group administrator
Wed 24 Oct 2018 07:36:11 AM UTC, comment #2: 

Sorry it seems that I didn't exported right commit. It has been fixed in later commits. I will post new patch soon. Sorry for inconvenience.

Ognjen Bjelica <ognjenbjelica>
Wed 24 Oct 2018 07:24:46 AM UTC, comment #1: 

The patch contains changes in err.h which seem to be unused, e.g. typedef ... err_status_t and #define ERR_IS_FATAL()

The patch also contains lots of lines with duplicated BUILD_EXEC() macros, e.g. "BUILD_EXEC( BUILD_EXEC( snmp_ans1_enc_tlv(pbuf_stream, &tlv) ) );"

The patch also has a lot of compile errors, since you use "return" statements in functions with return type "void", e.g. snmp_trap_header_enc_v1_specific, snmp_trap_header_enc_v2c_specific,
snmp_trap_header_en.

In function snmp_send_trap_or_notification_or_inform_generic() you declare variables AFTER statements (struct snmp_obj_id snmp_trap_oid =  { 0 };)


Complete compile errors from MSVC:


"W:\lwip\lwip\build\ALL_BUILD.vcxproj" (Standardziel) (1) ->
"W:\lwip\lwip\build\contrib\ports\win32\example_app\example_app.vcxproj" (Standardziel) (3) ->
"W:\lwip\lwip\build\contrib\ports\win32\example_app\lwipallapps.vcxproj" (Standardziel) (4) ->
(ClCompile Ziel) ->
  ..\..\..\..\..\src\apps\snmp\snmp_traps.c(379): warning C4047: 'initializing' : 'u16_t' differs in levels of indirection from 'void *' [W:\lwip\lwip\build\contrib\ports\win32\example_app\lwipallapps.vcxproj]
  ..\..\..\..\..\src\apps\snmp\snmp_traps.c(738): warning C4098: 'snmp_trap_header_enc_v1_specific' : 'void' function returning a value [W:\lwip\lwip\build\contrib\ports\win32\example_app\lwipallapps.vcxproj]
  ..\..\..\..\..\src\apps\snmp\snmp_traps.c(739): warning C4098: 'snmp_trap_header_enc_v1_specific' : 'void' function returning a value [W:\lwip\lwip\build\contrib\ports\win32\example_app\lwipallapps.vcxproj]
  ..\..\..\..\..\src\apps\snmp\snmp_traps.c(745): warning C4098: 'snmp_trap_header_enc_v1_specific' : 'void' function returning a value [W:\lwip\lwip\build\contrib\ports\win32\example_app\lwipallapps.vcxproj]
  ..\..\..\..\..\src\apps\snmp\snmp_traps.c(746): warning C4098: 'snmp_trap_header_enc_v1_specific' : 'void' function returning a value [W:\lwip\lwip\build\contrib\ports\win32\example_app\lwipallapps.vcxproj]
  ..\..\..\..\..\src\apps\snmp\snmp_traps.c(751): warning C4098: 'snmp_trap_header_enc_v1_specific' : 'void' function returning a value [W:\lwip\lwip\build\contrib\ports\win32\example_app\lwipallapps.vcxproj]
  ..\..\..\..\..\src\apps\snmp\snmp_traps.c(752): warning C4098: 'snmp_trap_header_enc_v1_specific' : 'void' function returning a value [W:\lwip\lwip\build\contrib\ports\win32\example_app\lwipallapps.vcxproj]
  ..\..\..\..\..\src\apps\snmp\snmp_traps.c(759): warning C4098: 'snmp_trap_header_enc_v1_specific' : 'void' function returning a value [W:\lwip\lwip\build\contrib\ports\win32\example_app\lwipallapps.vcxproj]
  ..\..\..\..\..\src\apps\snmp\snmp_traps.c(760): warning C4098: 'snmp_trap_header_enc_v1_specific' : 'void' function returning a value [W:\lwip\lwip\build\contrib\ports\win32\example_app\lwipallapps.vcxproj]
  ..\..\..\..\..\src\apps\snmp\snmp_traps.c(765): warning C4098: 'snmp_trap_header_enc_v1_specific' : 'void' function returning a value [W:\lwip\lwip\build\contrib\ports\win32\example_app\lwipallapps.vcxproj]
  ..\..\..\..\..\src\apps\snmp\snmp_traps.c(766): warning C4098: 'snmp_trap_header_enc_v1_specific' : 'void' function returning a value [W:\lwip\lwip\build\contrib\ports\win32\example_app\lwipallapps.vcxproj]
  ..\..\..\..\..\src\apps\snmp\snmp_traps.c(771): warning C4098: 'snmp_trap_header_enc_v1_specific' : 'void' function returning a value [W:\lwip\lwip\build\contrib\ports\win32\example_app\lwipallapps.vcxproj]
  ..\..\..\..\..\src\apps\snmp\snmp_traps.c(772): warning C4098: 'snmp_trap_header_enc_v1_specific' : 'void' function returning a value [W:\lwip\lwip\build\contrib\ports\win32\example_app\lwipallapps.vcxproj]
  ..\..\..\..\..\src\apps\snmp\snmp_traps.c(790): warning C4098: 'snmp_trap_header_enc_v2c_specific' : 'void' function returning a value [W:\lwip\lwip\build\contrib\ports\win32\example_app\lwipallapps.vcxproj]
  ..\..\..\..\..\src\apps\snmp\snmp_traps.c(791): warning C4098: 'snmp_trap_header_enc_v2c_specific' : 'void' function returning a value [W:\lwip\lwip\build\contrib\ports\win32\example_app\lwipallapps.vcxproj]
  ..\..\..\..\..\src\apps\snmp\snmp_traps.c(796): warning C4098: 'snmp_trap_header_enc_v2c_specific' : 'void' function returning a value [W:\lwip\lwip\build\contrib\ports\win32\example_app\lwipallapps.vcxproj]
  ..\..\..\..\..\src\apps\snmp\snmp_traps.c(797): warning C4098: 'snmp_trap_header_enc_v2c_specific' : 'void' function returning a value [W:\lwip\lwip\build\contrib\ports\win32\example_app\lwipallapps.vcxproj]
  ..\..\..\..\..\src\apps\snmp\snmp_traps.c(802): warning C4098: 'snmp_trap_header_enc_v2c_specific' : 'void' function returning a value [W:\lwip\lwip\build\contrib\ports\win32\example_app\lwipallapps.vcxproj]
  ..\..\..\..\..\src\apps\snmp\snmp_traps.c(803): warning C4098: 'snmp_trap_header_enc_v2c_specific' : 'void' function returning a value [W:\lwip\lwip\build\contrib\ports\win32\example_app\lwipallapps.vcxproj]
  ..\..\..\..\..\src\apps\snmp\snmp_traps.c(816): warning C4142: benign redefinition of type [W:\lwip\lwip\build\contrib\ports\win32\example_app\lwipallapps.vcxproj]
  ..\..\..\..\..\src\apps\snmp\snmp_traps.c(821): warning C4098: 'snmp_trap_header_enc' : 'void' function returning a value [W:\lwip\lwip\build\contrib\ports\win32\example_app\lwipallapps.vcxproj]
  ..\..\..\..\..\src\apps\snmp\snmp_traps.c(826): warning C4098: 'snmp_trap_header_enc' : 'void' function returning a value [W:\lwip\lwip\build\contrib\ports\win32\example_app\lwipallapps.vcxproj]
  ..\..\..\..\..\src\apps\snmp\snmp_traps.c(827): warning C4098: 'snmp_trap_header_enc' : 'void' function returning a value [W:\lwip\lwip\build\contrib\ports\win32\example_app\lwipallapps.vcxproj]
  ..\..\..\..\..\src\apps\snmp\snmp_traps.c(831): warning C4098: 'snmp_trap_header_enc' : 'void' function returning a value [W:\lwip\lwip\build\contrib\ports\win32\example_app\lwipallapps.vcxproj]
  ..\..\..\..\..\src\apps\snmp\snmp_traps.c(832): warning C4098: 'snmp_trap_header_enc' : 'void' function returning a value [W:\lwip\lwip\build\contrib\ports\win32\example_app\lwipallapps.vcxproj]
  ..\..\..\..\..\src\apps\snmp\snmp_traps.c(835): warning C4013: 'snmp_trap_header_enc_pdu' undefined; assuming extern returning int [W:\lwip\lwip\build\contrib\ports\win32\example_app\lwipallapps.vcxproj]


"W:\lwip\lwip\build\ALL_BUILD.vcxproj" (Standardziel) (1) ->
"W:\lwip\lwip\build\contrib\ports\win32\example_app\example_app.vcxproj" (Standardziel) (3) ->
"W:\lwip\lwip\build\contrib\ports\win32\example_app\lwipallapps.vcxproj" (Standardziel) (4) ->
(ClCompile Ziel) ->
  ..\..\..\..\..\src\apps\snmp\snmp_traps.c(686): error C2059: syntax error : 'if' [W:\lwip\lwip\build\contrib\ports\win32\example_app\lwipallapps.vcxproj]
  ..\..\..\..\..\src\apps\snmp\snmp_traps.c(686): error C2059: syntax error : '}' [W:\lwip\lwip\build\contrib\ports\win32\example_app\lwipallapps.vcxproj]
  ..\..\..\..\..\src\apps\snmp\snmp_traps.c(686): error C2059: syntax error : 'return' [W:\lwip\lwip\build\contrib\ports\win32\example_app\lwipallapps.vcxproj]
  ..\..\..\..\..\src\apps\snmp\snmp_traps.c(686): error C2059: syntax error : '}' [W:\lwip\lwip\build\contrib\ports\win32\example_app\lwipallapps.vcxproj]
  ..\..\..\..\..\src\apps\snmp\snmp_traps.c(686): error C2059: syntax error : ')' [W:\lwip\lwip\build\contrib\ports\win32\example_app\lwipallapps.vcxproj]
  ..\..\..\..\..\src\apps\snmp\snmp_traps.c(686): error C2059: syntax error : ')' [W:\lwip\lwip\build\contrib\ports\win32\example_app\lwipallapps.vcxproj]
  ..\..\..\..\..\src\apps\snmp\snmp_traps.c(688): error C2059: syntax error : 'while' [W:\lwip\lwip\build\contrib\ports\win32\example_app\lwipallapps.vcxproj]
  ..\..\..\..\..\src\apps\snmp\snmp_traps.c(694): error C2059: syntax error : 'return' [W:\lwip\lwip\build\contrib\ports\win32\example_app\lwipallapps.vcxproj]
  ..\..\..\..\..\src\apps\snmp\snmp_traps.c(695): error C2059: syntax error : '}' [W:\lwip\lwip\build\contrib\ports\win32\example_app\lwipallapps.vcxproj]

Dirk Ziegelmeier <dziegel>
Group administrator
Tue 23 Oct 2018 01:11:19 PM UTC, original submission:  

While we were using SNMP app, one of the requirements was to support sending traps in v1 and v2c. Since original app wasn’t providing these functionalities, we have implemented them as a patch. Now SNMP app has support for sending traps, notifications and informs. For informs we have added callback, so main app can have feedback.

Ognjen Bjelica <ognjenbjelica>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #45309:  snmp_informs.patch added by dziegel (33KiB - text/x-patch)
file #45279:  0003-Fixes-for-SNMP-app-by-Softhouse-ComAp - New-features.patch added by ognjenbjelica (35KiB - application/octet-stream - Please ignore previous patch.)
file #45273:  0002-Fixes-for-SNMP-app-by-Softhouse-ComAp - New-features.patch added by ognjenbjelica (35KiB - application/octet-stream - Please ignore previous patch.)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by dziegel (Posted a comment)
  • -email is unavailable- added by ognjenbjelica (Submitted the item)
  • -email is unavailable- added by ognjenbjelica
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-11-07 dziegel Open/ClosedOpen Closed
    2018-11-07 dziegel StatusNone Done
    2018-10-26 dziegel Attached File- Added snmp_informs.patch, #45309
    2018-10-24 ognjenbjelica Attached File- Added 0005-Fixes-for-SNMP-app-by-Softhouse-ComAp - New-features.patch, #45281
    2018-10-24 ognjenbjelica Attached File- Added 0004-Fixes-for-SNMP-app-by-Softhouse-ComAp - New-features.patch, #45280
    2018-10-24 ognjenbjelica Attached File- Added 0003-Fixes-for-SNMP-app-by-Softhouse-ComAp - New-features.patch, #45279
    2018-10-24 ognjenbjelica Attached File- Added 0002-Fixes-for-SNMP-app-by-Softhouse-ComAp - New-features.patch, #45273
    2018-10-23 ognjenbjelica Attached File- Added 0001-Fixes-for-SNMP-app-by-Softhouse-ComAp - New-features.patch, #45269
        Carbon-Copy- Added -email is unavailable-

    Back to the top

    Powered by Savane 3.13-caa5.
    Corresponding source code