freecwmp
[Top] [All Lists]

Re: patch for the freecwmp project

To: KALLEL Mohamed <mohamed.kallel@pivasoftware.com>
Subject: Re: patch for the freecwmp project
From: Luka Perkov <freecwmp@lukaperkov.net>
Date: Wed, 29 Aug 2012 22:05:06 +0200
Cc: freecwmp@linux-mips.org
In-reply-to: <503DCE1A.6070605@pivasoftware.com>
Mail-followup-to: KALLEL Mohamed <mohamed.kallel@pivasoftware.com>, freecwmp@linux-mips.org
Original-recipient: rfc822;freecwmp@linux-mips.org
References: <50334294.9020508@pivasoftware.com> <50334314.7020807@phrozen.org> <CAOiHx==ggn+ii_mr8Bc-7hgLRmwsARyuYhfNQQ-X3ERfVip6Uw@mail.gmail.com> <503DCE1A.6070605@pivasoftware.com>
Sender: freecwmp-bounce@linux-mips.org
User-agent: Mutt/1.5.21 (2010-09-15)
Hi Mohamed,

On Wed, Aug 29, 2012 at 09:08:58AM +0100, KALLEL Mohamed wrote:
> >>--- a/ext/openwrt/config/freecwmp
> >>+++ b/ext/openwrt/config/freecwmp
> >>@@ -2,7 +2,8 @@ config local
> >>    option interface eth0
> >>    option port 7547
> >>    option ubus_socket /var/run/ubus.sock
> >>-   option event bootstrap
> >>+   list event bootstrap
> >>+   list event boot
> >bootstrap should only be sent on the very first contact to the ACS -
> >you should make sure that the event gets removed after successful
> >delivery, so subsequent boots don't send another bootstrap.
> we could add in the launcher script (freecwmp) a script which add
> the bootsrap event in case the acs address is changed and a boot
> event in case of the boot of the equipment. The both event could be
> added in the config (in the event list) before starting the freecwmp
> daemon. after the send of the event, the freecwmpd should remove the
> events from the config (c code) . this behaviour could be added in
> the future

I dont think this is a good idea. I dont want freecwmp starter script to
touch the freecwmp config. This needs to be handled in freecwmp core.

> >>diff --git a/src/cwmp/cwmp.h b/src/cwmp/cwmp.h
> >>index 947539c..a1b3d8b 100644
> >>--- a/src/cwmp/cwmp.h
> >>+++ b/src/cwmp/cwmp.h
> >>@@ -27,6 +27,12 @@ enum cwmp_event_code
> >>    AUTONOMOUS_TRANSFER_COMPLETE
> >>  };
> >>
> >>+enum cwmp_get_order
> >>+{
> >>+   FIRST = 0,
> >>+   NEXT
> >>+};
> >>+
> >>  struct notification {
> >>    struct list_head list;
> >>
> >>@@ -55,7 +61,7 @@ int8_t cwmp_download_handler(char *url, char *size);
> >>  int8_t cwmp_reboot_handler(void);
> >>  int8_t cwmp_factory_reset_handler(void);
> >>  int8_t cwmp_reload_changes(void);
> >>-char * cwmp_get_event_code(void);
> >>+char * cwmp_get_event_code(enum cwmp_get_order);
> >Generally I don't really like this interface. Events might be
> >read/modified/added concurrently, and using this interface will make
> >it hard to make it thread-safe. I'd rather see the list directly
> >accessed, and then add a mutex for protecting accesses to it.

I agree with Jonas here.

> >>+   return 1;
> >You should return FC_SUCCESS here (which is 0). Also you are missing
> >FC_DEVEL_DEBUG("enter/exit") calls.

I will remove all FC_DEVEL_DEBUG("enter/exit") calls soon so it's ok if
they are missing.

Mohamed please resend your patch with the fixes Jonas and me pointed
out. After that we'll review it once again and see if it's ok for
merging.

Regards,
Luka

<Prev in Thread] Current Thread [Next in Thread>