Patches item #2772787, was opened at 2009-04-18 09:32
Message generated for change (Comment added) made by marineam
You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=312694&aid=2772787&group_id=12694
Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: None
Group: None
Status: Open
Resolution: Accepted
Priority: 5
Private: No
Submitted By: Bart Van Assche (bvassche)
Assigned to: Dave Shield (dts12)
Summary: Reverts patch 1627049
Initial Comment:
Patch 1627049 was introduced for the wrong reason -- see also https://sourceforge.net/tracker/?func=detail&atid=312694&aid=1627049&group_id=12694. This patch reverts it. The code with this patch applied is as close to Subversion revision 15983 as possible -- the revision before patch 1627049 was applied.
----------------------------------------------------------------------
Comment By: Michael Marineau (marineam)
Date: 2009-06-24 15:34
Message:
Re the 2009-06-23 23:30 reply:
Adding the third argument to netsnmp_copy_fd_set_to_large_fd_set() and
netsnmp_copy_large_fd_set_to_fd_set() works but as I mentioned this only
helps snmp_select_info()/snmp_sess_select_info() but not
snmp_read()/snmp_sess_read() which also takes a fd_set, converts it to a
large_fd_set, and passes that along to snmp_sess_read2(). A solution to
this that just came to mind would be to actually convert read back to use
the old-style fd_set and have snmp_sess_read2() be a wrapper around
snmp_sess_read() which passes the fd_set embedded in large_fd_set. Since
snmp_sess_read does not modify the fd_set we are safe from memory
corruption and still provide the new robust large_fd_set api.
----------------------------------------------------------------------
Comment By: Bart Van Assche (bvassche)
Date: 2009-06-23 23:30
Message:
Regarding the comment by marineam on 2009-06-24 07:07: an alternative
approach that implements the same functionality, that is safer and that
takes less work to implement is as follows:
* Add a third argument to netsnmp_copy_fd_set_to_large_fd_set() and
netsnmp_copy_large_fd_set_to_fd_set() and that represents the size of the
fd_set.
* Pass MAX(FD_SETSIZE, numfds) as the size argument to the aforementioned
two functions.
This approach is safer because it does not trigger memory corruption in
case the numeric value of the file descriptors used by Net-SNMP is larger
than the 'numfds' argument passed by the caller of
snmp_select_info()/snmp_sess_select_info().
Note: for both the original proposal by user marineam and my proposal it's
important that the documentation of
snmp_select_info()/snmp_sess_select_info() is updated such that it is
clearly specified that when working with large file descriptors the
'numfds' argument must be one more than the largest file descriptor in use
internally by Net-SNMP instead of one more than the largest file descriptor
in the file descriptor set passed by the caller. (The
snmp_select_info2()/snmp_sess_select_info2() functions do not have such
strange semantics.)
----------------------------------------------------------------------
Comment By: Michael Marineau (marineam)
Date: 2009-06-23 22:07
Message:
Although I welcome the ability to handle this better (it took me a while to
figure out a memory corruption problem due to trying to fit large fds in a
nomral fd_set) this change breaks how large fds must be handled with older
versions of net-snmp. As outlined in the original patch ticket (1627049)
the solution for using select with large fds is to simply create an extra
large fd_set variable instead of using the normal fd_set size. This patch
breaks this trick and spews lots of "Use snmp_sess_select_info2() for
processing large file descriptors" errors as soon as you pass FD_SETSIZE.
I would suggest a compromise would be if a value greater than FD_SETSIZE
is passed to snmp_select_info/snmp_sess_select_info for numfds then assume
that it is the max rather than FD_SETSIZE and pass it along to the copy
functions. Unfortunately this doesn't cover snmp_read/snmp_sess_read since
those functions do not have the numfds parameter. So at the moment I'm not
sure how backwards compatibility can be preserved and still make use of the
new large_fd_set as it currently stands.
Perhaps the simplest solution would be to pull large_fd_set back out and
simply modify snmp_select_info/snmp_sess_select_info to actually respect
the value of numfds passed by the user as and raise an error when
FD_SETSIZE or numfds is exceed (which ever is greater). This is the
behavior I originally expected at first glance at the API rather than
entirely ignoring numfds other than to increment it as it does now.
As a separate issue, tucking the large_fd_set struct away in the internal
library headers means users don't really have a good way of pulling the fds
out when using snmp_select_info2/snmp_sess_select_info2 for use with an
external event loop like they can with
snmp_select_info/snmp_sess_select_info.
----------------------------------------------------------------------
Comment By: Wes Hardaker (hardaker)
Date: 2009-06-11 14:43
Message:
I agree that right now things are confusing. I've commited r17650 at the
moment in an effort to at least include the large_fd_set.h file from
types.h. I'll wait for Dave to chime in on anything else since he's been
the one tracking this issue more than I.
----------------------------------------------------------------------
Comment By: Bart Van Assche (bvassche)
Date: 2009-04-23 11:20
Message:
To Dave: now that the netsnmp_large_fd_set typedef has been moved back to
the large_fd_set.h header, shouldn't "netsnmp_large_fd_set" be replaced by
"struct netsnmp_large_fd_set" in the session_api.h header file ?
----------------------------------------------------------------------
Comment By: Bart Van Assche (bvassche)
Date: 2009-04-23 09:23
Message:
OK !
----------------------------------------------------------------------
Comment By: Dave Shield (dts12)
Date: 2009-04-23 09:00
Message:
Don't worry - the structure is still defined in a header file,
and will be available to the compiler (and to developers).
The distinction between <net-snmp/*.h> and <net-snmp/library/*.h>
is effectively Core API vs library internals.
Structures that are defined in the top-level header files is the stuff
that developers are expected to use directly. Stuff defined in the
library/*.h header files is not (and hence may potentially be changed).
Now it's going to take us a while to clarify this distinction properly,
but we might as well make a start now.
OK?
Dave
----------------------------------------------------------------------
Comment By: Bart Van Assche (bvassche)
Date: 2009-04-23 01:37
Message:
I agree that the internals of the large_fd_set structure are not meant for
direct use. Please note that because of performance reasons it is
recommended to allocate the large_fd_set structure on the stack instead of
allocating it dynamically. So the compiler must know the size of the
structure at compile time. That is why I defined the structure in a header
file and why I did not hide it completely by e.g. defining it in
large_fd_set.c.
----------------------------------------------------------------------
Comment By: Dave Shield (dts12)
Date: 2009-04-23 01:30
Message:
Refinements patch applied to the main development trunk
SVN revision 17536
On due consideration, I don't believe that the internals of
the large_fd_set structure are really meant for direct use,
and this is probably best regarded as an "opaque" data type.
I've therefore moved the definition of this structure back
into the <net-snmp/library/large_fd_set.h> header file.
SVN revision 17537
Hopefully this feature is now stable
(hence closing the tracker entry)
----------------------------------------------------------------------
Comment By: Bart Van Assche (bvassche)
Date: 2009-04-22 23:12
Message:
Added patch netsnmp-large-fd-set-refinements.patch that implements the
following changes:
- Added more comments.
- Added functions netsnmp_copy_fd_set_to_large_fd_set() and
netsnmp_copy_large_fd_set_to_fd_set(). The latter now performs error
checking.
- An error message is now printed by those functions that return an fd_set
when dealing with large file descriptors.
- netsnmp_external_event_info() and netsnmp_dispatch_external_events() now
print an error message instead of silently triggering memory corruption
when working with large file descriptors.
- Fixed some coding style issues (see also the "} else {" changes).
- Inside large_fd_set.c, made sure that set resizing does not trigger
uninitialized memory reads.
This patch has been tested as follows on Windows and on Linux:
- Added test code in snmpd.c that created 2 * FD_SETSIZE + 1 sockets via a
loop (just after SOCK_STARTUP) to make sure that all file descriptors used
for SNMP communication were large file descriptors.
- Ran snmpwalk on the agent for a few minutes to make sure that the large
fd set implementation did not cause any memory leaks.
Please review.
----------------------------------------------------------------------
Comment By: Dave Shield (dts12)
Date: 2009-04-21 07:39
Message:
I've actually just committed a revised version of your original patch,
with some minor renaming (to keep magfr happy:)), and taking into
account my recent work on the header files.
SVN revision 17515
----------------------------------------------------------------------
Comment By: Bart Van Assche (bvassche)
Date: 2009-04-21 07:33
Message:
Added version two of the implementation of large file descriptors. For this
second version of the patch I have verified that the trunk + the v2 patch
still compiles fine on Windows and on Linux, and that an snmpwalk -v1 on
the running agent is still possible.
----------------------------------------------------------------------
Comment By: Magnus Fromreide (magfr)
Date: 2009-04-20 14:52
Message:
I dislike the naming. Could you please add a netsnmp_ prefix to all new
symbols?
(Yes, I know we have failed on that earler. Many times. But it would be
nice to start doing the right thing. Please.)
----------------------------------------------------------------------
Comment By: Bart Van Assche (bvassche)
Date: 2009-04-20 12:51
Message:
Added netsnmp-large-fd-set.patch: a proof-of-concept patch that adds
support for large file descriptors on Unix systems and for more than 64
sockets on Windows systems. Tests performed: compiled patched code on
Win32, compiled patched code on Linux, performed repeated snmpwalk on the
patched code on Linux.
----------------------------------------------------------------------
Comment By: Bart Van Assche (bvassche)
Date: 2009-04-19 22:55
Message:
Removed netsnmp-document-how-to-use-large-file-descriptors.patch because
I'm working on a better solution.
----------------------------------------------------------------------
Comment By: Bart Van Assche (bvassche)
Date: 2009-04-19 10:04
Message:
Added patch that explains how to deal with so-called large file
descriptors.
----------------------------------------------------------------------
Comment By: Magnus Fromreide (magfr)
Date: 2009-04-18 10:13
Message:
Added file to remove the configure parts of the poll patch as well.
----------------------------------------------------------------------
You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=312694&aid=2772787&group_id=12694
|
