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.
Vita