Vitezslav Samel <vitezslav(a)samel.cz> writes:
On Tue, Sep 04, 2012 at 03:21:36PM +0200, Nikola Pajkovsky wrote:
> Vitezslav Samel <vitezslav(a)samel.cz> writes:
>
> > On Tue, Sep 04, 2012 at 02:44:11PM +0200, Nikola Pajkovsky wrote:
> >> Vitezslav Samel <vitezslav(a)samel.cz> writes:
> >>
> >> > Please, change commit message to match reality of the patch.
> >>
> >> ech...
> >>
> >> >> diff --git a/src/promisc.c b/src/promisc.c
> >> >> index a0f5364..cb3e1dd 100644
> >> >> --- a/src/promisc.c
> >> >> +++ b/src/promisc.c
> >> >> @@ -17,178 +17,98 @@ promisc.c - handles the promiscuous mode flag
for the Ethernet/FDDI/
> >> >>
> >> >> #define PROMISC_MSG_MAX 80
> >> >>
> >> >> -void init_promisc_list(struct promisc_states **list)
> >> >> +static void add_dev(struct list_head *promisc, const char
*dev_name, int flags)
> >> >
> >> > maybe change the name to promisc_add_dev()
> >>
> >> it's small static helper which adds device to promisc list. It could be
> >> named with promisc_ prefix. I don't care. I'm pretty happy even
without
> >> promisc_ prefix
> >>
> >> >> -void save_promisc_list(struct promisc_states *list)
> >> >> +static int dev_promisc_flag(const char *dev_name)
> >> >
> >> > This function should go into ifaces.c
> >>
> >> why not
> >>
> >> >> {
> >> >> - int fd;
> >> >> - struct promisc_states *ptmp = list;
> >> >> -
> >> >> - fd = open(PROMISCLISTFILE, O_WRONLY | O_CREAT, S_IRUSR |
S_IWUSR);
> >> >> -
> >> >> - if (fd < 0) {
> >> >> - write_error("Unable to save interface flags");
> >> >> - return;
> >> >> + int flags = dev_get_flags(dev_name);
> >> >> + if (flags < 0) {
> >> >> + write_error("Unable to obtain interface parameters for
%s",
> >> >> + dev_name);
> >> >> + return -1;
> >> >> }
> >> >>
> >> >> - while (ptmp != NULL) {
> >> >> - write(fd, &(ptmp->params), sizeof(struct
promisc_params));
> >> >> - ptmp = ptmp->next_entry;
> >> >> - }
> >> >> + if (flags & IFF_PROMISC)
> >> >> + return -1;
> >> >
> >> > -1 should be reserved only for error states; this function should
> >> > return -1/0/1 on error (getting flags)/promisc not set/promisc set
> >>
> >> that func is about: 'if you can't give me flags about device, you
are
> >> not worth it. If you already have set promisc flags, you are not worth
> >> it'.
> >>
> >> I don't see why do we have to distinguish err and not set.
> >
> > Then change the name of this function.
> >
> >> > I would add this comment:
> >> >
> >> > /* restore: since we have only devices which need to clear promisc
flag
> >> > * on the list, call dev_clear_promisc() on it
> >> > */
> >>
> >> if we need comment here, that means something is wrong even with that
> >> simple function.
> >
> > Because of naming of the function: restore is not the same as
> > clear (promisc flag).
> >
> >> int dev_set_promisc(const char *ifname)
> >> {
> >> return dev_set_flags(ifname, IFF_PROMISC);
> >> }
> >>
> >> int dev_clear_promisc(char *ifname)
> >> {
> >> return dev_clear_flags(ifname, IFF_PROMISC);
> >> }
> >>
> >> better question is why this is not a macro?
> >>
> >> #define dev_set_promisc(dev) dev_set_flags((dev), IFF_PROMISC)
> >> #define dev_clear_promisc(dev) dev_clear_flags((dev), IFF_PROMISC)
> >>
> >> it would make things more simple.
> >
> > I don't see where it is more simple.
> >
> >> >> diff --git a/src/promisc.h b/src/promisc.h
> >> >> index f062ca1..1996982 100644
> >> >> --- a/src/promisc.h
> >> >> +++ b/src/promisc.h
> >> >> @@ -1,31 +1,19 @@
> >> >> #ifndef IPTRAF_NG_PROMISC_H
> >> >> #define IPTRAF_NG_PROMISC_H
> >> >>
> >> >> -/*
> >> >> - * promisc.h - definitions for promiscuous state save/recovery
> >> >> - *
> >> >> - * Thanks to Holger Friese
> >> >> - * <evildead(a)bs-pc5.et-inf.fho-emden.de> for the base
patch.
> >> >> - * Applied it, but then additional issues came up and I ended up
doing more
> >> >> - * than slight modifications. struct iflist is becoming way too
large for
> >> >> - * comfort and for something as little as this.
> >> >> - */
> >> >> +#include "list.h"
> >> >>
> >> >> -struct promisc_params {
> >> >> +struct promisc_list {
> >> >> + struct list_head list;
> >> >> char ifname[IFNAMSIZ];
> >> >> - int saved_state;
> >> >> - int state_valid;
> >> >> + int flags;
> >> >
> >> > I would rather see only "int promisc:1" with boolean
meaning of
> >> > promiscuous mode set or not set.
> >>
> >> if we guarantee, that promics_list will contains only devices with flags
> >> up, that we don't even need this. I'm not so sure it this is good
idea.
> >
> > I don't know what you mean by this: devices with flags up: are you
> > testing if device is up? Functions in promisc.c should only know about
> > promiscuity of interface and nothing else. And in promisc_restore_list()
> > you could test only on entry->was_promiscuous and leave IFF_PROMISC
> > testing to ifaces.c.
>
> the sentence should be:
> if we guarantee, that promics_list will contains only devices with
> promisc flag up, that we don't even need flags/promisc member.
That's the best solution: only list with interface name.
ok, it will be there
--
Nikola