Re: Initializing a device

[Akos Vandra <axos88 () gmail com>]

Hi!

I have read through the threads you mentioned, it's nice to see that
something similar has already been suggested, I would like to present
a few new arguments in favor of them.

On 6 January 2012 12:15, Guy Harris <guy () alum mit edu> wrote:
> On Jan 5, 2012, at 1:40 AM, Akos Vandra wrote:
>
> > I browsed through the code of pcap_open_live, and pcap_set_promisc,
> > and related stuff, and I think that now I understand how this works.
> >
> > However in my opinion, the way parameter passing is implemented breaks
> > the principle of modularity.
> >
> > iface here = pcap_canusb, pcap_usbmon, etc.
> > What is the correct term for these?
>
> There isn't one.  I've used "device-type module" later in this message.  "iface" is close to "interface", and that 
> could be confused with "interface" in the sense of a network interface, on which you might do capture.

Okay, we should settle on some name like device-type module, or
capture module before talking about this gets too confusing - you
chose. :)

> > I keep using iface and module interleavedly in my mails...
> >
> > If - let's say - my interface (canusb) needs a parameter for the baud
> > rate, and another interface, let's say a more low-level radio iface
> > needs parameters like frequency, channel, modulation (ASK or FSK),
> > then to fully support these capture ifaces, we would need to add a lot
> > of parameters to pcap_open_live,
>
> No, we wouldn't need to (and we wouldn't ever do so, as that'd break source and binary compatibility).

...............

> Programs that need to use *any* capabilities not provided by pcap_open_live(), such as setting the kernel capture 
> buffer size, setting monitor mode on 802.11 interfaces, or setting the time stamp type, must use the newer APIs - 
> pcap_create(), the appropriate setter routines, and pcap_activate().
>
> > and a lot of API functions (like
> > pcap_set_modulation, pcap_set_channel) which are used by a single
> > iface.
> >
> > That is - in my opinion - bad.
>
> The current API was chosen during a discussion on the tcpdump-workers mailing list back in 2008.
>
> "I.e. we should instead have a new pcap_open() call, and then we should
> have a serious of calls that set various options or ask for various
> things based upon that handle."
>
> > My suggestion for a solutoin - which is a *lot* of work -, but will
> > add a nice functionality to libpcap:
> >
> > Please view this from a higher level, if you guys like it we might
> > think about data structures, etc later, I have ideas about that too,
> > but they are quite immature yet.
> >
> > Add a new _op command list_parameters, which returns a list of the
> > parameters that it needs with data about: parameter name, type,
> > default value. If the _op is unset, or returns null, it means the
> > iface doesn't need any (backw compatibility).
> >
> > Add a few new API calls to create a list of parameters, and set values
> > in it. These functions check if the correct type of argument is given.
> > params_create - returns a handle
> > params_set_uint8 - sets an uint value, etc.
> >
> > Then pass this parameters structure to the activate_op, and it can do
> > the activation based on these parameters.
> >
> > Now if we add a new parameter, there is no need for new API calls,
> > just add a new entry to the param_list.
>
> Something along those lines was proposed in that thread:
>
>        http://thread.gmane.org/gmane.network.tcpdump.devel/2800/focus=2818
>
> and that proposal evolved:
>
>        http://thread.gmane.org/gmane.network.tcpdump.devel/2800/focus=2819
>
>        http://thread.gmane.org/gmane.network.tcpdump.devel/2800/focus=2820
>
>        http://thread.gmane.org/gmane.network.tcpdump.devel/2800/focus=2822
>
>        http://thread.gmane.org/gmane.network.tcpdump.devel/2800/focus=2823
>
>        http://thread.gmane.org/gmane.network.tcpdump.devel/2800/focus=2824
>
>        http://thread.gmane.org/gmane.network.tcpdump.devel/2800/focus=2826
>
> and then Michael Richardson said
>
>        http://thread.gmane.org/gmane.network.tcpdump.devel/2800/focus=2828
>
> which, shortly afterwards, got us moving in the direction of his suggestion.
>
> An API pattern based on a list of parameters is more complicated than the current API pattern, as you have to create 
> and manipulate the list.
>
> One disadvantage of the current API pattern is that it can't handle new parameters added by new device-type modules 
> without changing the core API.

Yeah, this is what I was trying to point out, and what I find to be
quite a barrier for scalability of libpcap. I will be comparing
libpcap to wireshark in the next few lines, as they are quite close to
eachother, and I really love the architecture of wireshark.

The best thing would be if it was possible to add new capture modules
(aka device-type modules) just by writing a pcap-*.c file, and adding
a few lines to the makefile and autoconf.

As you said the only way to add new parameters with the current API is
to extend it with a new setter function, which may or may not be
called by the user program to set the parameters new value as needed.
Right now the API works by setting the right member field of the opts
structure for each parameter, so in order to add a new parameter, the
opts structure must be extended as well.

Another approach would be for the API to call function pointers (if
not NULL) from the pcap structure, which would avoid an opts
structure, and linking in code from the capture modules themselves,
but then we would need to extend the pcap structure itself with these
function pointers, because we cannot link in the setter code from the
capture modules (which might not be supported on some platforms or
configurations).

The main problem here is that in any case in order to add a new
parameter, either the pcap or the opts structure needs to be extended,
and probably only one capture module will be using that member
function pointer, or opts member field.

IMHO With many capture modules this might get out of hand, and anyways
the need to even to extend, not modify the API in order to support a
new module is not a good shot.

