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