8:06 a.m.
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.
Vita