Another reason why this is not a good approach: Let's get wireshark in
the picture. Let's say the user selected a canusb device. The only way
for wireshark to know what parameters (ex. baudrate) the canusb device
needs is if wireshark knows how the canusb device works. This is bad.
IMHO one of the main goals of libpcap would be to hide how the capture
device actually works from the user application, so that it can use a
device-independent way of getting packets.
Even if wireshark would magically know that it needs to request a
baudrate from the user, and does, it would need to know that there is
an API call names pcap_set_baud(pcap_t*, int) used for that. And this
is where the most important problem gets in the picture. Let's say
that now libpcap is updated on the system, and a new capture module,
"usbradio" is added, which needs "frequency" and "channel" parameters.
Even if wireshark magically knows (maybe by querying libpcap for that)
that it needs to request these parameters from the user, there is
ABSOLUTELY NO WAY for it to call pcap_set_frequency() and
pcap_set_radiochannel() to set those parameters, as wireshark was not
linked to call those methods! So in order to add support for a new
type of device, wireshark needs to be patched and recompiled, which
breaks the principle of modularity. (In order to extend libpcap you
need to extend wireshark).

In my solution, no new API calls are added, and wireshark would
auto-magically support any new devices. It would work this way:

  - wireshark queries libpcap for available capture devices / interfaces
  - user select one device, and asks wireshark what options can be set
  - wireshark calls pcap_create(), which sets op_activate, and
op_getoptlist, op_setparam, op_getparam
  - wireshark calls pcap_get_optlist(), which is forwarded to the
capture modules op_getoptlist. This returns a list of tuples, with
option's <name, id, TYPE, default value>
  - wireshark presents an dialog generated on-the-fly using the getoptlist.
  - user sets some parameters, chooses ok
  - wireshark repeadetly calls pcap_setparam_int(id, value) or
pcap_setparam_bool(id, value), or whatever, which checks for parameter
type safety, and calls the capture module's pcap_setparam(id, &value).
(Dereferencing will be safe as the API already checked for type
safety. This idea is from the wireshark API, a few basic types will be
supported, like UINT, INT, BOOL, ENUM, etc.
  - wireshark calls pcap_activate().


Advantages comparing to my former suggestion are that there is no
complication for maintiaining a parameter list.
The API is static, no need to extend it in order to add new parameters
to a capture module.
Wireshark (or some other user application) can use new interfaces
without any logic added to them, if new parameters are present, it can
either ask them from the user, or use the default values.
If a capture module does not need any parameters, the op_getoptlist
may simply be NULL.
the pcap strucure's opts member could be removed, but better be
deprecated for backw compatibility (user application shouldn't have
poked around it anyways). Those few modules like the 802.11 which used
it can be rewritten to set the parameters with the new API, user
applications would never see the difference.
Advantages comparing to the discussion on the list would be that there
is no central repository for the parameters, so each capture module
can maintain it's own list. (I would add a version number here as
well, it might be needed where a user application uses a specific
capture module, and does know how it works, and wants to set its
parameters automatically) - This allows all capture interfaces to be
contained in a single file (except registration! - see later).

Parameter values can be set even by name or id. The first one is more
useful for human-inputs and the latter for automatic control of
parameters.

Even though it does not change the API, I would also change the way
the best capture module is chosen by pcap_open.
Right now there is a bunch of calls to strstr checking for a magic
string. In order to add support for a new module, this function must
be extended by adding another strstr call, and calling the appropriate
pcap_xxx_create function if the test succeeds.
I had problem with this when adding the "canusb" capture device. There
was already a "can" module, and I added my strstr after the "can"
check, so libpcap never got there (as the test against "can"
succeded).

If you ask me, I would either trust the pcap_xxx_create function with
this test, and if it suspects the device is not for that module,
return NULL. Then just call the pcap_xxx_create functions until one of
them returns a non-NULL pointer, which means its test succeded, and it
succeded creating the pcap device, or until it runs out of capture
module pcap_xxx_create()-s to call.
I would go further, by adding a pcap_register_capture_module(pcap_t
(*constructor)(char*device, char*err)) call, (constructor is what
pcap_xxx_create is now) which would be called once on initialization
of the pcap library, it would load all the possible capture modules
into an array or list, and then it would iterate over the list and
call the pcap_xxx_cretate funcitons one-by-one, until the list ends,
or one of them returns a valid pcap_t structure.
I would go even further (i suspect this will be denied :) by putting
all the capture modules in their own mini-library, which would be
loaded by the libpcap initialization function. It would get the
contents of a magic directory containing all the capture module
libraries, dynamically loading each one of them, and calling the
pcap_capture_module_init function, which in turns calls back the
pcap_register_capture_module with the appropriate constructor
functions.

This way all capture modules would truly be contained only in a single
source file, and a new capture module might be added without even
recompiling libpcap itself, but just copying the appropriate
mini-capture-module-library in the magic directory. Wireshark works in
a very similar way, except that instead of using dynamic linking of
these libraries it uses a script to generate the function which calls
all register functions in the submodules, and links them at linking
time. (that's a repetition I don't know how to avoid)


Disclaimer:

I am usually the guy with a lot ideas (some brilliant and some really
stupid), but usually can't figure out which one is from which category
:). This is what you guys are here for, you probably have a *lot* more
experience than me, especially with opensource projects, writing
portable software, and software design.
These are my two cents in on what I think would be awesome, so please
don't get offended that I kinda want to change everything, God forbid
me to intend to say that what is now is not good.

Also my description of the way it works still needs a few rounds of
refining, but I guess it's enough for you to understand what am I
thinking of.

Regards,
  Ákos Vandra
-
This is the tcpdump-workers list.
Visit https://cod.sandelman.ca/ to unsubscribe.