Re: [LEDE-DEV] [PATCH] ubusd: Use linked list for queued messages

[Alexandru Ardelean]

On Wed, May 2, 2018 at 11:55 PM, Benjamin Hansmann  wrote:
> The fixed size array for queuing messages led to discarding messages
> when it was full, using a linked list instead solves this issue.
>
> Having the list_head link in the ubus_msg_buf itself avoids the
> allocation of more memory for an independent list.
>
> The motivation was that for a recursive "ubus list" the function
> ubusd_proto.c:ubusd_handle_lookup() produces more than n messages in
> one uloop cycle when n objects are registered on the bus.
>

Hey,

I second this patch.
I also proposed it a while back
http://patchwork.ozlabs.org/patch/772366/

This was part of a series of ubus fixes.
I added a test that shows the issue [I was seeing]:
http://patchwork.ozlabs.org/patch/772365/

The issue [I was seeing] was]more of a fast-producer - slow-consumer issue.
This was caused mostly when running a ubus cmd on the TTY serial [
causing a slowdown which showed the issue ].
But in my case, another patch had a greater impact that the dynamic TX queue ;
this one: http://patchwork.ozlabs.org/patch/772364/
I was never hitting the 32 entries limit.

I also discarded this patch [at the time] because I was unsure whether
it causes more issues [than it solves].
And did not have time to go more in-depth with it.

But I think, that if this helps your case, it should be good.

Also [with this occasion]: thanks Felix for merging my other ubus patches.

Thanks
Alex

> Signed-off-by: Benjamin Hansmann 
> ---
>  ubusd.c   | 19 ---
>  ubusd.h   |  6 +++---
>  ubusd_proto.c |  1 +
>  3 files changed, 12 insertions(+), 14 deletions(-)
>
> diff --git a/ubusd.c b/ubusd.c
> index ba1ff07..b7e1f79 100644
> --- a/ubusd.c
> +++ b/ubusd.c
> @@ -138,11 +138,8 @@ static int ubus_msg_writev(int fd, struct ubus_msg_buf 
> *ub, int offset)
>
>  static void ubus_msg_enqueue(struct ubus_client *cl, struct ubus_msg_buf *ub)
>  {
> -   if (cl->tx_queue[cl->txq_tail])
> -   return;
> -
> -   cl->tx_queue[cl->txq_tail] = ubus_msg_ref(ub);
> -   cl->txq_tail = (cl->txq_tail + 1) % ARRAY_SIZE(cl->tx_queue);
> +   struct ubus_msg_buf *qub = ubus_msg_ref(ub);
> +   list_add_tail(>queue, >tx_queue);
>  }
>
>  /* takes the msgbuf reference */
> @@ -153,7 +150,7 @@ void ubus_msg_send(struct ubus_client *cl, struct 
> ubus_msg_buf *ub)
> if (ub->hdr.type != UBUS_MSG_MONITOR)
> ubusd_monitor_message(cl, ub, true);
>
> -   if (!cl->tx_queue[cl->txq_cur]) {
> +   if (list_empty(>tx_queue)) {
> written = ubus_msg_writev(cl->sock.fd, ub, 0);
>
> if (written < 0)
> @@ -172,20 +169,20 @@ void ubus_msg_send(struct ubus_client *cl, struct 
> ubus_msg_buf *ub)
>
>  static struct ubus_msg_buf *ubus_msg_head(struct ubus_client *cl)
>  {
> -   return cl->tx_queue[cl->txq_cur];
> +   if (list_empty(>tx_queue))
> +   return NULL;
> +   return list_first_entry(>tx_queue, struct ubus_msg_buf, queue);
>  }
>
>  static void ubus_msg_dequeue(struct ubus_client *cl)
>  {
> struct ubus_msg_buf *ub = ubus_msg_head(cl);
> -
> if (!ub)
> return;
>
> -   ubus_msg_free(ub);
> +   list_del_init(>queue);
> cl->txq_ofs = 0;
> -   cl->tx_queue[cl->txq_cur] = NULL;
> -   cl->txq_cur = (cl->txq_cur + 1) % ARRAY_SIZE(cl->tx_queue);
> +   ubus_msg_free(ub);
>  }
>
>  static void handle_client_disconnect(struct ubus_client *cl)
> diff --git a/ubusd.h b/ubusd.h
> index 4d87920..375f31f 100644
> --- a/ubusd.h
> +++ b/ubusd.h
> @@ -23,13 +23,13 @@
>  #include "ubusmsg.h"
>  #include "ubusd_acl.h"
>
> -#define UBUSD_CLIENT_BACKLOG   32
>  #define UBUS_OBJ_HASH_BITS 4
>
>  extern struct blob_buf b;
>
>  struct ubus_msg_buf {
> uint32_t refcount; /* ~0: uses external data buffer */
> +   struct list_head queue;
> struct ubus_msghdr hdr;
> struct blob_attr *data;
> int fd;
> @@ -48,8 +48,8 @@ struct ubus_client {
>
> struct list_head objects;
>
> -   struct ubus_msg_buf *tx_queue[UBUSD_CLIENT_BACKLOG];
> -   unsigned int txq_cur, txq_tail, txq_ofs;
> +   struct list_head tx_queue;
> +   unsigned int txq_ofs;
>
> struct ubus_msg_buf *pending_msg;
> struct ubus_msg_buf *retmsg;
> diff --git a/ubusd_proto.c b/ubusd_proto.c
> index 2d04b5a..ac9d075 100644
> --- a/ubusd_proto.c
> +++ b/ubusd_proto.c
> @@ -495,6 +495,7 @@ struct ubus_client *ubusd_proto_new_client(int fd, 
> uloop_fd_handler cb)
> goto free;
>
> INIT_LIST_HEAD(>objects);
> +   INIT_LIST_HEAD(>tx_queue);
> cl->sock.fd = fd;
> cl->sock.cb = cb;
> cl->pending_msg_fd = -1;
> --
> 2.11.0
>
>
> ___
> Lede-dev mailing list
> Lede-dev@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/lede-dev

___
Lede-dev mailing list

Re: [LEDE-DEV] [PATCH] wolfssl: fix options and add support for wpa_supplicant features

[Alexandru Ardelean]

On Sat, Apr 28, 2018 at 9:55 PM, Daniel Golle  wrote:
> Some options' default values have been changed upstream, others were
> accidentally inverted (CONFIG_WOLFSSL_HAS_DES3). Also add options
> needed to build hostapd/wpa_supplicant against wolfssl.

Sorry for the late reply.
I wanted to take a look at the patch and check it a bit.
I noticed that your patch is applied now to master.

Overall this is good.
I've been wanting for a while to rework this.
Your approach is better right now before the release [than my rework].

Something like this:
https://github.com/commodo/openwrt/commit/d41ea4f342de7dbb02c9cfb0b19373c39ec24f81
I'll test it a bit more.

One more comment inline below.

>
> Signed-off-by: Daniel Golle 
> ---
>  package/libs/wolfssl/Config.in | 12 
>  package/libs/wolfssl/Makefile  | 33 -
>  2 files changed, 40 insertions(+), 5 deletions(-)
>
> diff --git a/package/libs/wolfssl/Config.in b/package/libs/wolfssl/Config.in
> index 7e8a4b6cee..9b88914516 100644
> --- a/package/libs/wolfssl/Config.in
> +++ b/package/libs/wolfssl/Config.in
> @@ -32,10 +32,22 @@ config WOLFSSL_HAS_PSK
> bool "Include PKS (Pre Share Key) support"
> default n
>
> +config WOLFSSL_HAS_SESSION_TICKET
> +   bool "Include session ticket support"
> +   default n
> +
>  config WOLFSSL_HAS_DTLS
> bool "Include DTLS support"
> default n
>
> +config WOLFSSL_HAS_OCSP
> +   bool "Include OSCP support"
> +   default n
> +
> +config WOLFSSL_HAS_WPAS
> +   bool "Include wpa_supplicant support"
> +   default n
> +
>  config WOLFSSL_HAS_ECC25519
> bool "Include ECC Curve 22519 support"
> depends on WOLFSSL_HAS_ECC
> diff --git a/package/libs/wolfssl/Makefile b/package/libs/wolfssl/Makefile
> index 1d4b7f5579..d0bd3b5a35 100644
> --- a/package/libs/wolfssl/Makefile
> +++ b/package/libs/wolfssl/Makefile
> @@ -9,7 +9,7 @@ include $(TOPDIR)/rules.mk
>
>  PKG_NAME:=wolfssl
>  PKG_VERSION:=3.12.2
> -PKG_RELEASE:=1
> +PKG_RELEASE:=2
>
>  PKG_SOURCE:=$(PKG_NAME)-$(PKG_VERSION).zip
>  PKG_SOURCE_URL:=https://www.wolfssl.com/
> @@ -51,7 +51,9 @@ CONFIGURE_ARGS += \
> --enable-opensslextra \
> --enable-sni \
> --enable-stunnel \
> -   --disable-examples
> +   --disable-examples \
> +   --disable-leanpsk \
> +   --disable-leantls \

Nitpick: these are disabled by default.
No need to disable them here.

>
>  ifeq ($(CONFIG_IPV6),y)
>  CONFIGURE_ARGS += \
> @@ -79,19 +81,25 @@ CONFIGURE_ARGS += \
> --enable-supportedcurves
>  endif
>
> -ifneq ($(CONFIG_WOLFSSL_HAS_DH),y)
> +ifeq ($(CONFIG_WOLFSSL_HAS_DH),y)
>  CONFIGURE_ARGS += \
> --enable-dh
>  endif
>
> -ifeq ($(CONFIG_WOLFSSL_HAS_ARC4),n)
> +ifneq ($(CONFIG_WOLFSSL_HAS_ARC4),y)
>  CONFIGURE_ARGS += \
> --disable-arc4
> +else
> +CONFIGURE_ARGS += \
> +   --enable-arc4
>  endif
>
> -ifeq ($(CONFIG_WOLFSSL_HAS_DES3),y)
> +ifneq ($(CONFIG_WOLFSSL_HAS_DES3),y)
>  CONFIGURE_ARGS += \
> --disable-des3
> +else
> +CONFIGURE_ARGS += \
> +   --enable-des3
>  endif
>
>  ifeq ($(CONFIG_WOLFSSL_HAS_PSK),y)
> @@ -99,11 +107,26 @@ CONFIGURE_ARGS += \
> --enable-psk
>  endif
>
> +ifeq ($(CONFIG_WOLFSSL_HAS_SESSION_TICKET),y)
> +CONFIGURE_ARGS += \
> +   --enable-session-ticket
> +endif
> +
>  ifeq ($(CONFIG_WOLFSSL_HAS_DTLS),y)
>  CONFIGURE_ARGS += \
> --enable-dtls
>  endif
>
> +ifeq ($(CONFIG_WOLFSSL_HAS_OCSP),y)
> +CONFIGURE_ARGS += \
> +   --enable-ocsp --enable-ocspstapling --enable-ocspstapling2
> +endif
> +
> +ifeq ($(CONFIG_WOLFSSL_HAS_WPAS),y)
> +CONFIGURE_ARGS += \
> +   --enable-wpas --enable-sha512 --enable-fortress --enable-fastmath
> +endif
> +
>  ifeq ($(CONFIG_WOLFSSL_HAS_ECC25519),y)
>  CONFIGURE_ARGS += \
> --enable-curve25519

Thanks for this patch :)
Alex

> --
> 2.17.0
>

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

Re: [LEDE-DEV] [PATCH] libjson-c: Update to 0.13

[Alexandru Ardelean]

On Tue, Apr 3, 2018 at 4:33 PM, Karl Palsson <ka...@tweak.net.au> wrote:
>
> Alexandru Ardelean <ardeleana...@gmail.com> wrote:
>> On Sat, Mar 31, 2018 at 9:45 AM, Hans Dedecker
>> <dedec...@gmail.com> wrote:
>> > On Sat, Mar 31, 2018 at 12:25 AM, Rosen Penev <ros...@gmail.com> wrote:
>> >> From: Daniel Engberg <daniel.engberg.li...@pyret.net>
>> >>
>> >> Update (lib)json-c to 0.13
>> > What are the changes?
>> > Is there any size increase ?
>> > Please be a bit more verbose in the git commit description
>> >
>>
>> From me, this is a NAK.
>> See https://github.com/lede-project/source/pull/1575
>>
>> The size increase is reasonably big [percentage-wise +30%, even
>> though the lib is small]. 0.13 adds a few new features, but
>> nothing that is of interest to OpenWrt. The set of features in
>> 0.12 is sufficient for now.
>
> And fixes a pile of bugs, and improves performance :)

Not sure about performance, but I agree on the bugs part.

>
> Please remember that this library is heavily used by packages
> too, not just the ones in core. Yes, it's bigger, but hey, linux
> 4.14 is bigger than linux 4.9 too.

I agree on the linux 4.14 vs 4.9 size increase, but I would not use it
as an argument.
OpenWrt has also run on systems with 2 MB of flash, which is why it's
interesting to have disable flags for whatever is possible.
Or create libjson-c variants [libjson-c-tiny, libjson-c-full].
That may work for both core & packages.

>
>> What would be interesting in 0.13 or later, is to have disable
>> flags, to keep it slim. And maybe switch to cmake, since it's
>> better supported, and preferred [by various users of
>> libjson-c]. Maybe once we have the disable flags, then it would
>> be fine to upgrade.
>
> cmake better supported what? Upstream's "how to build"
> documentation doesn't even _mention_ cmake. The extended docs say
> that cmake is there for win32.

tbh, i did not check the docs about cmake;
i built libjson-c with cmake on my linux desktop a while back; not on
OpenWrt though ;
i also had to push some fixes for linux though:
https://github.com/json-c/json-c/pull/321/files
https://github.com/json-c/json-c/pull/329/files

I'd say, cmake is better supported in 0.13 vs 0.12.
But it's fine if we don't agree here :)

>
> Cheers,
> Karl P

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

Re: [LEDE-DEV] [OpenWrt-Devel] kernel version status

[Alexandru Ardelean]

On Sun, Apr 1, 2018 at 5:48 PM, Hauke Mehrtens  wrote:
> The next OpenWrt release will use kernel 4.9 and kernel 4.14 depending
> on the target. All targets that are *not* on either kernel 4.9 or 4.14
> will not be included in the next release.
>
> I did some overview of the kernel version some months ago here:
> http://lists.infradead.org/pipermail/lede-dev/2017-October/009446.html
> http://lists.infradead.org/pipermail/lede-dev/2018-February/011308.html
>
> Here is the current situation as of today:
>
>
> The following targets are on kernel 4.14 and are fine:
> * apm821xx
> * archs38
> * armvirt
> * bcm53xx
> * cns3xxx
> * imx6
> * ipq40xx
> * kirkwood
> * malta
> * mediatek
> * mvebu
> * mxs
> * octeon
> * octeontx
> * pistachio
> * sunxi
> * x86
>
>
> The following targets are on kernel 4.9 and are fine:
> * ar71xx
> * ar7
> * arc770
> * at91
> There are some patches for kernel 4.14 on the mailing list,
> but it looks like nobody with the hardware wants to take care
> of them.
> * ath25
> * brcm2708
> * brcm47xx
> There is a pull request with kernel 4.14 patches on github, we
> will probably stay with kernel 4.9 for the next release.
> * brcm63xx
> patches for 4.14 are available in master
> * ipq806x
> * ixp4xx
> * lantiq
> patches for 4.14 are available in master, we will probably stay
> with kernel 4.9 for the next release.
> * layerscape
> * mpc85xx
> * omap
> There is a pull request with kernel 4.14 patches on github.
> * orion
> * ramips
> patches for 4.14 are available in master, we will probably stay
> with kernel 4.9 for the next release.
> * rb532
> * uml
>
>
> The following targets are on kernel 4.4 and will probably not be
> included in the next release:
> * gemini
> There are patches for kernel 4.14 on the mailing list, we will
> probably get them in before the release and ship this with 4.14
> * oxnas
> * zynq

Will these targets be dropped/removed from the tree ?
I'd be interested in upgrading the zynq target.
And I also have a partial work/effort in a repo, but not enough time
to test + push it.

I don't need this to be part of the 18.xx release, just curios if it
will still be in the tree, and then I can alloc time for it at a later
time.

Thanks
Alex

>
> The following targets are on kernel 3.18 and will probably not be
> included in the next release:
> * adm5120
> * adm8668
> * au1000
> * mcs814x
> * ppc40x
> * ppc44x
> * xburst
>
>
> This target is on kernel 4.1 (WTF):
> * omap24xx
>
>
> All the targets which are not on kernel 4.9 or 4.14 will probably not be
> included in the next release, I also haven't seen any activity for any
> of them expected the gemini target to get support for more recent kernel
> versions, if you need them please take care now.
>
> I am fine with the current status and do not see this as a blocker for
> the next release, all important targets are on either 4.9 or 4.14, if
> someone wants to see his target on a more recent version we are still
> open for patches.
>
> Hauke
> ___
> openwrt-devel mailing list
> openwrt-de...@lists.openwrt.org
> https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

Re: [LEDE-DEV] [PATCH] libjson-c: Update to 0.13

[Alexandru Ardelean]

On Sat, Mar 31, 2018 at 9:45 AM, Hans Dedecker  wrote:
> On Sat, Mar 31, 2018 at 12:25 AM, Rosen Penev  wrote:
>> From: Daniel Engberg 
>>
>> Update (lib)json-c to 0.13
> What are the changes?
> Is there any size increase ?
> Please be a bit more verbose in the git commit description
>

>From me, this is a NAK.
See https://github.com/lede-project/source/pull/1575

The size increase is reasonably big [percentage-wise +30%, even though
the lib is small].
0.13 adds a few new features, but nothing that is of interest to OpenWrt.
The set of features in 0.12 is sufficient for now.

What would be interesting in 0.13 or later, is to have disable flags,
to keep it slim.
And maybe switch to cmake, since it's better supported, and preferred
[by various users of libjson-c].
Maybe once we have the disable flags, then it would be fine to upgrade.

Thanks
Alex

> Hans
>>
>> Signed-off-by: Daniel Engberg 
>> Signed-off-by: Rosen Penev 
>> ---
>>  package/libs/libjson-c/Makefile   |  4 +--
>>  package/libs/libjson-c/patches/000-libm.patch | 50 
>> ---
>>  2 files changed, 2 insertions(+), 52 deletions(-)
>>  delete mode 100644 package/libs/libjson-c/patches/000-libm.patch
>>
>> diff --git a/package/libs/libjson-c/Makefile 
>> b/package/libs/libjson-c/Makefile
>> index eeb7870f07..72b41e9fb8 100644
>> --- a/package/libs/libjson-c/Makefile
>> +++ b/package/libs/libjson-c/Makefile
>> @@ -8,12 +8,12 @@
>>  include $(TOPDIR)/rules.mk
>>
>>  PKG_NAME:=json-c
>> -PKG_VERSION:=0.12.1
>> +PKG_VERSION:=0.13
>>  PKG_RELEASE:=1
>>
>>  PKG_SOURCE:=$(PKG_NAME)-$(PKG_VERSION)-nodoc.tar.gz
>>  PKG_SOURCE_URL:=https://s3.amazonaws.com/json-c_releases/releases/
>> -PKG_HASH:=5a617da9aade997938197ef0f8aabd7f97b670c216dc173977e1d56eef9e1291
>> +PKG_HASH:=8572760646e9d23ee68f967ca62fa134a97b931665fd9af562192b7788c95a06
>>  PKG_SOURCE_SUBDIR:=$(PKG_NAME)-$(PKG_VERSION)
>>  PKG_BUILD_DIR:=$(BUILD_DIR)/$(PKG_SOURCE_SUBDIR)
>>
>> diff --git a/package/libs/libjson-c/patches/000-libm.patch 
>> b/package/libs/libjson-c/patches/000-libm.patch
>> deleted file mode 100644
>> index 45adb0597f..00
>> --- a/package/libs/libjson-c/patches/000-libm.patch
>> +++ /dev/null
>> @@ -1,50 +0,0 @@
>>  a/configure.ac
>> -+++ b/configure.ac
>> -@@ -43,12 +43,6 @@
>> - AC_FUNC_MALLOC
>> - AC_FUNC_REALLOC
>> - AC_CHECK_FUNCS(strcasecmp strdup strerror snprintf vsnprintf vasprintf 
>> open vsyslog strncasecmp setlocale)
>> --AC_CHECK_DECLS([INFINITY], [], [], [[#include ]])
>> --AC_CHECK_DECLS([nan], [], [], [[#include ]])
>> --AC_CHECK_DECLS([isnan], [], [], [[#include ]])
>> --AC_CHECK_DECLS([isinf], [], [], [[#include ]])
>> --AC_CHECK_DECLS([_isnan], [], [], [[#include ]])
>> --AC_CHECK_DECLS([_finite], [], [], [[#include ]])
>> -
>> - #check if .section.gnu.warning accepts long strings (for __warn_references)
>> - AC_LANG_PUSH([C])
>>  a/math_compat.h
>> -+++ b/math_compat.h
>> -@@ -1,28 +1,9 @@
>> - #ifndef __math_compat_h
>> - #define __math_compat_h
>> -
>> --/* Define isnan and isinf on Windows/MSVC */
>> --
>> --#ifndef HAVE_DECL_ISNAN
>> --# ifdef HAVE_DECL__ISNAN
>> --#include 
>> --#define isnan(x) _isnan(x)
>> --# endif
>> --#endif
>> --
>> --#ifndef HAVE_DECL_ISINF
>> --# ifdef HAVE_DECL__FINITE
>> --#include 
>> --#define isinf(x) (!_finite(x))
>> --# endif
>> --#endif
>> --
>> --#ifndef HAVE_DECL_NAN
>> --#error This platform does not have nan()
>> --#endif
>> --
>> --#ifndef HAVE_DECL_INFINITY
>> --#error This platform does not have INFINITY
>> --#endif
>> -+#undef isnan
>> -+#define isnan(x) __builtin_isnan(x)
>> -+#undef isinf
>> -+#define isinf(x) __builtin_isinf(x)
>> -
>> - #endif
>> --
>> 2.16.3
>>
>>
>> ___
>> Lede-dev mailing list
>> Lede-dev@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/lede-dev
>
> ___
> Lede-dev mailing list
> Lede-dev@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/lede-dev

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

Re: [LEDE-DEV] [PATCH] uboot-envtools: Change download to git.

[Alexandru Ardelean]

On Mon, Feb 26, 2018 at 11:25 PM, Rosen Penev  wrote:
> Currently, the build system uses an openwrt mirror which does not currently 
> workand FTP can be unreliable under several circumstances (Ubuntu 16.04 WSL 
> being an example). This change implicitly allows using all the mirrors to 
> download.
>
> Changing this to git also allows using .tar.xz archives which are smaller.
>
> Size difference:
>
> 10416503 u-boot-2015.10.tar.bz2
> 8351456 u-boot-2015.10.tar.xz
>

this is still not worth mentioning if you are switching to a git repo ;
due to the traffic generated by downloading from a git repo ;

> Signed-off-by: Rosen Penev 
>
> v2: Change git URL from GitHub to official mirror.
> ---
>  package/boot/uboot-envtools/Makefile | 13 +++--
>  1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/package/boot/uboot-envtools/Makefile 
> b/package/boot/uboot-envtools/Makefile
> index 57a2ec5393..ba6bae6cfe 100644
> --- a/package/boot/uboot-envtools/Makefile
> +++ b/package/boot/uboot-envtools/Makefile
> @@ -12,12 +12,13 @@ PKG_DISTNAME:=u-boot
>  PKG_VERSION:=2015.10
>  PKG_RELEASE:=1
>
> -PKG_BUILD_DIR:=$(BUILD_DIR)/u-boot-$(PKG_VERSION)
> -PKG_SOURCE:=$(PKG_DISTNAME)-$(PKG_VERSION).tar.bz2
> -PKG_SOURCE_URL:=\
> -   http://mirror2.openwrt.org/sources \
> -   ftp://ftp.denx.de/pub/u-boot
> -PKG_HASH:=bdc68d5f9455ad933b059c735d983f2c8b6b552dafb062e5ff1444f623021955
> +PKG_SOURCE_PROTO:=git
> +PKG_SOURCE:=$(PKG_DISTNAME)-$(PKG_VERSION).tar.xz
> +PKG_SOURCE_SUBDIR:=$(PKG_DISTNAME)-$(PKG_VERSION)
> +PKG_BUILD_DIR:=$(BUILD_DIR)/$(PKG_DISTNAME)-$(PKG_VERSION)
> +PKG_SOURCE_URL:=git://git.denx.de/u-boot.git

this would not work for people behind a firewall that allows only
HTTP(S) traffic;
HTTPS/HTTP is nicer for that, or git over HTTP(S) if available;

My preference is still towards using HTTP(S) over using git ;
especially if there is an official HTTP(S) mirror available.
For downloading tarballs it's a less-complicated protocol than cloning
a git repo.

> +PKG_SOURCE_VERSION:=5ec0003b19cbdf06ccd6941237cbc0d1c3468e2d
> +PKG_MIRROR_HASH:=e207d996ebfff7335eed99789e3dcb9da071499f347fcdd86725b9d4dac5a5bb
>
>  PKG_BUILD_DEPENDS:=fstools
>
> --
> 2.14.3
>
>
> ___
> Lede-dev mailing list
> Lede-dev@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/lede-dev

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

Re: [LEDE-DEV] [PATCH] metadata: compile dependencies only when the package is selected

[Alexandru Ardelean]

On Wed, Feb 28, 2018 at 7:07 AM, Yousong Zhou  wrote:
> This is intended to reduce build time for situations like the following
> where python and python-six and their dependencies could still be built
> as long as any subpackage within the srcpackage was selected regardless
> of the selection state of openvswitch-python
>
> define Package/openvswitch-python
>   ...
>   DEPENDS:=+python +python-six
> endef
>
> Previously we work around this by specifying the dependency as
> +PACKAGE_openvswitch-python:python, which is unintuitive

This is pretty cool actually.
I never thought of changing this logic ; I just took this behavior as
a given behavior for packages.

Thanks for this
Alex

>
> Signed-off-by: Yousong Zhou 
> ---
>  scripts/package-metadata.pl | 13 ++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/package-metadata.pl b/scripts/package-metadata.pl
> index 53bb45a..b827154 100755
> --- a/scripts/package-metadata.pl
> +++ b/scripts/package-metadata.pl
> @@ -382,6 +382,7 @@ sub gen_package_mk() {
> my %deplines = ('' => {});
>
> foreach my $pkg (@{$src->{packages}}) {
> +   my @pkgdeplines;
> foreach my $dep (@{$pkg->{depends}}) {
> next if ($dep =~ /@/);
>
> @@ -410,10 +411,17 @@ sub gen_package_mk() {
> }
> my $depline = 
> get_conditional_dep($condition, $depstr);
> if ($depline) {
> -   $deplines{''}{$depline}++;
> +   push @pkgdeplines, $depline;
> }
> }
> }
> +   if (@pkgdeplines) {
> +   my $depstr = join(' ', @pkgdeplines);
> +   if ($depstr) {
> +   my $depline = 
> get_conditional_dep('PACKAGE_'.$pkg->{name}, $depstr);
> +   $deplines{''}{$depline}++;
> +   }
> +   }
>
> my $config = '';
> $config = sprintf '$(CONFIG_PACKAGE_%s)', 
> $pkg->{name} unless $pkg->{buildonly};
> @@ -486,8 +494,7 @@ sub gen_package_mk() {
> }
>
> foreach my $suffix (sort keys %deplines) {
> -   my $depline = join(" ", sort keys 
> %{$deplines{$suffix}});
> -   if ($depline) {
> +   for my $depline (sort keys %{$deplines{$suffix}}) {
> $line .= sprintf "\$(curdir)/%s/compile += 
> %s\n", $src->{path}.$suffix, $depline;
> }
> }
> --
> 1.8.3.1
>

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

Re: [LEDE-DEV] [PATCH] uboot-envtools: Change download to git.

[Alexandru Ardelean]

On Fri, Feb 23, 2018 at 3:48 AM, Rosen Penev  wrote:
> Currently, the build system uses an openwrt mirror which does not currently 
> workand FTP can be unreliable under several circumstances (Ubuntu 16.04 WSL 
> being an example). This change implicitly allows using all the mirrors to 
> download.
>
> Changing this to git also allows using .tar.xz archives which are smaller.
>
> Size difference:
>
> 10416503 u-boot-2015.10.tar.bz2
> 8351456 u-boot-2015.10.tar.xz

The size difference is pointless here if you are about to do a git
clone of a repository.
The git history alone can add a transfer 2-10 times bigger than the
tar.bz2 file [depending on the repo's git history].

>
> Signed-off-by: Rosen Penev 
> ---
>  package/boot/uboot-envtools/Makefile | 13 +++--
>  1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/package/boot/uboot-envtools/Makefile 
> b/package/boot/uboot-envtools/Makefile
> index 57a2ec5393..4d2cd342aa 100644
> --- a/package/boot/uboot-envtools/Makefile
> +++ b/package/boot/uboot-envtools/Makefile
> @@ -12,12 +12,13 @@ PKG_DISTNAME:=u-boot
>  PKG_VERSION:=2015.10
>  PKG_RELEASE:=1
>
> -PKG_BUILD_DIR:=$(BUILD_DIR)/u-boot-$(PKG_VERSION)
> -PKG_SOURCE:=$(PKG_DISTNAME)-$(PKG_VERSION).tar.bz2
> -PKG_SOURCE_URL:=\
> -   http://mirror2.openwrt.org/sources \
> -   ftp://ftp.denx.de/pub/u-boot

What about switching the FTP site to http://ftp.denx.de/pub/u-boot/ ?
[At least] I prefer HTTP over git, and if FTP is problematic, HTTP
seems reliable for this.
HTTPS seems to exist: https://ftp.denx.de/pub/u-boot/
But it seems to point to some owncloud/nextcloud instance, so that is
unusable atm.

> -PKG_HASH:=bdc68d5f9455ad933b059c735d983f2c8b6b552dafb062e5ff1444f623021955
> +PKG_SOURCE_PROTO:=git
> +PKG_SOURCE:=$(PKG_DISTNAME)-$(PKG_VERSION).tar.xz
> +PKG_SOURCE_SUBDIR:=$(PKG_DISTNAME)-$(PKG_VERSION)
> +PKG_BUILD_DIR:=$(BUILD_DIR)/$(PKG_DISTNAME)-$(PKG_VERSION)
> +PKG_SOURCE_URL:=https://github.com/u-boot/u-boot
> +PKG_SOURCE_VERSION:=5ec0003b19cbdf06ccd6941237cbc0d1c3468e2d
> +PKG_MIRROR_HASH:=e207d996ebfff7335eed99789e3dcb9da071499f347fcdd86725b9d4dac5a5bb
>
>  PKG_BUILD_DEPENDS:=fstools
>
> --
> 2.14.3
>
>
> ___
> Lede-dev mailing list
> Lede-dev@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/lede-dev

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

Re: [LEDE-DEV] [OpenWrt-Devel] patchwork

[Alexandru Ardelean]

On Fri, Jan 19, 2018 at 10:09 AM, John Crispin <j...@phrozen.org> wrote:
>
>
> On 19/01/18 08:55, Alexandru Ardelean wrote:
>>
>> On Fri, Jan 19, 2018 at 3:02 AM, Val Kulkov <val.kul...@gmail.com> wrote:
>>>
>>> On 18 January 2018 at 19:49, Alberto Bursi <bobafetthotm...@gmail.com>
>>> wrote:
>>>>
>>>>
>>>>
>>>> On 01/19/2018 01:05 AM, Val Kulkov wrote:
>>>>>
>>>>> There is more than a handful of PRs currently bit-rotting in
>>>>> openwrt/packages that are ready for merging, with all requested
>>>>> changes in place since many months ago. Auto-closing such PRs will
>>>>> offend the contributors who would see their effort go down the drain
>>>>> only because no one in the LEDE/OpenWrt community had the time to
>>>>> review and merge their PRs.
>>>>
>>>>
>>>> Github has "labels" for PRs, so I think such timeout should look for
>>>> "needs changes" label or something like that.
>>>>
>>>> See this PR https://github.com/openwrt/openwrt/pull/655 (on the right,
>>>> the red label)
>>>>
>>>> -Alberto
>>>
>>> Problem is, the "Change requested" label does not necessarily mean
>>> that the requested changes have not been implemented by the
>>> contributors.
>>>
>>> There have been cases where a PRs gets labelled with "Change
>>> requested", then the contributor makes all changes as requested, and
>>> then nothing happens for many months because no one among members with
>>> write privileges has the time to review and merge the PR.
>>>
>> I feel we are complicating things a bit too much before we've even started
>> :)
>> I think it would be good to start as simple as possible and see what
>> other pain-points arise.
>>
>> Offending people with auto-closed PRs is a potential issue.
>> This is already [sort of] happening with delayed PRs/submissions.
>> Best I can think of handling this is the wording of the auto-close
>> message.
>>
>> But I would not worry about making the lives of contributors easier
>> [since they come & go].
>> I would worry about making the lives of core devs easier, since their
>> number is rarely changing, and they have to put in the effort.
>
>
> I started working on a script, here is how it works. its pretty much the
> same for patchwork and github with the difference that on github we write
> comments and patchwork we send mails.
> after 90 days we send a note saying "it stalled pleases remind folks and
> help get it merged" after 120 days we close it with a note saying "sorry try
> again, something did not work out".



after
"sorry try again, something did not work out".
it could also add:
"it's not you, it's me"



>
> John
>
>
>

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

Re: [LEDE-DEV] [OpenWrt-Devel] patchwork

[Alexandru Ardelean]

On Fri, Jan 19, 2018 at 3:02 AM, Val Kulkov  wrote:
> On 18 January 2018 at 19:49, Alberto Bursi  wrote:
>>
>>
>>
>> On 01/19/2018 01:05 AM, Val Kulkov wrote:
>>>
>>> There is more than a handful of PRs currently bit-rotting in
>>> openwrt/packages that are ready for merging, with all requested
>>> changes in place since many months ago. Auto-closing such PRs will
>>> offend the contributors who would see their effort go down the drain
>>> only because no one in the LEDE/OpenWrt community had the time to
>>> review and merge their PRs.
>>
>>
>> Github has "labels" for PRs, so I think such timeout should look for "needs 
>> changes" label or something like that.
>>
>> See this PR https://github.com/openwrt/openwrt/pull/655 (on the right, the 
>> red label)
>>
>> -Alberto
>
> Problem is, the "Change requested" label does not necessarily mean
> that the requested changes have not been implemented by the
> contributors.
>
> There have been cases where a PRs gets labelled with "Change
> requested", then the contributor makes all changes as requested, and
> then nothing happens for many months because no one among members with
> write privileges has the time to review and merge the PR.
>

I feel we are complicating things a bit too much before we've even started :)
I think it would be good to start as simple as possible and see what
other pain-points arise.

Offending people with auto-closed PRs is a potential issue.
This is already [sort of] happening with delayed PRs/submissions.
Best I can think of handling this is the wording of the auto-close message.

But I would not worry about making the lives of contributors easier
[since they come & go].
I would worry about making the lives of core devs easier, since their
number is rarely changing, and they have to put in the effort.

> ___
> Lede-dev mailing list
> Lede-dev@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/lede-dev

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

Re: [LEDE-DEV] Bug when processing long lines

[Alexandru Ardelean]

On Thu, Jan 11, 2018 at 6:28 PM, Jakub Horák  wrote:
> Hello LEDE developers,
>
> I found a bug in procd that gets triggered when long lines are printed
> by services whose stdout/stderr are being logged. The bug itself is
> explained in the attached patch.
>
> However, when I was testing the fix, I found out that the bug is present
> in other places, mostly those that call "ustream_get_read_buf" function.
> Some examples:
>
> - ubox/log/logread.c:logread_fb_data_cb() - when buffer passed on the
> descriptor is larger than 4096, reception stops
> - netifd/main.c:netifd_process_log_read_cb - this is a place that seems
> to have this bug fixed, but still has incorrect handling of NUL bytes
> - libubox/examples/ustream-example.c:client_read_cb - this is probably
> the place that originated this broken bit of code
> - uhttpd/relay.c:relay_process_headers - another place that seems broken
>
> I've attached an init script (that goes to /etc/init.d/flood) and three
> Lua programs (flood[123].lua) that trigger this behavior:
> - flood1.lua writes long message to stdout, that triggers this behavior
> in procd
> - flood2.lua writes message that gets correctly processed by procd, but
> triggers the bug in logread
> - flood3.lua writes message with embedded zeros
>
> I don't post patches to mailing lists very often, so I apologize if I'm
> sending this in a wrong format or in a too broken english.

Hey,

Patches usually are sent with git send-mail.
So  "git send-mail
0001-procd-Fix-behavior-when-parsing-long-lines.patch
--to=openwrt-de...@lists.openwrt.org"
[ FWIW: LEDE has merged back to OpenWrt :) ]

Now about the patch.

-   str = ustream_get_read_buf(s, NULL);
+   str = ustream_get_read_buf(s, );
if (!str)
break;

-   newline = strchr(str, '\n');
-   if (!newline)
-   break;
-
-   *newline = 0;
+   /* search for '\n', take into account NUL bytes */
+   newline = memchr(str, '\n', buflen);
+   if (!newline) {
+   /* is there a room in buffer? */
+   if (buflen < s->r.buffer_len) {
+   /* yes - bailout, newline may still come */
+   break;
+   } else {
+   /* no - force newline */
+   len = buflen;

It's weird that this would happen here, since there should be a
ustream_reserve() call that would guarantee that there is sufficient
buffer size.
I could be wrong, or it could be a bug somewhere; who knows ?

In any case, if this is a correct approach, I think you should also
add  *(str + len) = 0  ; to make sure the string is null-terminated.

+   }
+   } else {
+   *newline = 0;
+   len = newline + 1 - str;
+   }
+   /* "str" is NUL terminated by ustream_get_read_buf */
ulog(prio, "%s\n", str);
-
-   len = newline + 1 - str;
ustream_consume(s, len);


Alex

>
> Best regards,
> Jakub Horak
>
> ___
> Lede-dev mailing list
> Lede-dev@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/lede-dev
>

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

[LEDE-DEV] partial port of DPDK

[Alexandru Ardelean]

Hey,

I remember seeing some interest a while back in DPDK.

I started a port of DPDK for some experiment I wanted to do, but in
the meantime I stopped because of reasons.

Here's the current code so far:
https://github.com/commodo/packages/tree/dpdk/net/dpdk

It should build in the current master of OpenWrt. I tried it real quick now.
I did not manage to try it much.
It compiles on x86_64 ; I haven't tried to compile it on any other
platform/target,

If anyone wants to take this and maintain it, feel free to do so ; you
don't even need to mention me anywhere.

Alex

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

[LEDE-DEV] [PATCH][netifd] handler: replace is_error() helper with NULL check

[Alexandru Ardelean]

The `is_error()` is just a macro that checks
that object is NULL (which is considered an error
in libjson-c terminology).

Newer libjson-c versions have deprecated this.

Signed-off-by: Alexandru Ardelean <ardeleana...@gmail.com>
---
 handler.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/handler.c b/handler.c
index 0c4627f..a0b2a57 100644
--- a/handler.c
+++ b/handler.c
@@ -105,7 +105,7 @@ netifd_parse_script_handler(const char *name, 
script_dump_cb cb)
tok = json_tokener_new();
 
obj = json_tokener_parse_ex(tok, start, len);
-   if (!is_error(obj)) {
+   if (obj) {
netifd_init_script_handler(name, obj, cb);
json_object_put(obj);
json_tokener_free(tok);
-- 
2.11.0


___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

Re: [LEDE-DEV] [PATCH] handler: replace is_error() helper with NULL check

[Alexandru Ardelean]

On Fri, Dec 8, 2017 at 4:19 PM, Alexandru Ardelean
<ardeleana...@gmail.com> wrote:
> The `is_error()` is just a macro that checks
> that object is NULL (which is considered an error
> in libjson-c terminology).
>
> Newer libjson-c versions have deprecated this.

I forgot to add the [netifd] tag.

>
> Signed-off-by: Alexandru Ardelean <ardeleana...@gmail.com>
> ---
>  handler.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/handler.c b/handler.c
> index 0c4627f..a0b2a57 100644
> --- a/handler.c
> +++ b/handler.c
> @@ -105,7 +105,7 @@ netifd_parse_script_handler(const char *name, 
> script_dump_cb cb)
> tok = json_tokener_new();
>
> obj = json_tokener_parse_ex(tok, start, len);
> -   if (!is_error(obj)) {
> +   if (obj) {
> netifd_init_script_handler(name, obj, cb);
> json_object_put(obj);
> json_tokener_free(tok);
> --
> 2.11.0
>

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

[LEDE-DEV] [PATCH] handler: replace is_error() helper with NULL check

[Alexandru Ardelean]

The `is_error()` is just a macro that checks
that object is NULL (which is considered an error
in libjson-c terminology).

Newer libjson-c versions have deprecated this.

Signed-off-by: Alexandru Ardelean <ardeleana...@gmail.com>
---
 handler.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/handler.c b/handler.c
index 0c4627f..a0b2a57 100644
--- a/handler.c
+++ b/handler.c
@@ -105,7 +105,7 @@ netifd_parse_script_handler(const char *name, 
script_dump_cb cb)
tok = json_tokener_new();
 
obj = json_tokener_parse_ex(tok, start, len);
-   if (!is_error(obj)) {
+   if (obj) {
netifd_init_script_handler(name, obj, cb);
json_object_put(obj);
json_tokener_free(tok);
-- 
2.11.0


___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

Re: [LEDE-DEV] [PATCH] jshn: add functionality to read big JSON

[Alexandru Ardelean]

On Sat, Nov 18, 2017 at 3:55 PM, Christian Beier <dontm...@sdf.org> wrote:
> Am Fri, 17 Nov 2017 09:17:39 +0200
> schrieb Alexandru Ardelean <ardeleana...@gmail.com>:
>
>> On Thu, Nov 16, 2017 at 8:52 PM, Christian Beier <dontm...@freeshell.org>
>> wrote:
>> > The existing read functionality feeds the complete JSON to jshn as a
>> > cmdline argument, leading to `-ash: jshn: Argument list too long`
>> > errors for JSONs bigger than ca. 100KB.
>> >
>> > This commit adds the ability to read the JSON directly from a file if
>> > wanted, removing this shell-imposed size limit.
>> >
>> > Tested on x86-64 and ar71xx. An mmap()-based solution was also evaluated,
>> > but found to make no performance difference on either platform.
>> >
>> > Signed-off-by: Christian Beier <dontm...@freeshell.org>
>> > ---
>> >  jshn.c | 30 --
>> >  sh/jshn.sh |  4 
>> >  2 files changed, 32 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/jshn.c b/jshn.c
>> > index 79136dd..a3866a0 100644
>> > --- a/jshn.c
>> > +++ b/jshn.c
>> > @@ -25,6 +25,8 @@
>> >  #include 
>> >  #include 
>> >  #include 
>> > +#include 
>> > +#include 
>> >  #include "list.h"
>> >
>> >  #include "avl.h"
>> > @@ -300,7 +302,7 @@ out:
>> >
>> >  static int usage(const char *progname)
>> >  {
>> > -   fprintf(stderr, "Usage: %s [-n] [-i] -r |-w\n", progname);
>> > +   fprintf(stderr, "Usage: %s [-n] [-i] -r |-R |-w\n",
>> > progname); return 2;
>> >  }
>> >
>> > @@ -333,6 +335,10 @@ int main(int argc, char **argv)
>> > struct env_var *vars;
>> > int i;
>> > int ch;
>> > +   int fd;
>> > +   struct stat sb;
>> > +   char *fbuf;
>> > +   int ret;
>> >
>> > avl_init(_vars, avl_strcmp_var, false, NULL);
>> > for (i = 0; environ[i]; i++);
>> > @@ -354,7 +360,7 @@ int main(int argc, char **argv)
>> > avl_insert(_vars, [i].avl);
>> > }
>> >
>> > -   while ((ch = getopt(argc, argv, "p:nir:w")) != -1) {
>> > +   while ((ch = getopt(argc, argv, "p:nir:R:w")) != -1) {
>> > switch(ch) {
>> > case 'p':
>> > var_prefix = optarg;
>> > @@ -362,6 +368,26 @@ int main(int argc, char **argv)
>> > break;
>> > case 'r':
>> > return jshn_parse(optarg);
>> > +   case 'R':
>> > +   if((fd = open(optarg, O_RDONLY)) == -1) {
>> > +   fprintf(stderr, "Error opening %s\n", optarg);
>> > +   return 3;
>> > +   }
>> > +   if (fstat(fd, ) == -1) {
>> > +   fprintf(stderr, "Error getting size of %s\n",
>> > optarg);
>> > +   close(fd);
>> > +   return 3;
>> > +   }
>> > +   if(!(fbuf = malloc(sb.st_size)) || read(fd, fbuf,
>> > sb.st_size) != sb.st_size) {
>> > +   fprintf(stderr, "Error reading %s\n", optarg);
>> > +   free(fbuf);
>> > +   close(fd);
>> > +   return 3;
>> > +   }
>> > +   ret = jshn_parse(fbuf);
>> > +   free(fbuf);
>> > +   close(fd);
>> > +   return ret;
>> nitpick/preference: I would move this code into a file, so that the
>> case statement is not too loaded
>> it would allow for a simpler/cleaner code-path with some goto
>> statements [if put into a function]
>
> Yeah, I tried keeping all those error case printf and return blocks as minimal
> as possible, though it still boils down to 3. Might be an idea to factor out 
> the
> file-opening-and-reading into a separate function to keep the switch statement
> cleaner, but then OTOH it'd be a function with only one caller.
>
> Also, I was unsure if it'd be overkill to assign different exit codes to the
> different error conditions, 

Re: [LEDE-DEV] [PATCH] jshn: add functionality to read big JSON

[Alexandru Ardelean]

On Thu, Nov 16, 2017 at 8:52 PM, Christian Beier  wrote:
> The existing read functionality feeds the complete JSON to jshn as a
> cmdline argument, leading to `-ash: jshn: Argument list too long`
> errors for JSONs bigger than ca. 100KB.
>
> This commit adds the ability to read the JSON directly from a file if
> wanted, removing this shell-imposed size limit.
>
> Tested on x86-64 and ar71xx. An mmap()-based solution was also evaluated,
> but found to make no performance difference on either platform.
>
> Signed-off-by: Christian Beier 
> ---
>  jshn.c | 30 --
>  sh/jshn.sh |  4 
>  2 files changed, 32 insertions(+), 2 deletions(-)
>
> diff --git a/jshn.c b/jshn.c
> index 79136dd..a3866a0 100644
> --- a/jshn.c
> +++ b/jshn.c
> @@ -25,6 +25,8 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
>  #include "list.h"
>
>  #include "avl.h"
> @@ -300,7 +302,7 @@ out:
>
>  static int usage(const char *progname)
>  {
> -   fprintf(stderr, "Usage: %s [-n] [-i] -r |-w\n", progname);
> +   fprintf(stderr, "Usage: %s [-n] [-i] -r |-R |-w\n", 
> progname);
> return 2;
>  }
>
> @@ -333,6 +335,10 @@ int main(int argc, char **argv)
> struct env_var *vars;
> int i;
> int ch;
> +   int fd;
> +   struct stat sb;
> +   char *fbuf;
> +   int ret;
>
> avl_init(_vars, avl_strcmp_var, false, NULL);
> for (i = 0; environ[i]; i++);
> @@ -354,7 +360,7 @@ int main(int argc, char **argv)
> avl_insert(_vars, [i].avl);
> }
>
> -   while ((ch = getopt(argc, argv, "p:nir:w")) != -1) {
> +   while ((ch = getopt(argc, argv, "p:nir:R:w")) != -1) {
> switch(ch) {
> case 'p':
> var_prefix = optarg;
> @@ -362,6 +368,26 @@ int main(int argc, char **argv)
> break;
> case 'r':
> return jshn_parse(optarg);
> +   case 'R':
> +   if((fd = open(optarg, O_RDONLY)) == -1) {
> +   fprintf(stderr, "Error opening %s\n", optarg);
> +   return 3;
> +   }
> +   if (fstat(fd, ) == -1) {
> +   fprintf(stderr, "Error getting size of %s\n", 
> optarg);
> +   close(fd);
> +   return 3;
> +   }
> +   if(!(fbuf = malloc(sb.st_size)) || read(fd, fbuf, 
> sb.st_size) != sb.st_size) {
> +   fprintf(stderr, "Error reading %s\n", optarg);
> +   free(fbuf);
> +   close(fd);
> +   return 3;
> +   }
> +   ret = jshn_parse(fbuf);
> +   free(fbuf);
> +   close(fd);
> +   return ret;
nitpick/preference: I would move this code into a file, so that the
case statement is not too loaded
it would allow for a simpler/cleaner code-path with some goto
statements [if put into a function]

> case 'w':
> return jshn_format(no_newline, indent);
> case 'n':
> diff --git a/sh/jshn.sh b/sh/jshn.sh
> index bf76edb..0a146e1 100644
> --- a/sh/jshn.sh
> +++ b/sh/jshn.sh
> @@ -174,6 +174,10 @@ json_load() {
> eval "`jshn -r "$1"`"
>  }
>
> +json_load_file() {
> +   eval "`jshn -R "$1"`"
would it be an idea to try to use $JSON_PREFIX here ?
for cases when you need to change the JSON namespace, the JSON_PREFIX
var is used if available
it would definitely complete this function

> +}
> +
>  json_dump() {
> jshn "$@" ${JSON_PREFIX:+-p "$JSON_PREFIX"} -w
>  }
> --
> 2.11.0
>

from my side [as a simple reviewer], this looks pretty neat

thanks :)

Alex

>
> ___
> Lede-dev mailing list
> Lede-dev@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/lede-dev

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

Re: [LEDE-DEV] [PATCH] ubus: Remove unnecessary memset calls.

[Alexandru Ardelean]

On Tue, Nov 7, 2017 at 11:18 PM, Arjen de Korte  wrote:
> Citeren Rosen Penev :
>
>> I beg to differ. https://vorpus.org/blog/why-does-calloc-exist/
>>
>> Section 2.
>
>
> I don't care about theoretical gains, benchmarks please. How much do you
> gain with these patches? I really doubt that in any of these patches there
> will be a tangible gain.

This feels like an opinion battle :)

I'd also vote for calloc [vs malloc + memset] mostly for reduction of
line numbers and/or cleaning up.

I also feel that Rosen's argument for "faster in extreme situations"
is somewhat "playing it by the ear".
But I would not dismiss this based on that.

These days compilers do so much optimization, it's hard to evaluate
performance of one case vs another.
For this case [specifically], we would need to see/check the assembly
code for a few architectures [mips, powerpc, arm, x86, etc] to
properly evaluate the performance improvement of calloc() vs malloc()
+ memset().
I feel that effort would be a too much for such a simple/trivial change.

I would re-spin this with just the "cleaning up" part [since I feel
there is more agreement on that].

But feel free do disagree with me :)

>
>> On Tue, Nov 7, 2017 at 12:46 PM, Arjen de Korte 
>> wrote:
>>>
>>> Citeren Rosen Penev :
>>>
 Replace malloc+memset with calloc. Cleaner and faster in extreme
 situations.
>>>
>>>
>>>
>>> Calloc is definitly *not* faster than malloc + memset. Under the hood,
>>> calloc will call malloc, check if memory allocation was successful and
>>> then
>>> proceed to set all allocated memory to 0. You need to check the return
>>> value
>>> of malloc or calloc anyway, so this will be actually slower.
>>>
>>> It *may* be considered cleaner, since it will check if the memory
>>> allocation
>>> succeeded before writing zeros.
>>>
>>>
 Signed-off-by: Rosen Penev 
 ---
  libubus.c  |  6 ++
  lua/ubus.c | 18 ++
  2 files changed, 8 insertions(+), 16 deletions(-)

 diff --git a/libubus.c b/libubus.c
 index 9463522..260e40f 100644
 --- a/libubus.c
 +++ b/libubus.c
 @@ -133,7 +133,7 @@ struct ubus_lookup_request {
  static void ubus_lookup_cb(struct ubus_request *ureq, int type, struct
 blob_attr *msg)
  {
 struct ubus_lookup_request *req;
 -   struct ubus_object_data obj;
 +   struct ubus_object_data obj = {};
 struct blob_attr **attr;

 req = container_of(ureq, struct ubus_lookup_request, req);
 @@ -143,7 +143,6 @@ static void ubus_lookup_cb(struct ubus_request
 *ureq,
 int type, struct blob_attr
 !attr[UBUS_ATTR_OBJTYPE])
 return;

 -   memset(, 0, sizeof(obj));
 obj.id = blob_get_u32(attr[UBUS_ATTR_OBJID]);
 obj.path = blob_data(attr[UBUS_ATTR_OBJPATH]);
 obj.type_id = blob_get_u32(attr[UBUS_ATTR_OBJTYPE]);
 @@ -220,7 +219,7 @@ int ubus_register_event_handler(struct ubus_context
 *ctx,
 const char *pattern)
  {
 struct ubus_object *obj = >obj;
 -   struct blob_buf b2;
 +   struct blob_buf b2 = {};
 int ret;

 if (!obj->id) {
 @@ -236,7 +235,6 @@ int ubus_register_event_handler(struct ubus_context
 *ctx,
 }

 /* use a second buffer, ubus_invoke() overwrites the primary one
 */
 -   memset(, 0, sizeof(b2));
 blob_buf_init(, 0);
 blobmsg_add_u32(, "object", obj->id);
 if (pattern)
 diff --git a/lua/ubus.c b/lua/ubus.c
 index 74a15b0..e9fd10e 100644
 --- a/lua/ubus.c
 +++ b/lua/ubus.c
 @@ -410,11 +410,10 @@ static int ubus_lua_load_methods(lua_State *L,
 struct ubus_method *m)
 }

 /* setup the policy pointers */
 -   p = malloc(sizeof(struct blobmsg_policy) * plen);
 +   p = calloc(plen, sizeof(struct blobmsg_policy));
 if (!p)
 return 1;

 -   memset(p, 0, sizeof(struct blobmsg_policy) * plen);
 m->policy = p;
 lua_pushnil(L);
 while (lua_next(L, -2) != 0) {
 @@ -481,26 +480,23 @@ static struct ubus_object*
 ubus_lua_load_object(lua_State *L)
 int midx = 0;

 /* setup object pointers */
 -   obj = malloc(sizeof(struct ubus_lua_object));
 +   obj = calloc(1, sizeof(struct ubus_lua_object));
 if (!obj)
 return NULL;

 -   memset(obj, 0, sizeof(struct ubus_lua_object));
 obj->o.name = lua_tostring(L, -2);

 /* setup method pointers */
 -   m = malloc(sizeof(struct ubus_method) * mlen);
 -   memset(m, 0, sizeof(struct ubus_method) * mlen);
 +   m = calloc(1, 

Re: [LEDE-DEV] [PATCH] netifd-proto.sh: add ip4table & ip6table to `proto_add_dynamic_defaults()`

[Alexandru Ardelean]

On Sat, Sep 9, 2017 at 10:51 PM, Baptiste Jonglez
<bapti...@bitsofnetworks.org> wrote:
> On 09-09-17, Baptiste Jonglez wrote:
>> On 05-09-17, Alexandru Ardelean wrote:
>> > The `proto_add_dynamic_defaults()` seems to be called mostly
>> > in the context of LTE/3G modems (via wwan, qmi, etc) setup.
>> >
>> > When they get setup, these devices override default routes.
>> >
>> > However, depending on setup, we want these modems to
>> > be part of a another routing table.
>> > This change allows that.
>> >
>> > ip4table/ip6table are of string type in netifd to allow
>> > for `default`, `local` routing table names to be specified.
>>
>> Just a remark on the names: "ip4table" and "ip6table" make it looks like
>> it's related to firewall.  But this has nothing to do with firewalls.
>>
>> Maybe use "routingtable" and "routingtablev6" instead?  Or just
>> "table"/"tablev6"?
>
> It seems that "ip4table" and "ip6table" is already understood by other
> protocols, so you can disregard my remark:
>
> https://wiki.openwrt.org/doc/uci/network#options_valid_for_all_protocol_types

This got into my spam folder for some reason.
GMail is weird sometimes.

I still have to try this out a bit more.
I'm load-balancing various stuff, so I did not get to try out stuff in depth.

Atm I'm doing:
config interface 'usb0_1'
 option ifname 'usb0'
 option proto 'wwan'
 option ip4table 100
 option ip6table 100

This seems to make an interface [in netifd] called
network.interface.usb0_1  which adds the ip4table/ip6table to usb0.
However, for some reason [likely a bug with my config generation code]
the DHCP part is not initializing properly ; well for some reason no
RX packets are incoming.
I'll dig into that.

My trip through getting LTE setup on our system was a bit interesting.
At some point, I've found that the modem needs a backport from the
upstream kernel.
For the 4.4 kernel, this is needed for some QMI modems:
https://github.com/torvalds/linux/commit/93725149794d3d418cf1eddcae60c7b536c5faa1

Otherwise, the modem does not init. That patch should be in 4.5 though.


>
>
>> > Signed-off-by: Alexandru Ardelean <ardeleana...@gmail.com>
>> > ---
>> >  scripts/netifd-proto.sh | 2 ++
>> >  1 file changed, 2 insertions(+)
>> >
>> > diff --git a/scripts/netifd-proto.sh b/scripts/netifd-proto.sh
>> > index cc7031a..fd7b596 100644
>> > --- a/scripts/netifd-proto.sh
>> > +++ b/scripts/netifd-proto.sh
>> > @@ -26,6 +26,8 @@ proto_add_dynamic_defaults() {
>> > [ -n "$defaultroute" ] && json_add_boolean defaultroute "$defaultroute"
>> > [ -n "$peerdns" ] && json_add_boolean peerdns "$peerdns"
>> > [ -n "$metric" ] && json_add_int metric "$metric"
>> > +   [ -n "$ip4table" ] && json_add_string ip4table "$ip4table"
>> > +   [ -n "$ip6table" ] && json_add_string ip6table "$ip6table"
>> >  }
>> >
>> >  _proto_do_teardown() {
>
>

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

Re: [LEDE-DEV] [PATCH] netifd-proto.sh: add ip4table & ip6table to `proto_add_dynamic_defaults()`

[Alexandru Ardelean]

On Tue, Sep 5, 2017 at 11:05 PM, Hans Dedecker <dedec...@gmail.com> wrote:
> On Tue, Sep 5, 2017 at 1:39 PM, Alexandru Ardelean
> <ardeleana...@gmail.com> wrote:
>> The `proto_add_dynamic_defaults()` seems to be called mostly
>> in the context of LTE/3G modems (via wwan, qmi, etc) setup.
>>
>> When they get setup, these devices override default routes.
>>
>> However, depending on setup, we want these modems to
>> be part of a another routing table.
>> This change allows that.
>>
>> ip4table/ip6table are of string type in netifd to allow
>> for `default`, `local` routing table names to be specified.
>>
>> Signed-off-by: Alexandru Ardelean <ardeleana...@gmail.com>
>> ---
>>  scripts/netifd-proto.sh | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/scripts/netifd-proto.sh b/scripts/netifd-proto.sh
>> index cc7031a..fd7b596 100644
>> --- a/scripts/netifd-proto.sh
>> +++ b/scripts/netifd-proto.sh
>> @@ -26,6 +26,8 @@ proto_add_dynamic_defaults() {
>> [ -n "$defaultroute" ] && json_add_boolean defaultroute 
>> "$defaultroute"
>> [ -n "$peerdns" ] && json_add_boolean peerdns "$peerdns"
>> [ -n "$metric" ] && json_add_int metric "$metric"
>> +   [ -n "$ip4table" ] && json_add_string ip4table "$ip4table"
>> +   [ -n "$ip6table" ] && json_add_string ip6table "$ip6table"
> I doubt this is a correct approach as the defaults defaultroute,
> peerdns and metric are common defaults for all IPv4/IPv6 interfaces
> while ip4table is only relevant for IPv4 interface while ip6table only
> for IPv6 interfaces.
> Looking into directip.sh and the usage of proto_add_dynamic_defaults
> this change would add both ip4table but also ip6table (if defined) to
> the dynamic created DHCP interface.
> A better approach would be to add ip4table and ip6table individually
> similar as is done for the extendprefix option (which is also only
> relevant for the DHCPv6 interfaces)
>
> Hans

Thanks for the quick reply.

Will check on your proposed alternative.

Thanks
Alex

>>  }
>>
>>  _proto_do_teardown() {
>> --
>> 2.11.0
>>

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

[LEDE-DEV] [PATCH] netifd-proto.sh: add ip4table & ip6table to `proto_add_dynamic_defaults()`

[Alexandru Ardelean]

The `proto_add_dynamic_defaults()` seems to be called mostly
in the context of LTE/3G modems (via wwan, qmi, etc) setup.

When they get setup, these devices override default routes.

However, depending on setup, we want these modems to
be part of a another routing table.
This change allows that.

ip4table/ip6table are of string type in netifd to allow
for `default`, `local` routing table names to be specified.

Signed-off-by: Alexandru Ardelean <ardeleana...@gmail.com>
---
 scripts/netifd-proto.sh | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/scripts/netifd-proto.sh b/scripts/netifd-proto.sh
index cc7031a..fd7b596 100644
--- a/scripts/netifd-proto.sh
+++ b/scripts/netifd-proto.sh
@@ -26,6 +26,8 @@ proto_add_dynamic_defaults() {
[ -n "$defaultroute" ] && json_add_boolean defaultroute "$defaultroute"
[ -n "$peerdns" ] && json_add_boolean peerdns "$peerdns"
[ -n "$metric" ] && json_add_int metric "$metric"
+   [ -n "$ip4table" ] && json_add_string ip4table "$ip4table"
+   [ -n "$ip6table" ] && json_add_string ip6table "$ip6table"
 }
 
 _proto_do_teardown() {
-- 
2.11.0


___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

[LEDE-DEV] [RFC] kernel-packages: new package (adapted from `kernel` package from core)

[Alexandru Ardelean]

Hey,

A while back I opened a PR:
https://github.com/openwrt/packages/pull/4407

Etienne prompted me to try the ML too.

The gist of it is:
g1) LEDE core does not want to package all/certain in-tree kmods
(bloat reasons, which is reasonable)
g2) The `packages` feed would like to have certain in-tree kmods for
certain packages

There are 2 ways to handle this [minor conflict]:
w1) each package in the feed packages it's own in-tree kmod
w2) spin a copy of the core's `kernel` meta-package, and package kmods there

I have no idea about the implications/side-effects [on bloat or
whatever] of  w2).
Hence the RFC tag.
The only benefits I see:
a) it's a centralized package, where in-tree kmods [from core's
kernel] would reside [which sounds nice]
b) re-building the kmod would not require re-building the package
[hopefully i did not misunderstand build dependency logic here]
c) if another package would depend only on the kmod, it would need not
re-build the entire package [ that defines it in the package feed ]

I'm fine with either w1) or w2).

So, far, it's only OVS that touches on this.

The reason I prompted to spin up this RFC is seeing this [with
ipvsadm/Load Balancer thingi] :
i) https://github.com/lede-project/source/pull/936
ii) 
https://github.com/lede-project/source/pull/936/commits/05f3f8576291e6f0d59069d0f0a75aa0363db4bc

For the kmods, I can agree the addition is big, but maybe it could
make sense to have this package in such a `kernel-packages` package.

Thanks
Alex

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

Re: [LEDE-DEV] Help me understand ubus extensions via C api

[Alexandru Ardelean]

On Thu, Jul 6, 2017 at 4:31 AM, Carlito Nueno  wrote:
> Hi all,
>
> I want to customize a small feature in how hostapd responds to probe
> request and I was looking at
> https://github.com/lede-project/source/blob/master/package/network/services/hostapd/src/src/ap/ubus.c
>
> Please help me understand how it works? for example when I call `ubus
> call hostapd.wlan0-1 del_client "{}" what happens?

the `ubus` cli will talk to ubusd
ubusd is the broker [or hub] for all things ubus between all processes
[that register with ubus[d] ]

hostapd has registered the `hostapd.wlan0-1` ubus object here
https://github.com/lede-project/source/blob/master/package/network/services/hostapd/src/src/ap/ubus.c#L457
via the ubus_add_object()

all registration to ubusd is done via libubus, which abstracts a lot
of the stuff to connect/talk to ubusd
all that you need to use [in a daemon/process with ubus] is in
libubus.h ; that header is exported at build time for other modules to
use
http://git.openwrt.org/?p=project/ubus.git;a=blob;f=libubus.h;h=4e45cb620a28befe23c27d3b8dc7eb1a1d12fb1a;hb=HEAD

it's important that all operations to ubus[d] be done on a
ubus_context object, which is a wrapper struct for a socket FD, some
buffer data, and other stuff

[ well, this was mostly ubus internals [in case it interests you ] ]

>
> I know that ubus_cli
> (http://git.openwrt.org/?p=project/ubus.git;a=blob;f=cli.c;h=19ccbb5093ce4c326010a9b2f504d7cd50798275;hb=HEAD)
> parses the input but how does ubus call the functions inside the above
> file (the ubus.c, ubus hostapd extension)?

the ubus cli tool uses libubus
but the code for cli, may not be a sufficient example to do other
operations [like in hostapd ]

>
> I see that there is this function, but how is that called
>
> static int hostapd_bss_del_client(struct ubus_context *ctx, struct
> ubus_object *obj, struct ubus_request_data *req, const char *method,
> struct blob_attr *msg);
>

Coming back to your "I want to customize a small feature in how hostapd"
I think that all you might need to do is register a new function here:
https://github.com/lede-project/source/blob/master/package/network/services/hostapd/src/src/ap/ubus.c#L419
or extend an existing one.

As I mentioned, the ubus cli talks to ubusd.
When doing `ubus call hostapd.wlan0-1 del_client "{}"` , this
means that hostapd has already registered a ` hostapd.wlan0-1` with
ubusd
When you call "del_client", the hostapd_bss_del_client() callback will
be called [ubus cli will ask ubusd to access del_client from
hostapd.wlan0-1 ] , but before that, the `del_policy` object will be
used to validate the message format.
[ You may have noticed that some callbacks are UBUS_METHOD_NOARG() and
some are UBUS_METHOD() ; the ones without a policy arg will get
processed without any msg format validation ]

>
> Thank you!
>

Maybe this information was not too well structured.
But hope it helps.

> ___
> Lede-dev mailing list
> Lede-dev@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/lede-dev

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

[LEDE-DEV] [PATCH 3/3] ubusd_monitor: alloc & free the buffer outside of the loop

[Alexandru Ardelean]

Should save a few cycles, since the data that's
being changed is only the seq number.
And the `ub` is always created as shared.

Signed-off-by: Alexandru Ardelean <ardeleana...@gmail.com>
---
 ubusd_monitor.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/ubusd_monitor.c b/ubusd_monitor.c
index a192206..fcbc6a4 100644
--- a/ubusd_monitor.c
+++ b/ubusd_monitor.c
@@ -72,13 +72,15 @@ ubusd_monitor_message(struct ubus_client *cl, struct 
ubus_msg_buf *ub, bool send
blob_put_int8(, UBUS_MONITOR_SEND, send);
blob_put(, UBUS_MONITOR_DATA, blob_data(ub->data), 
blob_len(ub->data));
 
+   ub = ubus_msg_new(mb.head, blob_raw_len(mb.head), true);
+   ub->hdr.type = UBUS_MSG_MONITOR;
+
list_for_each_entry(m, , list) {
-   ub = ubus_msg_new(mb.head, blob_raw_len(mb.head), true);
-   ub->hdr.type = UBUS_MSG_MONITOR;
ub->hdr.seq = ++m->seq;
ubus_msg_send(m->cl, ub);
-   ubus_msg_free(ub);
}
+
+   ubus_msg_free(ub);
 }
 
 static int
-- 
2.7.4


___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

[LEDE-DEV] [PATCH 1/3] ubusd: don't free messages in ubus_send_msg() anymore

[Alexandru Ardelean]

This makes it clear that `ubus_msg_send()` is only
about sending and queue-ing messages, and has nothing
to do with free-ing.

It can be a bit misleading/confusing when trying to go
through the code and make assumptions about whether a
buffer is free'd in ubus_send_msg(), or is free'd outside.

In `ubusd_proto_receive_message()` the `ubus_msg_free()`
is now called before the `if (ret == -1)` check.
That way, all callbacks will have their messages free'd,
which is what's desired, but confusing, because:
* ubusd_handle_invoke() called ubus_msg_free() before returning -1
* ubusd_handle_notify() called ubus_msg_free() before returning -1
* ubusd_handle_response() called ubus_msg_send(,,free=true) before returning -1
* ubus_msg_send() would call ubus_msg_send(,,free=false)
* all other callback callers would `ubus_msg_send(,,free=true)`
  (free the buffers in ubus_msg_send() )

In all other places, where `ubus_msg_send(,,free=true)`
an explicit `ubus_msg_free()` was added.

Signed-off-by: Alexandru Ardelean <ardeleana...@gmail.com>
---
 ubusd.c |  8 ++--
 ubusd.h |  2 +-
 ubusd_event.c   |  2 +-
 ubusd_monitor.c |  3 ++-
 ubusd_proto.c   | 29 +++--
 5 files changed, 21 insertions(+), 23 deletions(-)

diff --git a/ubusd.c b/ubusd.c
index f060b38..ba1ff07 100644
--- a/ubusd.c
+++ b/ubusd.c
@@ -146,7 +146,7 @@ static void ubus_msg_enqueue(struct ubus_client *cl, struct 
ubus_msg_buf *ub)
 }
 
 /* takes the msgbuf reference */
-void ubus_msg_send(struct ubus_client *cl, struct ubus_msg_buf *ub, bool free)
+void ubus_msg_send(struct ubus_client *cl, struct ubus_msg_buf *ub)
 {
int written;
 
@@ -160,7 +160,7 @@ void ubus_msg_send(struct ubus_client *cl, struct 
ubus_msg_buf *ub, bool free)
written = 0;
 
if (written >= ub->len + sizeof(ub->hdr))
-   goto out;
+   return;
 
cl->txq_ofs = written;
 
@@ -168,10 +168,6 @@ void ubus_msg_send(struct ubus_client *cl, struct 
ubus_msg_buf *ub, bool free)
uloop_fd_add(>sock, ULOOP_READ | ULOOP_WRITE | 
ULOOP_EDGE_TRIGGER);
}
ubus_msg_enqueue(cl, ub);
-
-out:
-   if (free)
-   ubus_msg_free(ub);
 }
 
 static struct ubus_msg_buf *ubus_msg_head(struct ubus_client *cl)
diff --git a/ubusd.h b/ubusd.h
index 5031ed4..991a59f 100644
--- a/ubusd.h
+++ b/ubusd.h
@@ -67,7 +67,7 @@ struct ubus_path {
 extern const char *ubusd_acl_dir;
 
 struct ubus_msg_buf *ubus_msg_new(void *data, int len, bool shared);
-void ubus_msg_send(struct ubus_client *cl, struct ubus_msg_buf *ub, bool free);
+void ubus_msg_send(struct ubus_client *cl, struct ubus_msg_buf *ub);
 void ubus_msg_free(struct ubus_msg_buf *ub);
 struct blob_attr **ubus_parse_msg(struct blob_attr *msg);
 
diff --git a/ubusd_event.c b/ubusd_event.c
index 984f341..0469c20 100644
--- a/ubusd_event.c
+++ b/ubusd_event.c
@@ -129,7 +129,7 @@ static void ubusd_send_event_msg(struct ubus_msg_buf **ub, 
struct ubus_client *c
*objid_ptr = htonl(obj->id.id);
 
(*ub)->hdr.seq = ++event_seq;
-   ubus_msg_send(obj->client, *ub, false);
+   ubus_msg_send(obj->client, *ub);
 }
 
 static bool strmatch_len(const char *s1, const char *s2, int *len)
diff --git a/ubusd_monitor.c b/ubusd_monitor.c
index 82d0333..a192206 100644
--- a/ubusd_monitor.c
+++ b/ubusd_monitor.c
@@ -76,7 +76,8 @@ ubusd_monitor_message(struct ubus_client *cl, struct 
ubus_msg_buf *ub, bool send
ub = ubus_msg_new(mb.head, blob_raw_len(mb.head), true);
ub->hdr.type = UBUS_MSG_MONITOR;
ub->hdr.seq = ++m->seq;
-   ubus_msg_send(m->cl, ub, true);
+   ubus_msg_send(m->cl, ub);
+   ubus_msg_free(ub);
}
 }
 
diff --git a/ubusd_proto.c b/ubusd_proto.c
index 72da7a7..441d084 100644
--- a/ubusd_proto.c
+++ b/ubusd_proto.c
@@ -89,7 +89,8 @@ ubus_proto_send_msg_from_blob(struct ubus_client *cl, struct 
ubus_msg_buf *ub,
ub->hdr.type = type;
ub->fd = fd;
 
-   ubus_msg_send(cl, ub, true);
+   ubus_msg_send(cl, ub);
+   ubus_msg_free(ub);
 }
 
 static bool ubusd_send_hello(struct ubus_client *cl)
@@ -102,14 +103,15 @@ static bool ubusd_send_hello(struct ubus_client *cl)
return false;
 
ubus_msg_init(ub, UBUS_MSG_HELLO, 0, cl->id.id);
-   ubus_msg_send(cl, ub, true);
+   ubus_msg_send(cl, ub);
+   ubus_msg_free(ub);
return true;
 }
 
 static int ubusd_send_pong(struct ubus_client *cl, struct ubus_msg_buf *ub, 
struct blob_attr **attr)
 {
ub->hdr.type = UBUS_MSG_DATA;
-   ubus_msg_send(cl, ub, false);
+   ubus_msg_send(cl, ub);
return 0;
 }
 
@@ -274,7 +276,6 @@ static int ubusd_handle_invoke(struct ubus_client *cl, 
struct ubus_msg_buf *ub,
blob_buf_init(, 0);
 
ubusd_forward_invoke(cl, obj, method, ub, attr[UBUS

[LEDE-DEV] [PATCH 2/3] ubusd: rename goto label from `error` to `out`

[Alexandru Ardelean]

Semantic has changed a bit.

Signed-off-by: Alexandru Ardelean <ardeleana...@gmail.com>
---
 ubusd_proto.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/ubusd_proto.c b/ubusd_proto.c
index 441d084..b084b86 100644
--- a/ubusd_proto.c
+++ b/ubusd_proto.c
@@ -345,22 +345,22 @@ static int ubusd_handle_response(struct ubus_client *cl, 
struct ubus_msg_buf *ub
if (!attr[UBUS_ATTR_OBJID] ||
(ub->hdr.type == UBUS_MSG_STATUS && !attr[UBUS_ATTR_STATUS]) ||
(ub->hdr.type == UBUS_MSG_DATA && !attr[UBUS_ATTR_DATA]))
-   goto error;
+   goto out;
 
obj = ubusd_find_object(blob_get_u32(attr[UBUS_ATTR_OBJID]));
if (!obj)
-   goto error;
+   goto out;
 
if (cl != obj->client)
-   goto error;
+   goto out;
 
cl = ubusd_get_client_by_id(ub->hdr.peer);
if (!cl)
-   goto error;
+   goto out;
 
ub->hdr.peer = blob_get_u32(attr[UBUS_ATTR_OBJID]);
ubus_msg_send(cl, ub);
-error:
+out:
return -1;
 }
 
-- 
2.7.4


___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

Re: [LEDE-DEV] [PATCH][ubus] ubusd_monitor: alloc & free the buffer outside of the loop

[Alexandru Ardelean]

On Wed, Jul 5, 2017 at 4:12 PM, Alexandru Ardelean
<ardeleana...@gmail.com> wrote:
> From: Alexandru Ardelean <ardeleana...@gmail.com>
>
> Should save a few cycles, since the data that's
> being changed is only the seq number.
> And the `ub` is always created as shared.
>
> Signed-off-by: Alexandru Ardelean <ardeleana...@gmail.com>
> ---
>  ubusd_monitor.c | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/ubusd_monitor.c b/ubusd_monitor.c
> index a192206..fcbc6a4 100644
> --- a/ubusd_monitor.c
> +++ b/ubusd_monitor.c
> @@ -72,13 +72,15 @@ ubusd_monitor_message(struct ubus_client *cl, struct 
> ubus_msg_buf *ub, bool send
> blob_put_int8(, UBUS_MONITOR_SEND, send);
> blob_put(, UBUS_MONITOR_DATA, blob_data(ub->data), 
> blob_len(ub->data));
>
> +   ub = ubus_msg_new(mb.head, blob_raw_len(mb.head), true);
> +   ub->hdr.type = UBUS_MSG_MONITOR;
> +
> list_for_each_entry(m, , list) {
> -   ub = ubus_msg_new(mb.head, blob_raw_len(mb.head), true);
> -   ub->hdr.type = UBUS_MSG_MONITOR;
> ub->hdr.seq = ++m->seq;
> ubus_msg_send(m->cl, ub);
> -   ubus_msg_free(ub);

Urgs...
Disregard this patch.
It's based on a unsubmitted patch from my local tree.

> }
> +
> +   ubus_msg_free(ub);
>  }
>
>  static int
> --
> 2.7.4
>

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

[LEDE-DEV] [PATCH][ubus] ubusd_monitor: alloc & free the buffer outside of the loop

[Alexandru Ardelean]

From: Alexandru Ardelean <ardeleana...@gmail.com>

Should save a few cycles, since the data that's
being changed is only the seq number.
And the `ub` is always created as shared.

Signed-off-by: Alexandru Ardelean <ardeleana...@gmail.com>
---
 ubusd_monitor.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/ubusd_monitor.c b/ubusd_monitor.c
index a192206..fcbc6a4 100644
--- a/ubusd_monitor.c
+++ b/ubusd_monitor.c
@@ -72,13 +72,15 @@ ubusd_monitor_message(struct ubus_client *cl, struct 
ubus_msg_buf *ub, bool send
blob_put_int8(, UBUS_MONITOR_SEND, send);
blob_put(, UBUS_MONITOR_DATA, blob_data(ub->data), 
blob_len(ub->data));
 
+   ub = ubus_msg_new(mb.head, blob_raw_len(mb.head), true);
+   ub->hdr.type = UBUS_MSG_MONITOR;
+
list_for_each_entry(m, , list) {
-   ub = ubus_msg_new(mb.head, blob_raw_len(mb.head), true);
-   ub->hdr.type = UBUS_MSG_MONITOR;
ub->hdr.seq = ++m->seq;
ubus_msg_send(m->cl, ub);
-   ubus_msg_free(ub);
}
+
+   ubus_msg_free(ub);
 }
 
 static int
-- 
2.7.4


___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

Re: [LEDE-DEV] [PATCH 2/3] ubusd: make `tx_queue` backlog dynamic

[Alexandru Ardelean]

On Wed, Jun 7, 2017 at 5:02 PM, Alexandru Ardelean
<ardeleana...@gmail.com> wrote:
> On Wed, Jun 7, 2017 at 4:48 PM, Felix Fietkau <n...@nbd.name> wrote:
>> On 2017-06-07 15:44, Felix Fietkau wrote:
>>> On 2017-06-07 13:09, Alexandru Ardelean wrote:
>>>> It's not very often that the tx_queue is used,
>>>> to store backlog messages to send to a client.
>>>>
>>>> And for most cases, 32 backlog messages seems to be enough.
>>>> In fact, for most cases, I've seen ~1 entry in the queue
>>>> being used every now-n-then.
>>>>
>>>> The issue is more visible/present with the `ubus list` command.
>>>> I've traced the code to ubusd_handle_lookup() in:
>>>>
>>>> ```
>>>> if (!attr[UBUS_ATTR_OBJPATH]) {
>>>> avl_for_each_element(, obj, path)
>>>> ubusd_send_obj(cl, ub, obj);
>>>> return 0;
>>>> }
>>>> ```
>>>> The code-path eventually leads to `ubus_msg_send()`.
>>>> It seems that once the first element is queued, then
>>>> the condition check for `(!cl->tx_queue[cl->txq_cur])`
>>>> will queue all messages iterated in the above snippet,
>>>> without trying any writes.
>>>>
>>>> This can be solved, either by making the queue dynamic
>>>> and allow it to expand above the current fixed limit (1).
>>>>
>>>> Or, by forcing/allowing writes during the tx_queue-ing (2).
>>>>
>>>> This patch implements (1).
>>>>
>>>> Signed-off-by: Alexandru Ardelean <ardeleana...@gmail.com>
>>> If I remember correctly, I chose the backlog array because a message can
>>> be referenced multiple times (e.g. in the notify/subscribe case).
>>> This would break in your linked-list implementation if a message needs
>>> to be queued for multiple clients.
>> I just re-checked and it seems that multiple references are no longer
>> used, so this implementation would probably work. I will take a closer look.
>
> I also need to take a closer look.
> The ubus_msg_ref() incrememts the refcount for non-shared buffers.
> So, there may be a loose end I may have missed somewhere.
>
> To be honest, I'm getting lost a bit within the code sometimes, as it
> seems to have been one single file, and gradually split.
> And I keep having to fight some wrong assumptions.
>
> Regarding this dynamic queue.
> One idea I was having is to make the array expand [via realloc] up to
> a limit [let's say 64k] if needed.
> I doubt anyone would use 64k netifd interfaces in a near future.
> ~200 sounds almost around the corner of getting there.
> Would this be an alternative ?
>
> Another proposal I would have, is to re-organize the code of ubus
> [ubusd mostly] in a series of gradual commits.
> The aim would be to reduce shared stuff between files, to make the
> code easier to follow.
> [ The big thing I am looking at, is the shared `struct blob_buf b;` in
> ubusd_proto.c ; that one really makes stuff hard to follow ]
> That would maybe make ubusd take up a bit more memory [ don't think
> it's too much ], because some more stuff would be alloc-ed per
> client/buffer.
> I would be glad to do it, if that's fine with you.

So, let's leave this for later.
I'll start on the re-organization.
I'd prefer we not waste energy on this, since this change is still a
bit vague, and not very obvious.

Thanks
Alex

>
> Alex
>
>>
>> - Felix

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

Re: [LEDE-DEV] [PATCH 2/3] ubusd: make `tx_queue` backlog dynamic

[Alexandru Ardelean]

On Wed, Jun 7, 2017 at 4:48 PM, Felix Fietkau <n...@nbd.name> wrote:
> On 2017-06-07 15:44, Felix Fietkau wrote:
>> On 2017-06-07 13:09, Alexandru Ardelean wrote:
>>> It's not very often that the tx_queue is used,
>>> to store backlog messages to send to a client.
>>>
>>> And for most cases, 32 backlog messages seems to be enough.
>>> In fact, for most cases, I've seen ~1 entry in the queue
>>> being used every now-n-then.
>>>
>>> The issue is more visible/present with the `ubus list` command.
>>> I've traced the code to ubusd_handle_lookup() in:
>>>
>>> ```
>>> if (!attr[UBUS_ATTR_OBJPATH]) {
>>> avl_for_each_element(, obj, path)
>>> ubusd_send_obj(cl, ub, obj);
>>> return 0;
>>> }
>>> ```
>>> The code-path eventually leads to `ubus_msg_send()`.
>>> It seems that once the first element is queued, then
>>> the condition check for `(!cl->tx_queue[cl->txq_cur])`
>>> will queue all messages iterated in the above snippet,
>>> without trying any writes.
>>>
>>> This can be solved, either by making the queue dynamic
>>> and allow it to expand above the current fixed limit (1).
>>>
>>> Or, by forcing/allowing writes during the tx_queue-ing (2).
>>>
>>> This patch implements (1).
>>>
>>> Signed-off-by: Alexandru Ardelean <ardeleana...@gmail.com>
>> If I remember correctly, I chose the backlog array because a message can
>> be referenced multiple times (e.g. in the notify/subscribe case).
>> This would break in your linked-list implementation if a message needs
>> to be queued for multiple clients.
> I just re-checked and it seems that multiple references are no longer
> used, so this implementation would probably work. I will take a closer look.

I also need to take a closer look.
The ubus_msg_ref() incrememts the refcount for non-shared buffers.
So, there may be a loose end I may have missed somewhere.

To be honest, I'm getting lost a bit within the code sometimes, as it
seems to have been one single file, and gradually split.
And I keep having to fight some wrong assumptions.

Regarding this dynamic queue.
One idea I was having is to make the array expand [via realloc] up to
a limit [let's say 64k] if needed.
I doubt anyone would use 64k netifd interfaces in a near future.
~200 sounds almost around the corner of getting there.
Would this be an alternative ?

Another proposal I would have, is to re-organize the code of ubus
[ubusd mostly] in a series of gradual commits.
The aim would be to reduce shared stuff between files, to make the
code easier to follow.
[ The big thing I am looking at, is the shared `struct blob_buf b;` in
ubusd_proto.c ; that one really makes stuff hard to follow ]
That would maybe make ubusd take up a bit more memory [ don't think
it's too much ], because some more stuff would be alloc-ed per
client/buffer.
I would be glad to do it, if that's fine with you.

Alex

>
> - Felix

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

[LEDE-DEV] [PATCH 1/3] examples (test): add example/test that shows tx-queue-ing issue

[Alexandru Ardelean]

The context where we've seen this issue, is in
our internal tests (which are mostly shell scripts).

When we try to setup 200 interfaces with netifd,
there are about 200+ ubus entries (network.interface.* ).

When we use `ubus list` to iterate through those
entries, we've noticed the `ubus list` hangs indefinitely.
Naturally, the test times out.

After a few wrong assumptions, we've identified 4 issues:
* One addressed here 97ac89f
* One addressed here c09e4f0
* One to be addressed: tx_queue backlog getting filled up
  and messages getting silently discarded
* One to be addressed: the `retmsg` blob buf seems to be
  shared amongst clients, and there's a rare race where
  the `retmsg` seq & peer are inconsistent for a certain client.

The overall problem seems to be a case of
"fast producer, slow consumer".

If the `ubus list` output (from this script/test) is
piped to a file, the problem doesn't show up.
But if it's displayed on the serial console (which is
a slow consumer), the issue is easily reproduce-able
with this lua script.

In our tests (shell scripts) we don't display the `ubus list`
output to the serial console ; maybe the netifd data is
big enough that the tx_queue-ing issue occurs (also due
to socket buffer size).

Signed-off-by: Alexandru Ardelean <ardeleana...@gmail.com>
---
 lua/test_many_ubus_entries.lua | 43 ++
 1 file changed, 43 insertions(+)
 create mode 100755 lua/test_many_ubus_entries.lua

diff --git a/lua/test_many_ubus_entries.lua b/lua/test_many_ubus_entries.lua
new file mode 100755
index 000..2324bf7
--- /dev/null
+++ b/lua/test_many_ubus_entries.lua
@@ -0,0 +1,43 @@
+#!/usr/bin/env lua
+
+require "ubus"
+require "uloop"
+
+uloop.init()
+
+local conn = ubus.connect()
+if not conn then
+   error("Failed to connect to ubus")
+end
+
+local my_method = {
+   test_0 = {
+   hello = {
+   function(req, msg)
+   conn:reply(req, {message="foo"});
+   print("Call to function 'hello'")
+   for k, v in pairs(msg) do
+   print("key=" .. k .. " value=" .. 
tostring(v))
+   end
+   end, {id = ubus.INT32, msg = ubus.STRING }
+   },
+   }
+}
+
+for i=1,2000 do
+   my_method["test_" .. tostring(i)] = my_method["test_0"]
+end
+
+conn:add(my_method)
+
+uloop.timer(
+   function()
+   -- running "ubus list" to make sure ubus contexts are isolated
+   os.execute("ubus list")
+   conn:close()
+   uloop.cancel()
+   end,
+   2000
+)
+
+uloop.run()
-- 
2.7.4


___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

[LEDE-DEV] [PATCH 2/3] ubusd: make `tx_queue` backlog dynamic

[Alexandru Ardelean]

It's not very often that the tx_queue is used,
to store backlog messages to send to a client.

And for most cases, 32 backlog messages seems to be enough.
In fact, for most cases, I've seen ~1 entry in the queue
being used every now-n-then.

The issue is more visible/present with the `ubus list` command.
I've traced the code to ubusd_handle_lookup() in:

```
if (!attr[UBUS_ATTR_OBJPATH]) {
avl_for_each_element(, obj, path)
ubusd_send_obj(cl, ub, obj);
return 0;
}
```
The code-path eventually leads to `ubus_msg_send()`.
It seems that once the first element is queued, then
the condition check for `(!cl->tx_queue[cl->txq_cur])`
will queue all messages iterated in the above snippet,
without trying any writes.

This can be solved, either by making the queue dynamic
and allow it to expand above the current fixed limit (1).

Or, by forcing/allowing writes during the tx_queue-ing (2).

This patch implements (1).

Signed-off-by: Alexandru Ardelean <ardeleana...@gmail.com>
---
 ubusd.c   | 18 +-
 ubusd.h   |  6 +++---
 ubusd_proto.c |  1 +
 3 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/ubusd.c b/ubusd.c
index f060b38..8fdb85b 100644
--- a/ubusd.c
+++ b/ubusd.c
@@ -59,6 +59,7 @@ struct ubus_msg_buf *ubus_msg_new(void *data, int len, bool 
shared)
return NULL;
 
ub->fd = -1;
+   INIT_LIST_HEAD(>list);
 
if (shared) {
ub->refcount = ~0;
@@ -138,11 +139,9 @@ static int ubus_msg_writev(int fd, struct ubus_msg_buf 
*ub, int offset)
 
 static void ubus_msg_enqueue(struct ubus_client *cl, struct ubus_msg_buf *ub)
 {
-   if (cl->tx_queue[cl->txq_tail])
-   return;
-
-   cl->tx_queue[cl->txq_tail] = ubus_msg_ref(ub);
-   cl->txq_tail = (cl->txq_tail + 1) % ARRAY_SIZE(cl->tx_queue);
+   ub = ubus_msg_ref(ub);
+   if (ub)
+   list_add_tail(>list, >tx_queue);
 }
 
 /* takes the msgbuf reference */
@@ -153,7 +152,7 @@ void ubus_msg_send(struct ubus_client *cl, struct 
ubus_msg_buf *ub, bool free)
if (ub->hdr.type != UBUS_MSG_MONITOR)
ubusd_monitor_message(cl, ub, true);
 
-   if (!cl->tx_queue[cl->txq_cur]) {
+   if (list_empty(>tx_queue)) {
written = ubus_msg_writev(cl->sock.fd, ub, 0);
 
if (written < 0)
@@ -176,7 +175,9 @@ out:
 
 static struct ubus_msg_buf *ubus_msg_head(struct ubus_client *cl)
 {
-   return cl->tx_queue[cl->txq_cur];
+   if (list_empty(>tx_queue))
+   return NULL;
+   return list_first_entry(>tx_queue, struct ubus_msg_buf, list);
 }
 
 static void ubus_msg_dequeue(struct ubus_client *cl)
@@ -186,10 +187,9 @@ static void ubus_msg_dequeue(struct ubus_client *cl)
if (!ub)
return;
 
+   list_del(>list);
ubus_msg_free(ub);
cl->txq_ofs = 0;
-   cl->tx_queue[cl->txq_cur] = NULL;
-   cl->txq_cur = (cl->txq_cur + 1) % ARRAY_SIZE(cl->tx_queue);
 }
 
 static void handle_client_disconnect(struct ubus_client *cl)
diff --git a/ubusd.h b/ubusd.h
index 5031ed4..6748c65 100644
--- a/ubusd.h
+++ b/ubusd.h
@@ -23,12 +23,12 @@
 #include "ubusmsg.h"
 #include "ubusd_acl.h"
 
-#define UBUSD_CLIENT_BACKLOG   32
 #define UBUS_OBJ_HASH_BITS 4
 
 extern struct blob_buf b;
 
 struct ubus_msg_buf {
+   struct list_head list;
uint32_t refcount; /* ~0: uses external data buffer */
struct ubus_msghdr hdr;
struct blob_attr *data;
@@ -47,8 +47,8 @@ struct ubus_client {
 
struct list_head objects;
 
-   struct ubus_msg_buf *tx_queue[UBUSD_CLIENT_BACKLOG];
-   unsigned int txq_cur, txq_tail, txq_ofs;
+   unsigned int txq_ofs;
+   struct list_head tx_queue;
 
struct ubus_msg_buf *pending_msg;
int pending_msg_offset;
diff --git a/ubusd_proto.c b/ubusd_proto.c
index 72da7a7..631047b 100644
--- a/ubusd_proto.c
+++ b/ubusd_proto.c
@@ -480,6 +480,7 @@ struct ubus_client *ubusd_proto_new_client(int fd, 
uloop_fd_handler cb)
goto free;
 
INIT_LIST_HEAD(>objects);
+   INIT_LIST_HEAD(>tx_queue);
cl->sock.fd = fd;
cl->sock.cb = cb;
cl->pending_msg_fd = -1;
-- 
2.7.4


___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

[LEDE-DEV] [PATCH 3/3] ubusd: move global retmsg per client

[Alexandru Ardelean]

Even with the tx_queue-ing issue resolved, what
seems to happen afterwards, is that all the messages
seems to get through, but the client still loops
in the `ubus_complete_request()` waiting for
`req->status_msg` or for a timeout.

Though, the timeout does not seem to happen, because
the data is processed in `ubus_poll_data()`, with
a infinite poll() timeout (ubus_complete_request() is
called with timeout 0).

It's likely that either the `seq` or `peer` sent from
ubusd are wrong, and the client cannot get the correct
ubus request in `ubus_process_req_msg()`.
I haven't digged too deep into this ; setting the
`retmsg` object on the client struct seems to have
resolved any hanging with the `ubus list` command.

Signed-off-by: Alexandru Ardelean <ardeleana...@gmail.com>
---
 ubusd.h   |  2 ++
 ubusd_proto.c | 35 +++
 2 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/ubusd.h b/ubusd.h
index 6748c65..a9dd04e 100644
--- a/ubusd.h
+++ b/ubusd.h
@@ -39,6 +39,7 @@ struct ubus_msg_buf {
 struct ubus_client {
struct ubus_id id;
struct uloop_fd sock;
+   struct blob_buf b;
 
uid_t uid;
gid_t gid;
@@ -57,6 +58,7 @@ struct ubus_client {
struct ubus_msghdr hdr;
struct blob_attr data;
} hdrbuf;
+   struct ubus_msg_buf *retmsg;
 };
 
 struct ubus_path {
diff --git a/ubusd_proto.c b/ubusd_proto.c
index 631047b..d71f74e 100644
--- a/ubusd_proto.c
+++ b/ubusd_proto.c
@@ -17,8 +17,6 @@
 #include "ubusd.h"
 
 struct blob_buf b;
-static struct ubus_msg_buf *retmsg;
-static int *retmsg_data;
 static struct avl_tree clients;
 
 static struct blob_attr *attrbuf[UBUS_ATTR_MAX];
@@ -444,6 +442,8 @@ void ubusd_proto_receive_message(struct ubus_client *cl, 
struct ubus_msg_buf *ub
 {
ubus_cmd_cb cb = NULL;
int ret;
+   struct ubus_msg_buf *retmsg = cl->retmsg;
+   int *retmsg_data = blob_data(blob_data(retmsg->data));
 
retmsg->hdr.seq = ub->hdr.seq;
retmsg->hdr.peer = ub->hdr.peer;
@@ -468,6 +468,22 @@ void ubusd_proto_receive_message(struct ubus_client *cl, 
struct ubus_msg_buf *ub
ubus_msg_send(cl, retmsg, false);
 }
 
+static int ubusd_proto_init_retmsg(struct ubus_client *cl)
+{
+   struct blob_buf *b = >b;
+
+   blob_buf_init(>b, 0);
+   blob_put_int32(>b, UBUS_ATTR_STATUS, 0);
+
+   /* we make the 'retmsg' buffer shared with the blob_buf b, to reduce 
mem duplication */
+   cl->retmsg = ubus_msg_new(b->head, blob_raw_len(b->head), true);
+   if (!cl->retmsg)
+   return -1;
+
+   cl->retmsg->hdr.type = UBUS_MSG_STATUS;
+   return 0;
+}
+
 struct ubus_client *ubusd_proto_new_client(int fd, uloop_fd_handler cb)
 {
struct ubus_client *cl;
@@ -488,6 +504,9 @@ struct ubus_client *ubusd_proto_new_client(int fd, 
uloop_fd_handler cb)
if (!ubus_alloc_id(, >id, 0))
goto free;
 
+   if (ubusd_proto_init_retmsg(cl))
+   goto free;
+
if (!ubusd_send_hello(cl))
goto delete;
 
@@ -508,6 +527,8 @@ void ubusd_proto_free_client(struct ubus_client *cl)
obj = list_first_entry(>objects, struct ubus_object, list);
ubusd_free_object(obj);
}
+   ubus_msg_free(cl->retmsg);
+   blob_buf_free(>b);
 
ubusd_acl_free_client(cl);
ubus_free_id(, >id);
@@ -550,14 +571,4 @@ void ubus_notify_unsubscribe(struct ubus_subscription *s)
 static void __constructor ubusd_proto_init(void)
 {
ubus_init_id_tree();
-
-   blob_buf_init(, 0);
-   blob_put_int32(, UBUS_ATTR_STATUS, 0);
-
-   retmsg = ubus_msg_from_blob(false);
-   if (!retmsg)
-   exit(1);
-
-   retmsg->hdr.type = UBUS_MSG_STATUS;
-   retmsg_data = blob_data(blob_data(retmsg->data));
 }
-- 
2.7.4


___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

Re: [LEDE-DEV] [OpenWrt-Devel] openwrt and lede - remerge proposal V3

[Alexandru Ardelean]

On Tue, May 30, 2017 at 8:38 AM, Felix Fietkau  wrote:
> On 2017-05-29 09:03, John Crispin wrote:
>> (resend, this time as plain text)
>>
>> Hi,
>>
>> here is a V3 of the remerge proposal, I tried to fold all the comments
>> people made into it, if anything is missing let me know. Please remeber
>> that post remerge anything can be voted on, so cluttering the proposal
>> with many details will delay the remerge even more.
>>
>> Ideally we manage to vote during this week.
> ACK.
>
> - Felix

ack

[forgot that GMail app does not support plain text mode, or is really
hidden somewhere ]

> ___
> openwrt-devel mailing list
> openwrt-de...@lists.openwrt.org
> https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

Re: [LEDE-DEV] [PATCH][procd] ubus: use ubus_auto_connect() logic

[Alexandru Ardelean]

On Mon, May 29, 2017 at 10:19 AM, Felix Fietkau <n...@nbd.name> wrote:
> On 2017-05-19 09:45, Alexandru Ardelean wrote:
>> Admittedly, the semantic is a bit different, in the sense
>> that there are no progressive retry timeouts.
>>
>> ubus_auto_connect() uses 1 second fixed retry intervals.
>> Whereas the old logic would start at 50 millisecs and
>> progress up to 1 second.
>>
>> Other than that, the rest should be the same overall logic.
>> This should cleanup the code a bit.
>>
>> For reference, the `ubus_add_uloop()` call is handled
>> by ubus_auto_connect() as well.
>>
>> Signed-off-by: Alexandru Ardelean <ardeleana...@gmail.com>
> procd intentionally uses different semantics, since it's responsible for
> launching ubus in the first place.
>
> This patch causes extra delay and extra error messages, so I don't think
> we should do something like this at all.
>
> - Felix

Fine from me.
Will close.

Thanks :)
Alex

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

[LEDE-DEV] [PATCH][procd] ubus: use ubus_auto_connect() logic

[Alexandru Ardelean]

Admittedly, the semantic is a bit different, in the sense
that there are no progressive retry timeouts.

ubus_auto_connect() uses 1 second fixed retry intervals.
Whereas the old logic would start at 50 millisecs and
progress up to 1 second.

Other than that, the rest should be the same overall logic.
This should cleanup the code a bit.

For reference, the `ubus_add_uloop()` call is handled
by ubus_auto_connect() as well.

Signed-off-by: Alexandru Ardelean <ardeleana...@gmail.com>
---
 ubus.c | 55 +--
 1 file changed, 5 insertions(+), 50 deletions(-)

diff --git a/ubus.c b/ubus.c
index 8d521ac..5dd5452 100644
--- a/ubus.c
+++ b/ubus.c
@@ -20,68 +20,23 @@
 #include "procd.h"
 
 char *ubus_socket = NULL;
-static struct ubus_context *ctx;
-static struct uloop_timeout ubus_timer;
-static int timeout;
-
-static void reset_timeout(void)
-{
-   timeout = 50;
-}
-
-static void timeout_retry(void)
-{
-   uloop_timeout_set(_timer, timeout);
-   timeout *= 2;
-   if (timeout > 1000)
-   timeout = 1000;
-}
+static struct ubus_auto_conn conn;
 
 static void
-ubus_reconnect_cb(struct uloop_timeout *timeout)
+ubus_auto_connect_cb(struct ubus_context *ctx)
 {
-   if (!ubus_reconnect(ctx, ubus_socket)) {
-   ubus_add_uloop(ctx);
-   return;
-   }
-
-   timeout_retry();
-}
-
-static void
-ubus_disconnect_cb(struct ubus_context *ctx)
-{
-   ubus_timer.cb = ubus_reconnect_cb;
-   reset_timeout();
-   timeout_retry();
-}
-
-static void
-ubus_connect_cb(struct uloop_timeout *timeout)
-{
-   ctx = ubus_connect(ubus_socket);
-
-   if (!ctx) {
-   DEBUG(4, "Connection to ubus failed\n");
-   timeout_retry();
-   return;
-   }
-
-   ctx->connection_lost = ubus_disconnect_cb;
ubus_init_service(ctx);
ubus_init_system(ctx);
watch_ubus(ctx);
 
DEBUG(2, "Connected to ubus, id=%08x\n", ctx->local_id);
-   reset_timeout();
-   ubus_add_uloop(ctx);
procd_state_ubus_connect();
 }
 
 void
 procd_connect_ubus(void)
 {
-   ubus_timer.cb = ubus_connect_cb;
-   reset_timeout();
-   timeout_retry();
+   conn.path = ubus_socket;
+   conn.cb = ubus_auto_connect_cb;
+   ubus_auto_connect();
 }
-- 
2.7.4


___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

Re: [LEDE-DEV] convention on uid/gid for packages

[Alexandru Ardelean]

On Sun, May 14, 2017 at 3:59 AM, Daniel Golle  wrote:
> Hi Val,
>
> On Sat, May 13, 2017 at 06:23:29PM -0400, Val Kulkov wrote:
>> Is there any convention on the use of uid and gid when creating new
>> users or groups? Can someone point me to it, if it exists?
>>
>> I noticed that two packages, icecast and postfix, compete for the same 
>> uid=87:
>>
>> icecast's Makefile:
>>   USERID:=icecast=87:icecast=87
>>
>> postfix's postfix.init:
>>   user_exists postfix || user_add postfix 87
>
> This looks wrong to me (user_add in the init script)...
>
>>
>> There may be more packages competing for the same uid/gid's, I have
>> not fully researched it.
>>
>> I am preparing a new package, opendkim, which should be run as a
>> non-privileged user. For this,
>> USERID:=opendkim=:opendkim= seems appropriate,
>> but what numbers should I assign?
>
> I run into this issue before and believe that we should have a wiki
> page which allows registering static UIDs/GIDs at least for the
> packages which actually need that (ie. if a specific UID or GID is
> referenced in other packages, or scripts like firewall rules, ...).
> Grep'ing for USERID allows to automatically generate that list based
> on the currently available packages very easily.
>
> Examples from elsewhere for inspiration:
>
> FreeBSD got those lists
> https://svnweb.freebsd.org/ports/head/UIDs?view=markup
> https://svnweb.freebsd.org/ports/head/GIDs?view=markup
>
> linuxfromscratch got a much smaller list for essential/system UIDs/GIDs
> http://linuxfromscratch.org/blfs/view/svn/postlfs/users.html
>
>
> Cheers
>

Just woke up from the weekend.
I recommend trying this out [based on lldpd] :
https://github.com/lede-project/source/blob/master/package/network/services/lldpd/Makefile#L35
We use lldpd and this seems to work ; lldpd does some priv separation.

Alex

>
> Daniel
>
> ___
> Lede-dev mailing list
> Lede-dev@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/lede-dev

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

[LEDE-DEV] [PATCH][opkg-lede][V3] opkg: add --no-check-certificate argument

[Alexandru Ardelean]

For cases when artifacts are stored on https:// accessible
locations and you don't want to install ca-certificates
(for various reasons).

I'll admit, using SSL like this is not recommended,
but since wget (even uclient-fetch) allows the
--no-check-certificate option, it would be nice
for opkg to support setting it if needed/configured.

Signed-off-by: Alexandru Ardelean <ardeleana...@gmail.com>
---
 libopkg/opkg_conf.c | 1 +
 libopkg/opkg_conf.h | 1 +
 libopkg/opkg_download.c | 5 -
 src/opkg-cl.c   | 7 +++
 4 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/libopkg/opkg_conf.c b/libopkg/opkg_conf.c
index 589fc49..bab8f57 100644
--- a/libopkg/opkg_conf.c
+++ b/libopkg/opkg_conf.c
@@ -54,6 +54,7 @@ opkg_option_t options[] = {
{"force_postinstall", OPKG_OPT_TYPE_BOOL, &_conf.force_postinstall},
{"force_checksum", OPKG_OPT_TYPE_BOOL, &_conf.force_checksum},
{"check_signature", OPKG_OPT_TYPE_BOOL, &_conf.check_signature},
+   {"no_check_certificate", OPKG_OPT_TYPE_BOOL, 
&_conf.no_check_certificate},
{"ftp_proxy", OPKG_OPT_TYPE_STRING, &_conf.ftp_proxy},
{"http_proxy", OPKG_OPT_TYPE_STRING, &_conf.http_proxy},
{"no_proxy", OPKG_OPT_TYPE_STRING, &_conf.no_proxy},
diff --git a/libopkg/opkg_conf.h b/libopkg/opkg_conf.h
index 9cf7681..b63a1e6 100644
--- a/libopkg/opkg_conf.h
+++ b/libopkg/opkg_conf.h
@@ -78,6 +78,7 @@ struct opkg_conf {
int force_checksum;
int check_signature;
int force_signature;
+   int no_check_certificate;
int nodeps; /* do not follow dependencies */
int nocase; /* perform case insensitive matching */
char *offline_root;
diff --git a/libopkg/opkg_download.c b/libopkg/opkg_download.c
index db4c90f..36db231 100644
--- a/libopkg/opkg_download.c
+++ b/libopkg/opkg_download.c
@@ -87,11 +87,14 @@ opkg_download(const char *src, const char *dest_file_name,
 
{
int res;
-   const char *argv[8];
+   const char *argv[9];
int i = 0;
 
argv[i++] = "wget";
argv[i++] = "-q";
+   if (conf->no_check_certificate) {
+   argv[i++] = "--no-check-certificate";
+   }
if (conf->http_proxy || conf->ftp_proxy) {
argv[i++] = "-Y";
argv[i++] = "on";
diff --git a/src/opkg-cl.c b/src/opkg-cl.c
index c518bfc..a3ea5c1 100644
--- a/src/opkg-cl.c
+++ b/src/opkg-cl.c
@@ -52,6 +52,7 @@ enum {
ARGS_OPT_AUTOREMOVE,
ARGS_OPT_CACHE,
ARGS_OPT_FORCE_SIGNATURE,
+   ARGS_OPT_NO_CHECK_CERTIFICATE,
ARGS_OPT_SIZE,
 };
 
@@ -91,6 +92,8 @@ static struct option long_options[] = {
{"force_checksum", 0, 0, ARGS_OPT_FORCE_CHECKSUM},
{"force-signature", 0, 0, ARGS_OPT_FORCE_SIGNATURE},
{"force_signature", 0, 0, ARGS_OPT_FORCE_SIGNATURE},
+   {"no-check-certificate", 0, 0, ARGS_OPT_NO_CHECK_CERTIFICATE},
+   {"no_check_certificate", 0, 0, ARGS_OPT_NO_CHECK_CERTIFICATE},
{"noaction", 0, 0, ARGS_OPT_NOACTION},
{"download-only", 0, 0, ARGS_OPT_DOWNLOAD_ONLY},
{"nodeps", 0, 0, ARGS_OPT_NODEPS},
@@ -226,6 +229,9 @@ static int args_parse(int argc, char *argv[])
case ARGS_OPT_FORCE_SIGNATURE:
conf->force_signature = 1;
break;
+   case ARGS_OPT_NO_CHECK_CERTIFICATE:
+   conf->no_check_certificate = 1;
+   break;
case ':':
parse_err = -1;
break;
@@ -335,6 +341,7 @@ static void usage()
printf
("\t--force-remove  Remove package even if prerm script fails\n");
printf("\t--force-checksum  Don't fail on checksum mismatches\n");
+   printf("\t--no-check-certificate Don't validate SSL certificates\n");
printf("\t--noactionNo action -- test only\n");
printf("\t--download-only   No action -- download only\n");
printf("\t--nodeps  Do not follow dependencies\n");
-- 
2.7.4


___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

Re: [LEDE-DEV] [PATCH] opkg: add --no-check-certificate argument

[Alexandru Ardelean]

On Thu, May 11, 2017 at 6:42 PM, Jo-Philipp Wich  wrote:
> Hi,
>
> comments inline.
>
>> ---
>>  libopkg/opkg_conf.c | 1 +
>>  libopkg/opkg_conf.h | 1 +
>>  libopkg/opkg_download.c | 5 -
>>  src/opkg-cl.c   | 6 ++
>>  4 files changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/libopkg/opkg_conf.c b/libopkg/opkg_conf.c
>> index 589fc49..bab8f57 100644
>> --- a/libopkg/opkg_conf.c
>> +++ b/libopkg/opkg_conf.c
>> @@ -54,6 +54,7 @@ opkg_option_t options[] = {
>>   {"force_postinstall", OPKG_OPT_TYPE_BOOL, &_conf.force_postinstall},
>>   {"force_checksum", OPKG_OPT_TYPE_BOOL, &_conf.force_checksum},
>>   {"check_signature", OPKG_OPT_TYPE_BOOL, &_conf.check_signature},
>> + {"no_check_certificate", OPKG_OPT_TYPE_BOOL, 
>> &_conf.no_check_certificate},
>>   {"ftp_proxy", OPKG_OPT_TYPE_STRING, &_conf.ftp_proxy},
>>   {"http_proxy", OPKG_OPT_TYPE_STRING, &_conf.http_proxy},
>>   {"no_proxy", OPKG_OPT_TYPE_STRING, &_conf.no_proxy},
>> diff --git a/libopkg/opkg_conf.h b/libopkg/opkg_conf.h
>> index 9cf7681..b63a1e6 100644
>> --- a/libopkg/opkg_conf.h
>> +++ b/libopkg/opkg_conf.h
>> @@ -78,6 +78,7 @@ struct opkg_conf {
>>   int force_checksum;
>>   int check_signature;
>>   int force_signature;
>> + int no_check_certificate;
>>   int nodeps; /* do not follow dependencies */
>>   int nocase; /* perform case insensitive matching */
>>   char *offline_root;
>> diff --git a/libopkg/opkg_download.c b/libopkg/opkg_download.c
>> index db4c90f..36db231 100644
>> --- a/libopkg/opkg_download.c
>> +++ b/libopkg/opkg_download.c
>> @@ -87,11 +87,14 @@ opkg_download(const char *src, const char 
>> *dest_file_name,
>>
>>   {
>>   int res;
>> - const char *argv[8];
>> + const char *argv[9];
>>   int i = 0;
>>
>>   argv[i++] = "wget";
>>   argv[i++] = "-q";
>> + if (conf->no_check_certificate) {
>> + argv[i++] = "--no-check-certificate";
>> + }
>>   if (conf->http_proxy || conf->ftp_proxy) {
>>   argv[i++] = "-Y";
>>   argv[i++] = "on";
>> diff --git a/src/opkg-cl.c b/src/opkg-cl.c
>> index c518bfc..0ffad86 100644
>> --- a/src/opkg-cl.c
>> +++ b/src/opkg-cl.c
>> @@ -52,6 +52,7 @@ enum {
>>   ARGS_OPT_AUTOREMOVE,
>>   ARGS_OPT_CACHE,
>>   ARGS_OPT_FORCE_SIGNATURE,
>> + ARGS_OPT_NO_CHECK_CERTIFICATE,
>>   ARGS_OPT_SIZE,
>>  };
>>
>> @@ -91,6 +92,8 @@ static struct option long_options[] = {
>>   {"force_checksum", 0, 0, ARGS_OPT_FORCE_CHECKSUM},
>>   {"force-signature", 0, 0, ARGS_OPT_FORCE_SIGNATURE},
>>   {"force_signature", 0, 0, ARGS_OPT_FORCE_SIGNATURE},
>> + {"no-check-certificate", 0, 0, ARGS_OPT_NO_CHECK_CERTIFICATE},
>> + {"no_check_certificate", 0, 0, ARGS_OPT_NO_CHECK_CERTIFICATE},
>>   {"noaction", 0, 0, ARGS_OPT_NOACTION},
>>   {"download-only", 0, 0, ARGS_OPT_DOWNLOAD_ONLY},
>>   {"nodeps", 0, 0, ARGS_OPT_NODEPS},
>> @@ -226,6 +229,8 @@ static int args_parse(int argc, char *argv[])
>>   case ARGS_OPT_FORCE_SIGNATURE:
>>   conf->force_signature = 1;
>>   break;
>> + case ARGS_OPT_NO_CHECK_CERTIFICATE:
>> + conf->no_check_certificate = 1;
>
> I think a break is missing in this case.
>
>>   case ':':
>>   parse_err = -1;
>>   break;
>> @@ -335,6 +340,7 @@ static void usage()
>>   printf
>>   ("\t--force-remove  Remove package even if prerm script fails\n");
>>   printf("\t--force-checksum  Don't fail on checksum mismatches\n");
>> + printf("\t--no-check-certificate Don't validate the server's 
>> certificate\n");
>
> In the help text I'd state something like "Do not validate SSL
> certificates."

For reference, the "Don't validate the server's certificate" message
here, is actually copy+pasted from wget's output.
But I'm fine to have it either form.

>
>>   printf("\t--noactionNo action -- test only\n");
>>   printf("\t--download-only   No action -- download only\n");
>>   printf("\t--nodeps  Do not follow dependencies\n");
>
>
> ~ Jo
>
>
> ___
> Lede-dev mailing list
> Lede-dev@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/lede-dev

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

[LEDE-DEV] [PATCH] opkg: add --no-check-certificate argument

[Alexandru Ardelean]

For cases when artifacts are stored on https:// accessible
location and you don't want to install ca-certificates
(for various reasons).

I'll admit, using SSL like this is not recommended,
but since wget (even uclient-fetch) allows the
--no-check-certificate option, it would be nice
for opkg to support setting it if needed/configured.

Signed-off-by: Alexandru Ardelean <ardeleana...@gmail.com>
---
 libopkg/opkg_conf.c | 1 +
 libopkg/opkg_conf.h | 1 +
 libopkg/opkg_download.c | 5 -
 src/opkg-cl.c   | 6 ++
 4 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/libopkg/opkg_conf.c b/libopkg/opkg_conf.c
index 589fc49..bab8f57 100644
--- a/libopkg/opkg_conf.c
+++ b/libopkg/opkg_conf.c
@@ -54,6 +54,7 @@ opkg_option_t options[] = {
{"force_postinstall", OPKG_OPT_TYPE_BOOL, &_conf.force_postinstall},
{"force_checksum", OPKG_OPT_TYPE_BOOL, &_conf.force_checksum},
{"check_signature", OPKG_OPT_TYPE_BOOL, &_conf.check_signature},
+   {"no_check_certificate", OPKG_OPT_TYPE_BOOL, 
&_conf.no_check_certificate},
{"ftp_proxy", OPKG_OPT_TYPE_STRING, &_conf.ftp_proxy},
{"http_proxy", OPKG_OPT_TYPE_STRING, &_conf.http_proxy},
{"no_proxy", OPKG_OPT_TYPE_STRING, &_conf.no_proxy},
diff --git a/libopkg/opkg_conf.h b/libopkg/opkg_conf.h
index 9cf7681..b63a1e6 100644
--- a/libopkg/opkg_conf.h
+++ b/libopkg/opkg_conf.h
@@ -78,6 +78,7 @@ struct opkg_conf {
int force_checksum;
int check_signature;
int force_signature;
+   int no_check_certificate;
int nodeps; /* do not follow dependencies */
int nocase; /* perform case insensitive matching */
char *offline_root;
diff --git a/libopkg/opkg_download.c b/libopkg/opkg_download.c
index db4c90f..36db231 100644
--- a/libopkg/opkg_download.c
+++ b/libopkg/opkg_download.c
@@ -87,11 +87,14 @@ opkg_download(const char *src, const char *dest_file_name,
 
{
int res;
-   const char *argv[8];
+   const char *argv[9];
int i = 0;
 
argv[i++] = "wget";
argv[i++] = "-q";
+   if (conf->no_check_certificate) {
+   argv[i++] = "--no-check-certificate";
+   }
if (conf->http_proxy || conf->ftp_proxy) {
argv[i++] = "-Y";
argv[i++] = "on";
diff --git a/src/opkg-cl.c b/src/opkg-cl.c
index c518bfc..0ffad86 100644
--- a/src/opkg-cl.c
+++ b/src/opkg-cl.c
@@ -52,6 +52,7 @@ enum {
ARGS_OPT_AUTOREMOVE,
ARGS_OPT_CACHE,
ARGS_OPT_FORCE_SIGNATURE,
+   ARGS_OPT_NO_CHECK_CERTIFICATE,
ARGS_OPT_SIZE,
 };
 
@@ -91,6 +92,8 @@ static struct option long_options[] = {
{"force_checksum", 0, 0, ARGS_OPT_FORCE_CHECKSUM},
{"force-signature", 0, 0, ARGS_OPT_FORCE_SIGNATURE},
{"force_signature", 0, 0, ARGS_OPT_FORCE_SIGNATURE},
+   {"no-check-certificate", 0, 0, ARGS_OPT_NO_CHECK_CERTIFICATE},
+   {"no_check_certificate", 0, 0, ARGS_OPT_NO_CHECK_CERTIFICATE},
{"noaction", 0, 0, ARGS_OPT_NOACTION},
{"download-only", 0, 0, ARGS_OPT_DOWNLOAD_ONLY},
{"nodeps", 0, 0, ARGS_OPT_NODEPS},
@@ -226,6 +229,8 @@ static int args_parse(int argc, char *argv[])
case ARGS_OPT_FORCE_SIGNATURE:
conf->force_signature = 1;
break;
+   case ARGS_OPT_NO_CHECK_CERTIFICATE:
+   conf->no_check_certificate = 1;
case ':':
parse_err = -1;
break;
@@ -335,6 +340,7 @@ static void usage()
printf
("\t--force-remove  Remove package even if prerm script fails\n");
printf("\t--force-checksum  Don't fail on checksum mismatches\n");
+   printf("\t--no-check-certificate Don't validate the server's 
certificate\n");
printf("\t--noactionNo action -- test only\n");
printf("\t--download-only   No action -- download only\n");
printf("\t--nodeps  Do not follow dependencies\n");
-- 
2.7.4


___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

[LEDE-DEV] [PATCH][opkg-lede] opkg: add --force-ssl argument

[Alexandru Ardelean]

For cases when artifacts are stored on https:// accessible
location and you don't want to install ca-certificates
(for various reasons).

I'll admit, using SSL like this is not recommended,
but since wget (even uclient-fetch) allows the
--no-check-certificate option, it would be nice
for opkg to support setting it if needed/configured.

Signed-off-by: Alexandru Ardelean <ardeleana...@gmail.com>
---
 libopkg/opkg_conf.c | 1 +
 libopkg/opkg_conf.h | 1 +
 libopkg/opkg_download.c | 5 -
 src/opkg-cl.c   | 6 ++
 4 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/libopkg/opkg_conf.c b/libopkg/opkg_conf.c
index 589fc49..1890145 100644
--- a/libopkg/opkg_conf.c
+++ b/libopkg/opkg_conf.c
@@ -54,6 +54,7 @@ opkg_option_t options[] = {
{"force_postinstall", OPKG_OPT_TYPE_BOOL, &_conf.force_postinstall},
{"force_checksum", OPKG_OPT_TYPE_BOOL, &_conf.force_checksum},
{"check_signature", OPKG_OPT_TYPE_BOOL, &_conf.check_signature},
+   {"force_ssl", OPKG_OPT_TYPE_BOOL, &_conf.force_ssl},
{"ftp_proxy", OPKG_OPT_TYPE_STRING, &_conf.ftp_proxy},
{"http_proxy", OPKG_OPT_TYPE_STRING, &_conf.http_proxy},
{"no_proxy", OPKG_OPT_TYPE_STRING, &_conf.no_proxy},
diff --git a/libopkg/opkg_conf.h b/libopkg/opkg_conf.h
index 9cf7681..a8c4a9e 100644
--- a/libopkg/opkg_conf.h
+++ b/libopkg/opkg_conf.h
@@ -78,6 +78,7 @@ struct opkg_conf {
int force_checksum;
int check_signature;
int force_signature;
+   int force_ssl;
int nodeps; /* do not follow dependencies */
int nocase; /* perform case insensitive matching */
char *offline_root;
diff --git a/libopkg/opkg_download.c b/libopkg/opkg_download.c
index db4c90f..c8e0013 100644
--- a/libopkg/opkg_download.c
+++ b/libopkg/opkg_download.c
@@ -87,11 +87,14 @@ opkg_download(const char *src, const char *dest_file_name,
 
{
int res;
-   const char *argv[8];
+   const char *argv[9];
int i = 0;
 
argv[i++] = "wget";
argv[i++] = "-q";
+   if (conf->force_ssl) {
+   argv[i++] = "--no-check-certificate";
+   }
if (conf->http_proxy || conf->ftp_proxy) {
argv[i++] = "-Y";
argv[i++] = "on";
diff --git a/src/opkg-cl.c b/src/opkg-cl.c
index c518bfc..77f59ff 100644
--- a/src/opkg-cl.c
+++ b/src/opkg-cl.c
@@ -52,6 +52,7 @@ enum {
ARGS_OPT_AUTOREMOVE,
ARGS_OPT_CACHE,
ARGS_OPT_FORCE_SIGNATURE,
+   ARGS_OPT_FORCE_SSL,
ARGS_OPT_SIZE,
 };
 
@@ -91,6 +92,8 @@ static struct option long_options[] = {
{"force_checksum", 0, 0, ARGS_OPT_FORCE_CHECKSUM},
{"force-signature", 0, 0, ARGS_OPT_FORCE_SIGNATURE},
{"force_signature", 0, 0, ARGS_OPT_FORCE_SIGNATURE},
+   {"force-ssl", 0, 0, ARGS_OPT_FORCE_SSL},
+   {"force_ssl", 0, 0, ARGS_OPT_FORCE_SSL},
{"noaction", 0, 0, ARGS_OPT_NOACTION},
{"download-only", 0, 0, ARGS_OPT_DOWNLOAD_ONLY},
{"nodeps", 0, 0, ARGS_OPT_NODEPS},
@@ -226,6 +229,8 @@ static int args_parse(int argc, char *argv[])
case ARGS_OPT_FORCE_SIGNATURE:
conf->force_signature = 1;
break;
+   case ARGS_OPT_FORCE_SSL:
+   conf->force_ssl = 1;
case ':':
parse_err = -1;
break;
@@ -335,6 +340,7 @@ static void usage()
printf
("\t--force-remove  Remove package even if prerm script fails\n");
printf("\t--force-checksum  Don't fail on checksum mismatches\n");
+   printf("\t--force-ssl   Don't validate the server's 
certificate\n");
printf("\t--noactionNo action -- test only\n");
printf("\t--download-only   No action -- download only\n");
printf("\t--nodeps  Do not follow dependencies\n");
-- 
2.7.4


___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

Re: [LEDE-DEV] [PATCH] kernel: update kernel 4.9 to 4.9.25

[Alexandru Ardelean]

Hey,

You can also Reject your patch here:
http://patchwork.ozlabs.org/project/lede/list/

Specifically, this patch is:
http://patchwork.ozlabs.org/patch/756400/

And [for reference] you can see all patches submitted by you [from
your email account]:
http://patchwork.ozlabs.org/project/lede/list/?submitter=69184=*===

The email list is indexed in that tool.

You do need to create an account.
But it's a simple process ; should not require too many personal details.

Patchwork is still partially used for tracking some patches sent via email.
Tho, I will admit, I'm more of a fan of the Github PR process.

Alex


On Mon, May 8, 2017 at 10:29 AM, Koen Vandeputte
 wrote:
> Dear,
>
> Please cancel this patch
>
>
> Since this was submitted:
>
> - 4.9.27 already arrived
> - Some other kernel patches have been submitted or got staged after this
> patch was created
>
> Logically, this one will now not apply anymore.
>
>
> Koen
>
> On 2017-04-28 15:17, Koen Vandeputte wrote:
>>
>> - Refresh all patches
>> - Removed upstreamed
>> - Adapted 1
>>
>> Compiled & Tested on targets: cns3xxx & imx6
>>
>> Signed-off-by: Koen Vandeputte 
>> ---
>>   include/kernel-version.mk  |   4 +-
>>   .../802-usb-xhci-force-msi-renesas-xhci.patch  |   2 +-
>>   ...X-Add-back-handler-ignoring-external-impr.patch |  75 
>>   ...-BCM5301X-Correct-GIC_PPI-interrupt-flags.patch |  41 ---
>>   ...ave-host-bridge-window-resource-in-struct.patch | 131
>> -
>>   ...-add-support-for-performing-fake-doorbell.patch |  10 +-
>>   .../patches-4.9/905-BCM53573-minor-hacks.patch |   2 +-
>>   .../patches-4.9/950-0031-Add-dwc_otg-driver.patch  |   2 +-
>>   ...-thermal-driver-for-reporting-core-temper.patch |   2 +-
>>   ...le-CONFIG_MEMCG-but-leave-it-disabled-due.patch |   4 +-
>>   ...-Fix-hang-for-writing-messages-larger-tha.patch |  90 --
>>   .../031-ubifs-fix-RENAME_WHITEOUT-support.patch|  25 
>>   .../040-01-MIPS-Introduce-irq_stack.patch  |  70 ---
>>   ...2-MIPS-Stack-unwinding-while-on-IRQ-stack.patch |  42 ---
>>   ...hange-28-to-thread_info-if-coming-from-us.patch |  48 
>>   ...IPS-Switch-to-the-irq_stack-in-interrupts.patch | 116
>> --
>>   ...05-MIPS-Select-HAVE_IRQ_EXIT_ON_IRQ_STACK.patch |  21 
>>   ...ack-Fix-erroneous-jal-to-plat_irq_dispatc.patch |  35 --
>>   ...part-fix-parsing-first-block-after-aligne.patch |  40 ---
>>   ...jecting-with-source-address-failed-policy.patch |  16 +--
>>   .../810-pci_disable_common_quirks.patch|   6 +-
>>   .../generic/patches-4.9/904-debloat_dma_buf.patch  |   2 +-
>>   ...sdhc-imx-increase-the-pad-I-O-drive-stren.patch |  42 ---
>>   ...om-use-scm_call-to-route-GPIO-irq-to-Apps.patch |   2 +-
>>   .../0008-MIPS-lantiq-backport-old-timer-code.patch |   2 +-
>>   ...-lantiq-wifi-and-ethernet-eeprom-handling.patch |   2 +-
>>   ...soc-mediatek-Add-MT2701-power-dt-bindings.patch |  10 +-
>>   ...k-Refine-scpsys-to-support-multiple-platf.patch |  19 ++-
>>   ...015-soc-mediatek-Add-MT2701-scpsys-driver.patch |  12 +-
>>   .../patches-4.9/0071-pwm-add-pwm-mediatek.patch|  16 +--
>>   .../linux/mediatek/patches-4.9/0083-mfd-led3.patch |   4 +-
>>   .../mediatek/patches-4.9/0085-pmic-led0.patch  |   3 -
>>   .../mediatek/patches-4.9/0086-pmic-led1.patch  |   4 +-
>>   .../mediatek/patches-4.9/0087-pmic-led2.patch  |  10 +-
>>   .../mediatek/patches-4.9/0088-pmic-led3.patch  |   4 +-
>>   target/linux/mediatek/patches-4.9/0091-dsa1.patch  |   3 -
>>   .../0091-net-next-mediatek-fix-DQL-support.patch   |   6 +-
>>   target/linux/mediatek/patches-4.9/0092-dsa2.patch  |  23 +---
>>   target/linux/mediatek/patches-4.9/0092-dsa3.patch  |   6 +-
>>   target/linux/mediatek/patches-4.9/0092-dsa4.patch  |   4 +-
>>   target/linux/mediatek/patches-4.9/0092-dsa5.patch  |  16 +--
>>   .../mediatek/patches-4.9/0093-dsa-compat.patch |  18 +--
>>   .../mediatek/patches-4.9/0094-net-affinity.patch   |  12 +-
>>   target/linux/mediatek/patches-4.9/0095-ephy.patch  |  12 +-
>>   .../mediatek/patches-4.9/0096-dsa-multi-cpu.patch  |  30 ++---
>>   .../mediatek/patches-4.9/0097-dsa-mt7530.patch |   6 +-
>>   .../patches-4.9/200-rt3883-fix-pinctrl-typo.patch  |  21 
>>   47 files changed, 91 insertions(+), 980 deletions(-)
>>   delete mode 100644
>> target/linux/bcm53xx/patches-4.9/031-ARM-BCM5301X-Add-back-handler-ignoring-external-impr.patch
>>   delete mode 100644
>> target/linux/bcm53xx/patches-4.9/033-0013-ARM-dts-BCM5301X-Correct-GIC_PPI-interrupt-flags.patch
>>   delete mode 100644
>> target/linux/bcm53xx/patches-4.9/089-PCI-iproc-Save-host-bridge-window-resource-in-struct.patch
>>   delete mode 100644
>> target/linux/brcm2708/patches-4.9/950-0106-i2c-bcm2835-Fix-hang-for-writing-messages-larger-tha.patch
>>   delete mode 100644
>> 

Re: [LEDE-DEV] What optimizations does low-end MIPS userspace need?

[Alexandru Ardelean]

On Sun, Apr 16, 2017 at 1:47 AM, Jay Carlson <n...@nop.com> wrote:
>
>> On Apr 15, 2017, at 1:57 AM, Alexandru Ardelean <ardeleana...@gmail.com> 
>> wrote:
>>
>> On Sat, 15 Apr 2017 at 02:01, Jay Carlson <n...@nop.com> wrote:
>>
>>> I’ve been fooling around with various userspace toolchain hacks for mips, 
>>> trying to make things faster globally. But then I realized: I don’t know 
>>> what “faster” is. Or what “better” is, in general.
>>>
>>> I want my Python/Firmata apps to launch faster on mt7688, but there is a 
>>> clear metric for winning there; I don’t need your help. :-)
>>
>> Strictly speaking for Python (if you haven't already) you might want to 
>> check some recent changes.
>>
>> Python now ships with bytecode files instead of source.
>> Seems that app startup is improved by even a factor of 9x on some low end 
>> CPUs.
>> Something like 500 msecs to 70 msecs to start a simple Hello World script.
>
> Very cool. That’s
>
> "python: use default host build prefix, remove cross-compile workarounds for 
> host” [1]
>
> and
>
> "python,python3: drop remove .pyc & .pyo files” [2]
>
> right?
>

Ah, last reply got through only to you, and not the list.
I've sent it from my phone, and for some reason, the Plain Text mode
wasn't enabled on it.
Will have to check that ; my browser has it enabled.

The commits are in this PR:
https://github.com/openwrt/packages/pull/4130/commits
Some are cleanups, and a few actually deal with the byte-code changes.

This commit should be the more important one : "python: split source
packages away from compiled packages "
But, it has some pre-work commits.


> --
> Jay Carlson
> n...@nop.com
>
> [0]: Am I supposed to be citing the openwrt package repository? If so, how?

not sure ; i don't know of a rule

> [1]: 
> https://github.com/openwrt/packages/commit/b70b978cc61bdd6fcdc7f8f7bae8ee3ef16baf4a
> [2}: 
> https://github.com/openwrt/packages/commit/05f8d6edf06c93dadab34f14a5fad48449dd7eda
>
>

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

[LEDE-DEV] [PATCH] base-files: use restart if no reload hook for service

[Alexandru Ardelean]

This was also working before, with a slightly
different semantic.

[ Original semantic ]
If no reload hooks was implemented, the default one would
kick in, it would return fail, and restart would happen.

This would happen also in the case where a reload hook
would be implemented, it would fail, and it would restart
the service.

[ New semantic ]
The default reload hook calls restart.
Services can implement their own reload.

If reload fails, then the '/etc/init.d/ reload'
would return a non-zero code, and the caller can choose
a way to handle this.

Signed-off-by: Alexandru Ardelean <ardeleana...@gmail.com>
---
 package/base-files/files/etc/rc.common | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/package/base-files/files/etc/rc.common 
b/package/base-files/files/etc/rc.common
index 95cf956..af7bed4 100755
--- a/package/base-files/files/etc/rc.common
+++ b/package/base-files/files/etc/rc.common
@@ -17,7 +17,7 @@ stop() {
 }
 
 reload() {
-   return 1
+   restart
 }
 
 restart() {
@@ -68,7 +68,7 @@ Available commands:
start   Start the service
stopStop the service
restart Restart the service
-   reload  Reload configuration files (or restart if that fails)
+   reload  Reload configuration files (or restart if service does not 
implement reload)
enable  Enable service autostart
disable Disable service autostart
 $EXTRA_HELP
@@ -130,7 +130,7 @@ ${INIT_TRACE:+set -x}
if eval "type reload_service" 2>/dev/null >/dev/null; then
reload_service "$@"
else
-   start
+   restart
fi
}
 
@@ -141,5 +141,4 @@ ${INIT_TRACE:+set -x}
 
 ALL_COMMANDS="start stop reload restart boot shutdown enable disable enabled 
depends ${EXTRA_COMMANDS}"
 list_contains ALL_COMMANDS "$action" || action=help
-[ "$action" = "reload" ] && action='eval reload "$@" || restart "$@" && :'
 $action "$@"
-- 
2.7.4


___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

Re: [LEDE-DEV] [PATCH 2/3][RFC] base-files: add option to rc.common to disable default reload behavior

[Alexandru Ardelean]

On Tue, Mar 28, 2017 at 11:36 PM, Hans Dedecker <dedec...@gmail.com> wrote:
> On Mon, Mar 20, 2017 at 3:08 PM, Alexandru Ardelean
> <ardeleana...@gmail.com> wrote:
>> From: Alexandru Ardelean <ardeleana...@gmail.com>
>>
>> Traditionally if a reload script fails, it will fallback to restart.
>>
>> That seems to be the default behavior in case no reload
>> handler has been specified, and `reload` will return 1.
>>
>> That also has the disadvantage of masking reload errors
>> from `/etc/init.d/ reload`.
>>
>> Still, it's a pretty old behavior, and in most cases
>> it should be fine.
>>
>> For other cases, the `RESTART_ON_RELOAD_ERR=0` can
>> be specified to override this.
>>
>> Not sure about the correctness of this approach,
>> so this patch is RFC.
> Discussed the restart-on-reload-fail behaviour with Felix and
> Matthias; general consensus is to remove the restart-on-reload-fail
> behaviour in rc.common. Packages do not seem to depend on this
> restart-on-reload fail behaviour and it's more logical to handle
> reload failure in the packages itself if extra logic is required.

Thanks for the consideration.

I'll submit another patch series just for this restart-on-reload fail.

I feel this mechanism was maybe intended to act as default reload
implementation.
In the sense, that a default reload hook is implemented returning
non-zero, and if no specific reload is implemented, this logic [
restart on reload fail ] would work.

Will think about it a bit.

Thanks
Alex

>
> Hans
>>
>> Signed-off-by: Alexandru Ardelean <ardeleana...@gmail.com>
>> ---
>>  package/base-files/files/etc/rc.common | 8 +++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/package/base-files/files/etc/rc.common 
>> b/package/base-files/files/etc/rc.common
>> index 95cf956..a893b09 100755
>> --- a/package/base-files/files/etc/rc.common
>> +++ b/package/base-files/files/etc/rc.common
>> @@ -139,7 +139,13 @@ ${INIT_TRACE:+set -x}
>> }
>>  }
>>
>> +RESTART_ON_RELOAD_ERR=${RESTART_ON_RELOAD_ERR:-1}
>> +
>>  ALL_COMMANDS="start stop reload restart boot shutdown enable disable 
>> enabled depends ${EXTRA_COMMANDS}"
>>  list_contains ALL_COMMANDS "$action" || action=help
>> -[ "$action" = "reload" ] && action='eval reload "$@" || restart "$@" && :'
>> +[ "$action" = "reload" ] && {
>> +   if [ "$RESTART_ON_RELOAD_ERR" == "1" ] ; then
>> +   action='eval reload "$@" || restart "$@" && :'
>> +   fi
>> +}
>>  $action "$@"
>> --
>> 2.7.4
>>

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

Re: [LEDE-DEV] [PATCH] openssl: Remove RIPEMD-160 from OpenSSL

[Alexandru Ardelean]

On Tue, Mar 28, 2017 at 1:45 AM, txt.file  wrote:
> The topic and patch is about OpenSSL but description is about OpenSSH.
> What has OpenSSL to do with OpenSSH?
>
> kind regards
> txt.file
> --
> This message is signed.
>
> Rosen Penev:
>> The commit that removed no-ripemd stated that it was needed for openssh.
>> However with recent OpenSSH releases (7.4), RIPEMD-160 is run-time disabled.
>> I've verified this with ssh -vvv making no mention of RIPEMD-160 anywhere.
>> ---
>>  package/libs/openssl/Makefile | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/package/libs/openssl/Makefile b/package/libs/openssl/Makefile
>> index 2543a46..a2d3ce3 100644
>> --- a/package/libs/openssl/Makefile
>> +++ b/package/libs/openssl/Makefile
>> @@ -100,7 +100,7 @@ endef
>>
>>
>>  OPENSSL_NO_CIPHERS:= no-idea no-md2 no-mdc2 no-rc5 no-sha0 no-camellia 
>> no-krb5 \
>> - no-whrlpool no-whirlpool no-seed no-jpake
>> + no-whrlpool no-whirlpool no-seed no-jpake no-ripemd
>>  OPENSSL_OPTIONS:= shared no-err no-sse2 no-ssl2 no-ssl2-method no-heartbeats
>>
>>  ifdef CONFIG_OPENSSL_ENGINE_CRYPTO
>>
>
>
> ___
> Lede-dev mailing list
> Lede-dev@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/lede-dev
>

as far as things go, openssh is part of the package feeds here:
https://github.com/openwrt/packages/tree/master/net/openssh

while openssl is part of the core packages
removing this cipher if unused, makes sense also to reduce openssl size

my 2c :)

thanks
Alex

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

[LEDE-DEV] [PATCH][V4] netifd: propagate error code on netifd_reload()

[Alexandru Ardelean]

The context is that we generate some of the UCI config
for netifd via scripts/programs.

Every once in a while, there's a goof when doing that
UCI generation, and netifd prints out the error at
stderr, but returns 0 (success) err-code.

This change will fail the ubus call if UCI config
is invalid or missing for /etc/config/network.

Signed-off-by: Alexandru Ardelean <ardeleana...@gmail.com>
---
 config.c | 10 --
 config.h |  2 +-
 main.c   |  4 ++--
 netifd.h |  2 +-
 ubus.c   |  5 +++--
 5 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/config.c b/config.c
index 0d965d3..2454e9b 100644
--- a/config.c
+++ b/config.c
@@ -393,16 +393,20 @@ config_init_wireless(void)
vlist_flush(>interfaces);
 }
 
-void
+int
 config_init_all(void)
 {
+   int ret = 0;
+
uci_network = config_init_package("network");
if (!uci_network) {
fprintf(stderr, "Failed to load network config\n");
-   return;
+   return -1;
}
 
uci_wireless = config_init_package("wireless");
+   if (!uci_wireless)
+   ret = -1;
 
vlist_update();
config_init = true;
@@ -426,4 +430,6 @@ config_init_all(void)
interface_refresh_assignments(false);
interface_start_pending();
wireless_start_pending();
+
+   return ret;
 }
diff --git a/config.h b/config.h
index 5adaca6..fae7cd8 100644
--- a/config.h
+++ b/config.h
@@ -19,6 +19,6 @@
 
 extern bool config_init;
 
-void config_init_all(void);
+int config_init_all(void);
 
 #endif
diff --git a/main.c b/main.c
index 5717b81..c173cef 100644
--- a/main.c
+++ b/main.c
@@ -208,9 +208,9 @@ static void netifd_do_restart(struct uloop_timeout *timeout)
execvp(global_argv[0], global_argv);
 }
 
-void netifd_reload(void)
+int netifd_reload(void)
 {
-   config_init_all();
+   return config_init_all();
 }
 
 void netifd_restart(void)
diff --git a/netifd.h b/netifd.h
index 5a90858..e565423 100644
--- a/netifd.h
+++ b/netifd.h
@@ -98,6 +98,6 @@ struct interface;
 extern const char *main_path;
 extern const char *config_path;
 void netifd_restart(void);
-void netifd_reload(void);
+int netifd_reload(void);
 
 #endif
diff --git a/ubus.c b/ubus.c
index 1b1a4cd..c17f3b3 100644
--- a/ubus.c
+++ b/ubus.c
@@ -44,8 +44,9 @@ netifd_handle_reload(struct ubus_context *ctx, struct 
ubus_object *obj,
 struct ubus_request_data *req, const char *method,
 struct blob_attr *msg)
 {
-   netifd_reload();
-   return 0;
+   if (netifd_reload())
+   return UBUS_STATUS_UNKNOWN_ERROR;
+   return UBUS_STATUS_OK;
 }
 
 enum {
-- 
2.7.4


___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

Re: [LEDE-DEV] [PATCH 1/3][RFC] netifd: propagate error code on netifd_reload()

[Alexandru Ardelean]

On Sun, Mar 26, 2017 at 7:06 PM, Hans Dedecker <dedec...@gmail.com> wrote:
> On Mon, Mar 20, 2017 at 3:08 PM, Alexandru Ardelean
> <ardeleana...@gmail.com> wrote:
>> From: Alexandru Ardelean <ardeleana...@gmail.com>
>>
>> The context is that we generate some of the UCI config
>> for netifd via scripts/programs.
>>
>> Every once in a while, there's a goof when doing that
>> UCI generation, and netifd prints out the error at
>> stderr, but returns 0 (success) err-code.
>>
>> This change will fail the ubus call if UCI config
>> is invalid or missing for /etc/config/network.
>>
>> Signed-off-by: Alexandru Ardelean <ardeleana...@gmail.com>
>> ---
>>  config.c | 10 --
>>  config.h |  9 -
>>  main.c   |  4 ++--
>>  netifd.h |  2 +-
>>  ubus.c   |  8 ++--
>>  5 files changed, 25 insertions(+), 8 deletions(-)
>>
>> diff --git a/config.c b/config.c
>> index 0d965d3..3b1af82 100644
>> --- a/config.c
>> +++ b/config.c
>> @@ -393,16 +393,20 @@ config_init_wireless(void)
>> vlist_flush(>interfaces);
>>  }
>>
>> -void
>> +int
>>  config_init_all(void)
>>  {
>> +   int ret = CONFIG_INIT_OK;
>> +
>> uci_network = config_init_package("network");
>> if (!uci_network) {
>> fprintf(stderr, "Failed to load network config\n");
>> -   return;
>> +   return CONFIG_INIT_ERR_NO_NETWORK;
>> }
>>
>> uci_wireless = config_init_package("wireless");
>> +   if (!uci_wireless)
>> +   ret = CONFIG_INIT_ERR_NO_WIRELESS;
>>
>> vlist_update();
>> config_init = true;
>> @@ -426,4 +430,6 @@ config_init_all(void)
>> interface_refresh_assignments(false);
>> interface_start_pending();
>> wireless_start_pending();
>> +
>> +   return ret;
>>  }
>> diff --git a/config.h b/config.h
>> index 5adaca6..df30b64 100644
>> --- a/config.h
>> +++ b/config.h
>> @@ -19,6 +19,13 @@
>>
>>  extern bool config_init;
>>
>> -void config_init_all(void);
>> +enum {
>> +   CONFIG_INIT_OK,
>> +   CONFIG_INIT_ERR_NO_WIRELESS,
>> +   CONFIG_INIT_ERR_NO_NETWORK,
>> +   __CONFIG_INIT_LAST
>> +};
>> +
>> +int config_init_all(void);
>>
>>  #endif
>> diff --git a/main.c b/main.c
>> index 5717b81..c173cef 100644
>> --- a/main.c
>> +++ b/main.c
>> @@ -208,9 +208,9 @@ static void netifd_do_restart(struct uloop_timeout 
>> *timeout)
>> execvp(global_argv[0], global_argv);
>>  }
>>
>> -void netifd_reload(void)
>> +int netifd_reload(void)
>>  {
>> -   config_init_all();
>> +   return config_init_all();
>>  }
>>
>>  void netifd_restart(void)
>> diff --git a/netifd.h b/netifd.h
>> index 5a90858..e565423 100644
>> --- a/netifd.h
>> +++ b/netifd.h
>> @@ -98,6 +98,6 @@ struct interface;
>>  extern const char *main_path;
>>  extern const char *config_path;
>>  void netifd_restart(void);
>> -void netifd_reload(void);
>> +int netifd_reload(void);
>>
>>  #endif
>> diff --git a/ubus.c b/ubus.c
>> index 1b1a4cd..31e535c 100644
>> --- a/ubus.c
>> +++ b/ubus.c
>> @@ -44,8 +44,12 @@ netifd_handle_reload(struct ubus_context *ctx, struct 
>> ubus_object *obj,
>>  struct ubus_request_data *req, const char *method,
>>  struct blob_attr *msg)
>>  {
>> -   netifd_reload();
>> -   return 0;
>> +   switch (netifd_reload()) {
>> +   case CONFIG_INIT_ERR_NO_NETWORK:
>> +   return UBUS_STATUS_UNKNOWN_ERROR;
> Is there any reason why only an ubus error code is returned for a
> network uci failure and not for a wireless uci failure ?
>
> Hans
>> +   default:
>> +   return UBUS_STATUS_OK;
>> +   }
>>  }
>>
>>  enum {
>> --
>> 2.7.4
>>


That would make the "ubus call network reload" return non-zero.
Hmm, I guess that would not be a problem.
I interpreted Felix's comment a bit differently regarding not failing
the wireless config.

Alex

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

Re: [LEDE-DEV] [PATCH 1/3][RFC] netifd: propagate error code on netifd_reload()

[Alexandru Ardelean]

On Mon, Mar 20, 2017 at 4:08 PM, Alexandru Ardelean
<ardeleana...@gmail.com> wrote:
> From: Alexandru Ardelean <ardeleana...@gmail.com>
>
> The context is that we generate some of the UCI config
> for netifd via scripts/programs.
>
> Every once in a while, there's a goof when doing that
> UCI generation, and netifd prints out the error at
> stderr, but returns 0 (success) err-code.
>
> This change will fail the ubus call if UCI config
> is invalid or missing for /etc/config/network.
>
> Signed-off-by: Alexandru Ardelean <ardeleana...@gmail.com>
> ---
>  config.c | 10 --
>  config.h |  9 -
>  main.c   |  4 ++--
>  netifd.h |  2 +-
>  ubus.c   |  8 ++--
>  5 files changed, 25 insertions(+), 8 deletions(-)
>
> diff --git a/config.c b/config.c
> index 0d965d3..3b1af82 100644
> --- a/config.c
> +++ b/config.c
> @@ -393,16 +393,20 @@ config_init_wireless(void)
> vlist_flush(>interfaces);
>  }
>
> -void
> +int
>  config_init_all(void)
>  {
> +   int ret = CONFIG_INIT_OK;
> +
> uci_network = config_init_package("network");
> if (!uci_network) {
> fprintf(stderr, "Failed to load network config\n");
> -   return;
> +   return CONFIG_INIT_ERR_NO_NETWORK;
> }
>
> uci_wireless = config_init_package("wireless");
> +   if (!uci_wireless)
> +   ret = CONFIG_INIT_ERR_NO_WIRELESS;
>
> vlist_update();
> config_init = true;
> @@ -426,4 +430,6 @@ config_init_all(void)
> interface_refresh_assignments(false);
> interface_start_pending();
> wireless_start_pending();
> +
> +   return ret;
>  }
> diff --git a/config.h b/config.h
> index 5adaca6..df30b64 100644
> --- a/config.h
> +++ b/config.h
> @@ -19,6 +19,13 @@
>
>  extern bool config_init;
>
> -void config_init_all(void);
> +enum {
> +   CONFIG_INIT_OK,
> +   CONFIG_INIT_ERR_NO_WIRELESS,
> +   CONFIG_INIT_ERR_NO_NETWORK,
> +   __CONFIG_INIT_LAST
> +};
> +
> +int config_init_all(void);
>
>  #endif
> diff --git a/main.c b/main.c
> index 5717b81..c173cef 100644
> --- a/main.c
> +++ b/main.c
> @@ -208,9 +208,9 @@ static void netifd_do_restart(struct uloop_timeout 
> *timeout)
> execvp(global_argv[0], global_argv);
>  }
>
> -void netifd_reload(void)
> +int netifd_reload(void)
>  {
> -   config_init_all();
> +   return config_init_all();
>  }
>
>  void netifd_restart(void)
> diff --git a/netifd.h b/netifd.h
> index 5a90858..e565423 100644
> --- a/netifd.h
> +++ b/netifd.h
> @@ -98,6 +98,6 @@ struct interface;
>  extern const char *main_path;
>  extern const char *config_path;
>  void netifd_restart(void);
> -void netifd_reload(void);
> +int netifd_reload(void);
>
>  #endif
> diff --git a/ubus.c b/ubus.c
> index 1b1a4cd..31e535c 100644
> --- a/ubus.c
> +++ b/ubus.c
> @@ -44,8 +44,12 @@ netifd_handle_reload(struct ubus_context *ctx, struct 
> ubus_object *obj,
>  struct ubus_request_data *req, const char *method,
>  struct blob_attr *msg)
>  {
> -   netifd_reload();
> -   return 0;
> +   switch (netifd_reload()) {
> +   case CONFIG_INIT_ERR_NO_NETWORK:
> +   return UBUS_STATUS_UNKNOWN_ERROR;
> +   default:
> +   return UBUS_STATUS_OK;
> +   }
>  }
>
>  enum {
> --
> 2.7.4
>

hey,

sorry for spamming [if i gave that feeling now].
but, i prefer asking this question with a patch series with a RFC tag

one reason i went silent after my last RFC patch, is because a
colleague notified me that a reload that returns non-zero, would
trigger a restart
which would mask the error when checking if   `/etc/init.d/network
reload `  fails
a restart would not necessarily make things better [ from my point of view ]
i decided to check out [and brain storm internally] for an idea of how
to handle this

so, i am raising this point with you guys to get your take on this
i'll admit my proposals may not be the best approach to the matter

but in our internal tests, it would be nice to run
```
/etc/init.d/network reload || {
 fail_test
}
```

thanks
Alex

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

[LEDE-DEV] [PATCH 3/3][RFC] netifd: don't restart on reload error

[Alexandru Ardelean]

From: Alexandru Ardelean <ardeleana...@gmail.com>

We just want netifd to return a non-zero err-code.
Reload has failed, a restart will not make it better necessarily.

Signed-off-by: Alexandru Ardelean <ardeleana...@gmail.com>
---
 package/network/config/netifd/files/etc/init.d/network | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/package/network/config/netifd/files/etc/init.d/network 
b/package/network/config/netifd/files/etc/init.d/network
index a825dfd..1229ba2 100755
--- a/package/network/config/netifd/files/etc/init.d/network
+++ b/package/network/config/netifd/files/etc/init.d/network
@@ -4,6 +4,7 @@ START=20
 STOP=90
 
 USE_PROCD=1
+RESTART_ON_RELOAD_ERR=0
 
 init_switch() {
setup_switch() { return 0; }
@@ -27,7 +28,7 @@ start_service() {
 
 reload_service() {
init_switch
-   ubus call network reload
+   ubus call network reload || return 1
/sbin/wifi reload_legacy
 }
 
-- 
2.7.4


___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

[LEDE-DEV] [PATCH 1/3][RFC] netifd: propagate error code on netifd_reload()

[Alexandru Ardelean]

From: Alexandru Ardelean <ardeleana...@gmail.com>

The context is that we generate some of the UCI config
for netifd via scripts/programs.

Every once in a while, there's a goof when doing that
UCI generation, and netifd prints out the error at
stderr, but returns 0 (success) err-code.

This change will fail the ubus call if UCI config
is invalid or missing for /etc/config/network.

Signed-off-by: Alexandru Ardelean <ardeleana...@gmail.com>
---
 config.c | 10 --
 config.h |  9 -
 main.c   |  4 ++--
 netifd.h |  2 +-
 ubus.c   |  8 ++--
 5 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/config.c b/config.c
index 0d965d3..3b1af82 100644
--- a/config.c
+++ b/config.c
@@ -393,16 +393,20 @@ config_init_wireless(void)
vlist_flush(>interfaces);
 }
 
-void
+int
 config_init_all(void)
 {
+   int ret = CONFIG_INIT_OK;
+
uci_network = config_init_package("network");
if (!uci_network) {
fprintf(stderr, "Failed to load network config\n");
-   return;
+   return CONFIG_INIT_ERR_NO_NETWORK;
}
 
uci_wireless = config_init_package("wireless");
+   if (!uci_wireless)
+   ret = CONFIG_INIT_ERR_NO_WIRELESS;
 
vlist_update();
config_init = true;
@@ -426,4 +430,6 @@ config_init_all(void)
interface_refresh_assignments(false);
interface_start_pending();
wireless_start_pending();
+
+   return ret;
 }
diff --git a/config.h b/config.h
index 5adaca6..df30b64 100644
--- a/config.h
+++ b/config.h
@@ -19,6 +19,13 @@
 
 extern bool config_init;
 
-void config_init_all(void);
+enum {
+   CONFIG_INIT_OK,
+   CONFIG_INIT_ERR_NO_WIRELESS,
+   CONFIG_INIT_ERR_NO_NETWORK,
+   __CONFIG_INIT_LAST
+};
+
+int config_init_all(void);
 
 #endif
diff --git a/main.c b/main.c
index 5717b81..c173cef 100644
--- a/main.c
+++ b/main.c
@@ -208,9 +208,9 @@ static void netifd_do_restart(struct uloop_timeout *timeout)
execvp(global_argv[0], global_argv);
 }
 
-void netifd_reload(void)
+int netifd_reload(void)
 {
-   config_init_all();
+   return config_init_all();
 }
 
 void netifd_restart(void)
diff --git a/netifd.h b/netifd.h
index 5a90858..e565423 100644
--- a/netifd.h
+++ b/netifd.h
@@ -98,6 +98,6 @@ struct interface;
 extern const char *main_path;
 extern const char *config_path;
 void netifd_restart(void);
-void netifd_reload(void);
+int netifd_reload(void);
 
 #endif
diff --git a/ubus.c b/ubus.c
index 1b1a4cd..31e535c 100644
--- a/ubus.c
+++ b/ubus.c
@@ -44,8 +44,12 @@ netifd_handle_reload(struct ubus_context *ctx, struct 
ubus_object *obj,
 struct ubus_request_data *req, const char *method,
 struct blob_attr *msg)
 {
-   netifd_reload();
-   return 0;
+   switch (netifd_reload()) {
+   case CONFIG_INIT_ERR_NO_NETWORK:
+   return UBUS_STATUS_UNKNOWN_ERROR;
+   default:
+   return UBUS_STATUS_OK;
+   }
 }
 
 enum {
-- 
2.7.4


___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

[LEDE-DEV] [PATCH 2/3][RFC] base-files: add option to rc.common to disable default reload behavior

[Alexandru Ardelean]

From: Alexandru Ardelean <ardeleana...@gmail.com>

Traditionally if a reload script fails, it will fallback to restart.

That seems to be the default behavior in case no reload
handler has been specified, and `reload` will return 1.

That also has the disadvantage of masking reload errors
from `/etc/init.d/ reload`.

Still, it's a pretty old behavior, and in most cases
it should be fine.

For other cases, the `RESTART_ON_RELOAD_ERR=0` can
be specified to override this.

Not sure about the correctness of this approach,
so this patch is RFC.

Signed-off-by: Alexandru Ardelean <ardeleana...@gmail.com>
---
 package/base-files/files/etc/rc.common | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/package/base-files/files/etc/rc.common 
b/package/base-files/files/etc/rc.common
index 95cf956..a893b09 100755
--- a/package/base-files/files/etc/rc.common
+++ b/package/base-files/files/etc/rc.common
@@ -139,7 +139,13 @@ ${INIT_TRACE:+set -x}
}
 }
 
+RESTART_ON_RELOAD_ERR=${RESTART_ON_RELOAD_ERR:-1}
+
 ALL_COMMANDS="start stop reload restart boot shutdown enable disable enabled 
depends ${EXTRA_COMMANDS}"
 list_contains ALL_COMMANDS "$action" || action=help
-[ "$action" = "reload" ] && action='eval reload "$@" || restart "$@" && :'
+[ "$action" = "reload" ] && {
+   if [ "$RESTART_ON_RELOAD_ERR" == "1" ] ; then
+   action='eval reload "$@" || restart "$@" && :'
+   fi
+}
 $action "$@"
-- 
2.7.4


___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

Re: [LEDE-DEV] Kodi

[Alexandru Ardelean]

I'd just add an opinion here.
Please take it as such.

I think with Kodi [ specifically ] it's one of those
sugar-features/packages that could be interesting for tinker-ers to
try out when trying LEDE/OpenWrt.
Not sure how intensive it would be on the build infrastructure.

If you decide to package + maintain it, you'll probably see interest
start to pop-up in a year [max] from users of LEDE/OpenWrt [who are
not necessarily part of this mailing list].
I guess, the question is more up to you, if you have a personal
need/want to maintain it and support it.


On Wed, Mar 15, 2017 at 10:10 AM, Stijn Tintel  wrote:
> Last week I managed to get Kodi to build for LEDE, and I am running it
> on my Raspberry Pi Zero W. It's mostly quick and dirty, definitely not
> ready to push to the repository, and in its current form it will only
> work on Raspberry Pi. My main question now is: is there any interest in
> having Kodi for LEDE at all, or should I not bother preparing it for
> submission.
>
> Posted a few screenshots of the "System info" section on Twitter:
> https://twitter.com/stintel/status/840501146343084032
>
> Thanks,
> Stijn
>
>
> ___
> Lede-dev mailing list
> Lede-dev@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/lede-dev

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

[LEDE-DEV] [PATCH][V2][netifd] ubus: propagate error code on netifd_reload()

[Alexandru Ardelean]

The context is that we generate some of the UCI config
for netifd via scripts/programs.

Every once in a while, there's a goof when doing that
UCI generation, and netifd prints out the error at
stderr, but returns 0 (success) err-code.

This change will fail the ubus call if UCI config
is invalid for /etc/config/network & /etc/config/wireless,
or could not be loaded (via uci_load()).

Admittedly, failing the entire ubus call could
be a bit much [depending on various views].
Probably one idea, would be to return the err-code
as a ubus reply data (via ubus_send_reply()).
Not sure.

Signed-off-by: Alexandru Ardelean <ardeleana...@gmail.com>
---
 config.c | 10 --
 config.h |  2 +-
 main.c   |  4 ++--
 netifd.h |  2 +-
 ubus.c   |  5 +++--
 5 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/config.c b/config.c
index 0d965d3..d70747c 100644
--- a/config.c
+++ b/config.c
@@ -393,16 +393,20 @@ config_init_wireless(void)
vlist_flush(>interfaces);
 }
 
-void
+int
 config_init_all(void)
 {
uci_network = config_init_package("network");
if (!uci_network) {
fprintf(stderr, "Failed to load network config\n");
-   return;
+   return -1;
}
 
uci_wireless = config_init_package("wireless");
+   if (!uci_wireless) {
+   fprintf(stderr, "Failed to load wireless config\n");
+   return -1;
+   }
 
vlist_update();
config_init = true;
@@ -426,4 +430,6 @@ config_init_all(void)
interface_refresh_assignments(false);
interface_start_pending();
wireless_start_pending();
+
+   return 0;
 }
diff --git a/config.h b/config.h
index 5adaca6..fae7cd8 100644
--- a/config.h
+++ b/config.h
@@ -19,6 +19,6 @@
 
 extern bool config_init;
 
-void config_init_all(void);
+int config_init_all(void);
 
 #endif
diff --git a/main.c b/main.c
index 5717b81..c173cef 100644
--- a/main.c
+++ b/main.c
@@ -208,9 +208,9 @@ static void netifd_do_restart(struct uloop_timeout *timeout)
execvp(global_argv[0], global_argv);
 }
 
-void netifd_reload(void)
+int netifd_reload(void)
 {
-   config_init_all();
+   return config_init_all();
 }
 
 void netifd_restart(void)
diff --git a/netifd.h b/netifd.h
index 5a90858..e565423 100644
--- a/netifd.h
+++ b/netifd.h
@@ -98,6 +98,6 @@ struct interface;
 extern const char *main_path;
 extern const char *config_path;
 void netifd_restart(void);
-void netifd_reload(void);
+int netifd_reload(void);
 
 #endif
diff --git a/ubus.c b/ubus.c
index 66f714a..c9813fc 100644
--- a/ubus.c
+++ b/ubus.c
@@ -44,8 +44,9 @@ netifd_handle_reload(struct ubus_context *ctx, struct 
ubus_object *obj,
 struct ubus_request_data *req, const char *method,
 struct blob_attr *msg)
 {
-   netifd_reload();
-   return 0;
+   if (netifd_reload())
+   return UBUS_STATUS_UNKNOWN_ERROR;
+   return UBUS_STATUS_OK;
 }
 
 enum {
-- 
2.7.4


___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

[LEDE-DEV] [PATCH] netifd: propagate error code on netifd_reload()

[Alexandru Ardelean]

The context is that we generate some of the UCI config
for netifd via scripts/programs.

Every once in a while, there's a goof when doing that
UCI generation, and netifd prints out the error at
stderr, but returns 0 (success) err-code.

This change will fail the ubus call if UCI config
is invalid for /etc/config/network & /etc/config/wireless,
or could not be loaded (via uci_load()).

Admittedly, failing the entire ubus call could
be a bit much [depending on various views].
Probably one idea, would be to return the err-code
as a ubus reply data (via ubus_send_reply()).
Not sure.

Signed-off-by: Alexandru Ardelean <ardeleana...@gmail.com>
---
 config.c | 10 --
 config.h |  2 +-
 main.c   |  4 ++--
 netifd.h |  2 +-
 ubus.c   |  3 +--
 5 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/config.c b/config.c
index 0d965d3..d70747c 100644
--- a/config.c
+++ b/config.c
@@ -393,16 +393,20 @@ config_init_wireless(void)
vlist_flush(>interfaces);
 }
 
-void
+int
 config_init_all(void)
 {
uci_network = config_init_package("network");
if (!uci_network) {
fprintf(stderr, "Failed to load network config\n");
-   return;
+   return -1;
}
 
uci_wireless = config_init_package("wireless");
+   if (!uci_wireless) {
+   fprintf(stderr, "Failed to load wireless config\n");
+   return -1;
+   }
 
vlist_update();
config_init = true;
@@ -426,4 +430,6 @@ config_init_all(void)
interface_refresh_assignments(false);
interface_start_pending();
wireless_start_pending();
+
+   return 0;
 }
diff --git a/config.h b/config.h
index 5adaca6..fae7cd8 100644
--- a/config.h
+++ b/config.h
@@ -19,6 +19,6 @@
 
 extern bool config_init;
 
-void config_init_all(void);
+int config_init_all(void);
 
 #endif
diff --git a/main.c b/main.c
index 5717b81..c173cef 100644
--- a/main.c
+++ b/main.c
@@ -208,9 +208,9 @@ static void netifd_do_restart(struct uloop_timeout *timeout)
execvp(global_argv[0], global_argv);
 }
 
-void netifd_reload(void)
+int netifd_reload(void)
 {
-   config_init_all();
+   return config_init_all();
 }
 
 void netifd_restart(void)
diff --git a/netifd.h b/netifd.h
index 5a90858..e565423 100644
--- a/netifd.h
+++ b/netifd.h
@@ -98,6 +98,6 @@ struct interface;
 extern const char *main_path;
 extern const char *config_path;
 void netifd_restart(void);
-void netifd_reload(void);
+int netifd_reload(void);
 
 #endif
diff --git a/ubus.c b/ubus.c
index 66f714a..89f0db6 100644
--- a/ubus.c
+++ b/ubus.c
@@ -44,8 +44,7 @@ netifd_handle_reload(struct ubus_context *ctx, struct 
ubus_object *obj,
 struct ubus_request_data *req, const char *method,
 struct blob_attr *msg)
 {
-   netifd_reload();
-   return 0;
+   return netifd_reload();
 }
 
 enum {
-- 
2.7.4


___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

[LEDE-DEV] [PATCH][RFC] ubus: propagate error code on netifd_reload()

[Alexandru Ardelean]

The context is that we generate some of the UCI config
for netifd via scripts/programs.

Every once in a while, there's a goof when doing that
UCI generation, and netifd prints out the error at
stderr, but returns 0 (success) err-code.

This change will fail the ubus call if UCI config
is invalid for /etc/config/network & /etc/config/wireless,
or could not be loaded (via uci_load()).

Admittedly, failing the entire ubus call could
be a bit much [depending on various views].
Probably one idea, would be to return the err-code
as a ubus reply data (via ubus_send_reply()).
Not sure.

Signed-off-by: Alexandru Ardelean <ardeleana...@gmail.com>
---
 config.c | 10 --
 config.h |  2 +-
 main.c   |  4 ++--
 netifd.h |  2 +-
 ubus.c   |  3 +--
 5 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/config.c b/config.c
index 0d965d3..d70747c 100644
--- a/config.c
+++ b/config.c
@@ -393,16 +393,20 @@ config_init_wireless(void)
vlist_flush(>interfaces);
 }
 
-void
+int
 config_init_all(void)
 {
uci_network = config_init_package("network");
if (!uci_network) {
fprintf(stderr, "Failed to load network config\n");
-   return;
+   return -1;
}
 
uci_wireless = config_init_package("wireless");
+   if (!uci_wireless) {
+   fprintf(stderr, "Failed to load wireless config\n");
+   return -1;
+   }
 
vlist_update();
config_init = true;
@@ -426,4 +430,6 @@ config_init_all(void)
interface_refresh_assignments(false);
interface_start_pending();
wireless_start_pending();
+
+   return 0;
 }
diff --git a/config.h b/config.h
index 5adaca6..fae7cd8 100644
--- a/config.h
+++ b/config.h
@@ -19,6 +19,6 @@
 
 extern bool config_init;
 
-void config_init_all(void);
+int config_init_all(void);
 
 #endif
diff --git a/main.c b/main.c
index 5717b81..c173cef 100644
--- a/main.c
+++ b/main.c
@@ -208,9 +208,9 @@ static void netifd_do_restart(struct uloop_timeout *timeout)
execvp(global_argv[0], global_argv);
 }
 
-void netifd_reload(void)
+int netifd_reload(void)
 {
-   config_init_all();
+   return config_init_all();
 }
 
 void netifd_restart(void)
diff --git a/netifd.h b/netifd.h
index 5a90858..e565423 100644
--- a/netifd.h
+++ b/netifd.h
@@ -98,6 +98,6 @@ struct interface;
 extern const char *main_path;
 extern const char *config_path;
 void netifd_restart(void);
-void netifd_reload(void);
+int netifd_reload(void);
 
 #endif
diff --git a/ubus.c b/ubus.c
index 66f714a..89f0db6 100644
--- a/ubus.c
+++ b/ubus.c
@@ -44,8 +44,7 @@ netifd_handle_reload(struct ubus_context *ctx, struct 
ubus_object *obj,
 struct ubus_request_data *req, const char *method,
 struct blob_attr *msg)
 {
-   netifd_reload();
-   return 0;
+   return netifd_reload();
 }
 
 enum {
-- 
2.7.4


___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

Re: [LEDE-DEV] Identifying kernel version (major) during build (.mk file)

[Alexandru Ardelean]

Hey Mauro,

Your case seems a bit specific.
But I guess you can try a few things and inspire yourself from
include/kernel-version.mk

https://github.com/lede-project/source/blob/master/include/kernel-version.mk

Not sure if the KERNEL_BASE variable is what you need.
i.e. I can't tell what the output will be in that case.

But from the looks of it, you can also probably use the split_version
[ + maybe also the firstword ] function[s] to extract the major
version.



On Sat, Feb 18, 2017 at 1:49 PM, Mauro Mozzarelli  wrote:
> I was wondering someone could help me the following problem:
>
> I want to add some device drivers to the kernel build, but the kernel
> configuration in kernel version 3 is different from the configuration in
> kernel version 4.
>
> I was looking for a parameter that I could use inside an .mk file to find
> which kernel version I am building for.
>
> So far with trial and error (unfortunately I could not find specific
> documentation) I found that I can test reliably a variable that includes
> both kernel version and patchlevel as follows:
>
> LINUX_4_0||LINUX_4_1||LINUX_4_2||LINUX_4_3||LINUX_4_4||LINUX_4_5
>
> ... and so on.
>
> However all I would need is something like LINUX_4 or LINUX_3
>
> otherwise I would have to list every possible patchlevel (present and future
> to allow the build when patchlevel changes).
>
> Thank you in advance,
>
>
> Mauro
>
> ___
> Lede-dev mailing list
> Lede-dev@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/lede-dev

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

Re: [LEDE-DEV] [PATCH] libubus: initialize list member for ubus_auto_conn's timer

[Alexandru Ardelean]

On Tue, Dec 13, 2016 at 9:52 AM, Felix Fietkau <n...@nbd.name> wrote:
> On 2016-12-13 08:41, Alexandru Ardelean wrote:
>> Every once in a while, we'll get stacktrace:
>> ```
>> (gdb) bt
>> \#0  0xb7bc4668 in _list_del (entry=0x10015688 <conn+116>) at 
>> /home/sandu/work/Wrt/openwrt/build_dir/target-powerpc_8540_uClibc-0.9.33.2_ewok/libubox-2015-06-29/list.h:83
>> \#1  list_del (entry=0x10015688 <conn+116>) at 
>> /home/sandu/work/Wrt/openwrt/build_dir/target-powerpc_8540_uClibc-0.9.33.2_ewok/libubox-2015-06-29/list.h:90
>> \#2  uloop_timeout_cancel (timeout=timeout@entry=0x10015688 <conn+116>) at 
>> /home/sandu/work/Wrt/openwrt/build_dir/target-powerpc_8540_uClibc-0.9.33.2_ewok/libubox-2015-06-29/uloop.c:474
>> \#3  0x10003794 in ubus_auto_shutdown (conn=0x10015614 ) at 
>> /home/sandu/work/Wrt/openwrt/staging_dir/target-powerpc_8540_uClibc-0.9.33.2_ewok/usr/include/libubus.h:249
>> \#4  module_ubus_fini () at ubus.c:64
>> \#5  0x100013fc in main (argc=, argv=) at 
>> module.c:128
>> ```
>>
>> In our code, `ubus_auto_connect()` is called, then due to some logic,
>> the module has to quickly stop, calling ubus_auto_shutdown().
>>
>> It seems that there is case in where the `timer` timeout, is not yet
>> registered with the internal list of timeouts from uloop, which
>> ends up trying to delete an invalid list.
>>
>> So, one solution is to init the list element of the `timer` of
>> the `ubus_auto_conn` struct.
>>
>> Signed-off-by: Alexandru Ardelean <ardeleana...@gmail.com>
> I think this patch is a workaround that simply hides the real issue.
> uloop_timeout_cancel will only call list_del if the timer 'pending' flag
> is set.
>
> It seems to me that you might be using ubus_auto_connect on
> uninitialized memory. If that's the case, you might run into issues in
> other places as well, and you should just add a memset to your application.
>
> - Felix

Hmmm, you're right.
It is only one module I'm seeing this in.
Maybe it's some weird memory corruption, or race ; since that
ubus_auto_conn struct is init-ed to 0 [since it's static].

Will dig deeper into that.

Sorry for the noise.

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

[LEDE-DEV] [PATCH] libubus: initialize list member for ubus_auto_conn's timer

[Alexandru Ardelean]

Every once in a while, we'll get stacktrace:
```
(gdb) bt
\#0  0xb7bc4668 in _list_del (entry=0x10015688 <conn+116>) at 
/home/sandu/work/Wrt/openwrt/build_dir/target-powerpc_8540_uClibc-0.9.33.2_ewok/libubox-2015-06-29/list.h:83
\#1  list_del (entry=0x10015688 <conn+116>) at 
/home/sandu/work/Wrt/openwrt/build_dir/target-powerpc_8540_uClibc-0.9.33.2_ewok/libubox-2015-06-29/list.h:90
\#2  uloop_timeout_cancel (timeout=timeout@entry=0x10015688 <conn+116>) at 
/home/sandu/work/Wrt/openwrt/build_dir/target-powerpc_8540_uClibc-0.9.33.2_ewok/libubox-2015-06-29/uloop.c:474
\#3  0x10003794 in ubus_auto_shutdown (conn=0x10015614 ) at 
/home/sandu/work/Wrt/openwrt/staging_dir/target-powerpc_8540_uClibc-0.9.33.2_ewok/usr/include/libubus.h:249
\#4  module_ubus_fini () at ubus.c:64
\#5  0x100013fc in main (argc=, argv=) at 
module.c:128
```

In our code, `ubus_auto_connect()` is called, then due to some logic,
the module has to quickly stop, calling ubus_auto_shutdown().

It seems that there is case in where the `timer` timeout, is not yet
registered with the internal list of timeouts from uloop, which
ends up trying to delete an invalid list.

So, one solution is to init the list element of the `timer` of
the `ubus_auto_conn` struct.

Signed-off-by: Alexandru Ardelean <ardeleana...@gmail.com>
---
 libubus.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libubus.c b/libubus.c
index 8163ff7..faa30d3 100644
--- a/libubus.c
+++ b/libubus.c
@@ -337,6 +337,7 @@ static void ubus_auto_connect_cb(struct uloop_timeout 
*timeout)
 void ubus_auto_connect(struct ubus_auto_conn *conn)
 {
conn->timer.cb = ubus_auto_connect_cb;
+   INIT_LIST_HEAD(>timer.list);
ubus_auto_connect_cb(>timer);
 }
 
-- 
2.6.6


___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

Re: [LEDE-DEV] [PATCH] kernel: update to v4.4.23

[Alexandru Ardelean]

On Sun, Nov 13, 2016 at 10:47 PM, Giuseppe Lippolis
 wrote:
> From: Álvaro Fernández Rojas 
>
> Refresh patches for all targets that support kernel 4.4.
> compile/run-tested on brcm2708/bcm2710 only.
>
> Signed-off-by: Álvaro Fernández Rojas 
> ---
>  include/kernel-version.mk  |  4 +--
>  .../ar71xx/patches-4.4/500-MIPS-fw-myloader.patch  |  2 +-
>  .../patches-4.4/201-extra_optimization.patch   | 16 +++
>  .../patches-4.4/202-reduce_module_size.patch   |  2 +-
>  .../generic/patches-4.4/280-rfkill-stubs.patch | 32 
> --
>  .../generic/patches-4.4/304-mips_disable_fpu.patch |  2 +-
>  .../645-bridge_multicast_to_unicast.patch  |  2 +-
>  ...80-NET-skip-GRO-for-foreign-MAC-addresses.patch | 10 +++
>  .../generic/patches-4.4/902-debloat_proc.patch |  4 +--
>  .../patches-4.4/0026-NET-multi-phy-support.patch   |  6 ++--
>  .../patches-4.4/0001-NET-multi-phy-support.patch   |  6 ++--
>  ...ovide-a-hook-for-link-up-link-down-events.patch | 18 ++--
>  .../patches-4.4/0034-NET-multi-phy-support.patch   |  6 ++--
>  13 files changed, 53 insertions(+), 57 deletions(-)
>
> diff --git a/include/kernel-version.mk b/include/kernel-version.mk
> index 0db1b35..64b25e8 100644
> --- a/include/kernel-version.mk
> +++ b/include/kernel-version.mk
> @@ -4,11 +4,11 @@ LINUX_RELEASE?=1
>
>  LINUX_VERSION-3.18 = .29
>  LINUX_VERSION-4.1 = .20
> -LINUX_VERSION-4.4 = .22
> +LINUX_VERSION-4.4 = .23
>
>  LINUX_KERNEL_MD5SUM-3.18.29 = b25737a0bc98e80d12200de93f239c28
>  LINUX_KERNEL_MD5SUM-4.1.20 = 075c38a3a23ca5bc80437b13606df00a
> -LINUX_KERNEL_MD5SUM-4.4.22 = 404802389c7f0bbe94dda95f9d058d9e
> +LINUX_KERNEL_MD5SUM-4.4.23 = 39c3e2090931e83b7dd3438e7fb276d4
>
>  ifdef KERNEL_PATCHVER
>LINUX_VERSION:=$(KERNEL_PATCHVER)$(strip 
> $(LINUX_VERSION-$(KERNEL_PATCHVER)))
> diff --git a/target/linux/ar71xx/patches-4.4/500-MIPS-fw-myloader.patch 
> b/target/linux/ar71xx/patches-4.4/500-MIPS-fw-myloader.patch
> index f823b59..e877b0c 100644
> --- a/target/linux/ar71xx/patches-4.4/500-MIPS-fw-myloader.patch
> +++ b/target/linux/ar71xx/patches-4.4/500-MIPS-fw-myloader.patch
> @@ -1,6 +1,6 @@
>  --- a/arch/mips/Makefile
>  +++ b/arch/mips/Makefile
> -@@ -222,6 +222,7 @@ cflags-$(CONFIG_MIPS_COMPACT_BRANCHES_AL
> +@@ -218,6 +218,7 @@ endif
>   #
>   libs-$(CONFIG_FW_ARC) += arch/mips/fw/arc/
>   libs-$(CONFIG_FW_CFE) += arch/mips/fw/cfe/
> diff --git a/target/linux/generic/patches-4.4/201-extra_optimization.patch 
> b/target/linux/generic/patches-4.4/201-extra_optimization.patch
> index bcb6a00..508627a 100644
> --- a/target/linux/generic/patches-4.4/201-extra_optimization.patch
> +++ b/target/linux/generic/patches-4.4/201-extra_optimization.patch
> @@ -1,14 +1,18 @@
>  --- a/Makefile
>  +++ b/Makefile
> -@@ -608,9 +608,9 @@ include arch/$(SRCARCH)/Makefile
> - KBUILD_CFLAGS += $(call cc-option,-fno-delete-null-pointer-checks,)
> +@@ -619,12 +619,12 @@ KBUILD_CFLAGS+= $(call cc-option,-fno-d
> + KBUILD_CFLAGS += $(call cc-disable-warning,maybe-uninitialized,)
>
>   ifdef CONFIG_CC_OPTIMIZE_FOR_SIZE
> --KBUILD_CFLAGS += -Os $(call cc-disable-warning,maybe-uninitialized,)
> -+KBUILD_CFLAGS += -Os $(EXTRA_OPTIMIZATION) $(call 
> cc-disable-warning,maybe-uninitialized,)
> +-KBUILD_CFLAGS += -Os
> ++KBUILD_CFLAGS += -Os $(EXTRA_OPTIMIZATION)
>   else
> + ifdef CONFIG_PROFILE_ALL_BRANCHES
>  -KBUILD_CFLAGS += -O2
> -+KBUILD_CFLAGS += -O2 -fno-reorder-blocks -fno-tree-ch $(EXTRA_OPTIMIZATION)
> ++KBUILD_CFLAGS += -O2 $(EXTRA_OPTIMIZATION)
> + else
> +-KBUILD_CFLAGS   += -O2
> ++KBUILD_CFLAGS   += -O2 -fno-reorder-blocks -fno-tree-ch 
> $(EXTRA_OPTIMIZATION)
> + endif
>   endif
>
> - # Tell gcc to never replace conditional load with a non-conditional one
> diff --git a/target/linux/generic/patches-4.4/202-reduce_module_size.patch 
> b/target/linux/generic/patches-4.4/202-reduce_module_size.patch
> index cef04d1..283d48d 100644
> --- a/target/linux/generic/patches-4.4/202-reduce_module_size.patch
> +++ b/target/linux/generic/patches-4.4/202-reduce_module_size.patch
> @@ -1,6 +1,6 @@
>  --- a/Makefile
>  +++ b/Makefile
> -@@ -398,7 +398,7 @@ KBUILD_CFLAGS_KERNEL :=
> +@@ -402,7 +402,7 @@ KBUILD_CFLAGS_KERNEL :=
>   KBUILD_AFLAGS   := -D__ASSEMBLY__
>   KBUILD_AFLAGS_MODULE  := -DMODULE
>   KBUILD_CFLAGS_MODULE  := -DMODULE
> diff --git a/target/linux/generic/patches-4.4/280-rfkill-stubs.patch 
> b/target/linux/generic/patches-4.4/280-rfkill-stubs.patch
> index 8864902..96a98e2 100644
> --- a/target/linux/generic/patches-4.4/280-rfkill-stubs.patch
> +++ b/target/linux/generic/patches-4.4/280-rfkill-stubs.patch
> @@ -1,7 +1,5 @@
> -Index: linux-4.4.21/net/rfkill/Kconfig
> -===
>  linux-4.4.21.orig/net/rfkill/Kconfig   2016-09-15 08:29:29.0 
> +0200
> -+++ linux-4.4.21/net/rfkill/Kconfig2016-09-27 

Re: [LEDE-DEV] lots of broken packages

[Alexandru Ardelean]

On Mon, Oct 31, 2016 at 12:36 AM, Torbjorn Jansson
 wrote:
> On 2016-10-29 20:00, Torbjorn Jansson wrote:
>>
>> Hello
>>
>> looks like there are lots of broken packages currently.
>> was going to install i2c-tools but it looks like it failed with lots of
>> messages like:
>>
>> include/linux/i2c-dev.h:156:21: warning: inlining failed in call to
>> 'i2c_smbus_access': call is unlikely and code size would grow [-Winline]
>>  static inline __s32 i2c_smbus_access(int file, char read_write, __u8
>> command,
>>
>> is this a case of problem already known and i just have to wait a bit
>> and it will get fixed by itself?
>>
>> i'm currently testing something and i need i2c tools.
>> then later i probably need to read up how the building of primarily the
>> kernel is done so i can add a few missing kernel modules.
>>
>
> actually i think this is the interesting bit:
> bash: /bin/python2.7: No such file or directory
>
> and looks like there are more packages failing with similar errors
>
> ___
> Lede-dev mailing list
> Lede-dev@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/lede-dev

My bad.
Will fix.
I don't look over stuff during the weekend much.

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

Re: [LEDE-DEV] Need help with package dependencies

[Alexandru Ardelean]

On Tue, Oct 18, 2016 at 2:09 PM, Zefir Kurtisi
 wrote:
>
> Hi,
>
> to those understanding the package dependency logic by heart, I'm trying to
> achieve something I assumed to be common, but fail to get there with the help 
> of
> the available documentation.
>
> The short version is this:
> * package A has an optional feature X provided by package B
> * package B is optional (feeds package)
>   => feature X should be selectable only if B is installed and selected
> * if feature X is activated, package A depends from package B
>
> I did:
> 1) in Config.in of package A add
> config WITH_FEATURE_X
> bool
> depends on PACKAGE_B
> prompt "Enable feature X"
> default n
>
> 2) in Makefile of package A add
> DEPENDS += +WITH_FEATURE_X:B
Why not just do  DEPENDS+=+PACKAGE_B:B ?
This has the semantic that package A depends on B, only if it was
selected by some other package.

There are other depends like this PACKAGE_x around.

You could then drop WITH_FEATURE_X config option in A

If you need to add any other CFLAGS or LDFLAGS in A, you can just do
ifeq ($(CONFIG_PACKAGE_B),y)
CFLAGS+=
LDFLAGS+=
etc
endif

I hope I understood correctly what you want to do.

>
>
> With those changes, build fails due to recursive dependencies, because
> - symbol PACKAGE_B is selected by WITH_FEATURE_X
> - symbol WITH_FEATURE_X depends on PACKAGE_B
>
> If I leave out the additional line in Makefile of A and in menu select
> WITH_FEATURE_X, the build of A fails with missing dependency to package B.
>
> If I leave out the 'depends on PACKAGE_B' line in Config.in, I can select
> WITH_FEATURE_X without package B even being installed, which obviously fails 
> building.
>
>
> Is this doable out of the box? What am I missing?
>
> BTW, this is about SNMP agentx support for lldpd.
>
>
> Cheers,
> Zefir
>
>
> ___
> Lede-dev mailing list
> Lede-dev@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/lede-dev

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

Re: [LEDE-DEV] [PATCH] ubus cli: wait_for: fix race causing false timeouts

[Alexandru Ardelean]

On Fri, Oct 7, 2016 at 3:09 PM, Felix Fietkau  wrote:
> On 2016-10-07 13:57, Zefir Kurtisi wrote:
>> In ubus_cli_wait_for() there is a critical section between
>> initially checking for the requested services and the
>> following handling of 'ubus.object.add' events.
>>
>> In our system we let procd (re)start services and synchronize
>> inter-service dependencies by using 'ubus wait_for' in the
>> initscripts' service_started() functions. There we observe
>> that 'wait_for' randomly is waiting for the full timeout
>> and returning UBUS_STATUS_TIMEOUT, even if the service it
>> is waiting for is already up and running.
>>
>> This happens when the service is started in the critical
>> section mentioned above. This commit adds periodic lookup
>> for the requested services while waiting for the 'add' event
>> and with that fixes the observed failure.
>>
>> Signed-off-by: Zefir Kurtisi 
> Instead of introducing yet another timer, wouldn't it also be possible
> to close this race window by registering the event handler before
> attempting the lookup?
>
> - Felix
>
> ___
> Lede-dev mailing list
> Lede-dev@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/lede-dev

I've also seen this race.
I tried something like this:
https://github.com/commodo/ubus/commit/8c3986caaa7cd2c12f2b8907ceea54c5bdce3bd2

But never got around to doing much testing to see if the race goes
away completely.
So, I never pushed it upstream.

@Zefir, maybe you could try it ?

Thanks
Alex

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

[LEDE-DEV] [PATCH 1/2] ubox/logd: free regex's on log_shutdown() call

[Alexandru Ardelean]

Signed-off-by: Alexandru Ardelean <ardeleana...@gmail.com>
---
 log/syslog.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/log/syslog.c b/log/syslog.c
index e8b6774..ac4f1ae 100644
--- a/log/syslog.c
+++ b/log/syslog.c
@@ -300,4 +300,6 @@ log_shutdown(void)
close(slog.fd.fd);
close(klog.fd.fd);
free(log);
+   regfree(_prio);
+   regfree(_tstamp);
 }
-- 
1.9.1


___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

[LEDE-DEV] [PATCH 2/2] ubox/logd: free ubus context on exit

[Alexandru Ardelean]

Signed-off-by: Alexandru Ardelean <ardeleana...@gmail.com>
---
 log/logd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/log/logd.c b/log/logd.c
index 58a1dec..0175a5c 100644
--- a/log/logd.c
+++ b/log/logd.c
@@ -226,6 +226,7 @@ main(int argc, char **argv)
uloop_run();
log_shutdown();
uloop_done();
+   ubus_auto_shutdown();
 
return 0;
 }
-- 
1.9.1


___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

Re: [LEDE-DEV] can swconfig turn out the LEDs?

[Alexandru Ardelean]

On Wed, Sep 14, 2016 at 8:51 PM, Dave Taht  wrote:
> recently a whole bunch of posts went around on network world,
> slashdot, and elsewhere, about how to go about disabling or dimming
> the leds.
>
> http://www.networkworld.com/article/3119881/mobile-wireless/the-scourge-of-leds-everywhere-readers-speak-out.html
>
> In my own post I'd found a way to nuke many lights, but not all. In
> particular, is there a way to tell swconfig to turn out the light
> entirely?
>
>
> --
> Dave Täht
> Let's go make home routers and wifi faster! With better software!
> http://blog.cerowrt.org
>
> ___
> Lede-dev mailing list
> Lede-dev@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/lede-dev

Would be interesting to try to implement.
Disabling LEDs (entirely) looks pretty driver specific.

The rtl8366 (variants), rtl8367 and ar8327 seem to implement in the
driver, the blinkrate controls.
For example, on the b53 driver there is no control for LEDs
whatsoever, though technically the chip may support it.

Looking a bit into the driver (referenced in your blog):
https://github.com/lede-project/source/blob/master/target/linux/generic/files/drivers/net/phy/rtl8366s.c#L901

It looks like there is some per-port control of the led mode.
Though, I haven't found anything detailing those modes.

So you could try out a few variants with
swconfig dev rtl8366s port X set led Y
(or something like that)

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

Re: [LEDE-DEV] [UBUS] [PATCH] Issue of msgbuf_data_len in ubus_context

[Alexandru Ardelean]

On Sun, Jun 19, 2016 at 7:21 AM, Wang Linetkux  wrote:
> Sorry, forgot to paste the patch in the email for easy reading.
>
> ---
>
>  libubus-io.c | 1 +
>
>  1 file changed, 1 insertion(+)
>
>
>
> diff --git a/libubus-io.c b/libubus-io.c
>
> index 9320bf3..0582ff7 100644
>
> --- a/libubus-io.c
>
> +++ b/libubus-io.c
>
> @@ -259,6 +259,7 @@ static bool alloc_msg_buf(struct ubus_context
> *ctx, int len)
>
> return false;
>
>
>
> ctx->msgbuf.data = ptr;
>
> +   ctx->msgbuf_data_len = len;
>
> return true;
>
>  }
>
>
>
>
>
>
>
> 2016-06-19 12:16 GMT+08:00 Wang Linetkux :
>> Hi, Guys
>>   Since I cannot find other places to send this patch, so I send it
>> here, and hope it does not bother you.
>>   "alloc_msg_buf" function in "ubus/libubus-io.c" has the feature that
>> expand or shrink msg buffer when necessary. But I have noticed that
>> "msgbuf_data_len" has been properly set after msg buffer is expanded,
>> and it still stays with its origin value. I have generated one patch
>> for this bug. Feel free to talk with me, just in case I am wrong.
>>
>> ubus repo on lede:
>> https://git.lede-project.org/?p=project/ubus.git;a=summary
>>
>>
>> Rujun
>
> ___
> Lede-dev mailing list
> Lede-dev@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/lede-dev

@Felix or @John: can you please check this patch ?
Wang has a point here.
"ctx->msgbuf_data_len" is not adjusted after init, and should be, in
case of realloc.

I don't think this causes a leak.
It just makes the buffer size reduction logic useless.
So, if the max ubus msg is 1 MB, then the buffer size will remain 1 MB
until reboot/restart of the process.

@Wang: the way to send patches is via the git send-email command.
so in this case:
git send-email --to=lede-...@lists.infraread.org
0001-Fix-the-length-of-msg-buffer-after-realloc.patch

Thanks
Alex

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

Re: [LEDE-DEV] [PATCH 1/3] [ubox] remove unnecessary size struct between messages

[Alexandru Ardelean]

On Fri, Jun 3, 2016 at 2:27 PM, Conor O'Gorman  wrote:
> On 03/06/16 11:59, Dan Bugnar wrote:
>>
>> From: Dan Bugnar 
>>
>> The next message needs to be written after the data of current message.
>> This was adding "sizeof(struct log_head)" bytes between messages.
>>
>> Signed-off-by: Dan Bugnar 
>> ---
>>   log/syslog.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/log/syslog.c b/log/syslog.c
>> index e8b6774..2042e93 100644
>> --- a/log/syslog.c
>> +++ b/log/syslog.c
>> @@ -51,7 +51,7 @@ static regex_t pat_tstamp;
>>   static struct log_head*
>>   log_next(struct log_head *h, int size)
>>   {
>> -   struct log_head *n = (struct log_head *)
>> >data[PAD(sizeof(struct log_head) + size)];
>> +   struct log_head *n = (struct log_head *) >data[size];
>>
> You lost the PAD? Which is over the struct and size, not just the struct.
>
> Conor
>
>
> ___
> Lede-dev mailing list
> Lede-dev@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/lede-dev

hmm, what about moving the PAD() somewhere in the log_add() function ?
it would save some cycles, since, that's the only place where the size
would need to be adjusted for padding/alignment;
doing it in log_next(), can be a bit CPU expensive

as for the comment "Which is over the struct and size, not just the struct."
i am bit unclear;
apologies if i'll be too verbose (below)

this change is good (padding aside)
previously, the log_next() function would jump over "sizeof(struct
log_head) + PAD(sizeof(struct log_head) + size)"
the "data" field in the struct log_head is at the end of the struct
(offset sizeof(struct log_head) )
there should be no need to also jump "sizeof(struct log_head)"  in the
"data" field
basically, it would punch "sizeof(struct log_head)" holes (i think 20
bytes each) in the log buffer, between entries

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

[LEDE-DEV] Fwd: [PATCH 2/2] [ubox] logd: add ubus reload method

[Alexandru Ardelean]

Hmm, very weird.
My settings for GMail are to send email in Plain Text Mode and I still
got email delivery failure to lede-dev (for this message).

===
The error that the other server returned was:
550-Mailing lists do not accept HTML mail. See
550 http://david.woodhou.se/email.html
===

-- Forwarded message --
From: Alexandru Ardelean <ardeleana...@gmail.com>
Date: Sat, May 14, 2016 at 11:05 AM
Subject: Re: [LEDE-DEV] [PATCH 2/2] [ubox] logd: add ubus reload method
To: David Lang <da...@lang.hm>
Cc: Helmut Schaa <helmut.sc...@googlemail.com>,
lede-dev@lists.infradead.org, Bastian Bittorf <bitt...@bluebottle.com>


On Fri, May 13, 2016 at 11:40 PM, David Lang <da...@lang.hm> wrote:
>
> On Fri, 13 May 2016, Alexandru Ardelean wrote:
>
>> On Fri, May 13, 2016 at 8:55 PM, David Lang <da...@lang.hm> wrote:
>>>
>>> On Fri, 13 May 2016, Helmut Schaa wrote:
>>>>
>>>> I was thinking of a different use-case:
>>>> Increasing the buffer size on the fly comes in handy during a debug
>>>> session
>>>> where you'd like to not interrupt logging (and thus potentially lose some
>>>> parts
>>>> of the syslog when restarting logd).
>>>>
>>>> Independent of how the implementation looks like I think the functionality
>>>> would be quite useful.
>>>
>>>
>>>
>>> I don't think it's very valuable. If you are debugging, you really don't
>>> want to be tweaking anything in the middle of trying to capture data. you
>>> want to set everything up and let it run, then analyze the data.
>>>
>>> I don't see that changing the log size in the middle of a capture run is a
>>> good idea, let alone one that is common enough to have to introduce uci
>>> specific knowledge into the logd daemon.
>>>
>>> You are better off sending to a remote logserver anyway.
>>>
>>> David Lang
>>>
>>>
>>> ___
>>> Lede-dev mailing list
>>> Lede-dev@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/lede-dev
>>
>>
>> First of all, let's agree on a few things:
>> 1) the patch " [ubox] syslog: use realloc to change log buffer size ",
>> which precedes this, is an improvement over the existing code in logd
>> ; the initial code of logd, includes a logic to dynamically increase
>> the log buffer (using malloc() + memcpy()) ; there are 2 logical
>> options regarding that code:
>>   1.1) make it work (as that patch does)
>>   1.2) remove it
>> 2) there are people that don't need this ability to dynamically
>> increase the log buffer ; we do need it, but are not blocked by not
>> having it ; it would be neat to have in upstream ubox/logd, given that
>> logd was initially written with this ability partially intended;
>>
>> I don't know if this pushback is also amplified by my snappy reply to KarlP.
>> If it is, well, c'est la vie :) .  I lost an argument because of a
>> snappy come-back that upset some people. Wouldn't be the first time.
>>
>> I feel that this change [to dynamically increase the log-size] can be
>> achieved with minimal impact on code/binary size and logd behavior
>> (given point 1) ).
>> Normal operation should not be affected (or the patchset can be
>> adapted to less affect normal operation).
>> And then, if it's in, people can choose to use it or not.
>> Or, if it's too intrusive/bothersome, we can drop the patchset altogether.
>>
>> I'm still unclear yet how patch submission works in LEDE, and how
>> patches are accepted/voted, or who has the final go.
>> At least this process can shed some on light on that (for us).
>
>
> People don't object to the ability to resize the buffer if the code impact is 
> small, but when you start saying that you want to have logd understand/parse 
> UCI in the binary (as opposed to the script that starts the binary), the code 
> impact is not that small any longer.
>
> The use case isn't non-existant, but it is marginal.
>
> David Lang


@Felix, so then to conclude/converge. We keep the -S cli param, and
add a "size" ubus method that can return the current log size (if no
arg specified), and if an arg is specified, then update the log size ?
Something like:
- to read: ubus call log size   ==> output: {  size: 1024 }
- to write: ubus call log size  '{  "size" : 2048  }'  ==>  output: {
size: 2048 }

This would work fine for us as well.
I'm open to other m

Re: [LEDE-DEV] [PATCH 1/2] [ubox] syslog: use realloc to change log buffer size

[Alexandru Ardelean]

On Thu, May 12, 2016 at 3:09 PM, Dan Bugnar  wrote:
> From: Dan Bugnar 
>
> Change the log buffer size and copy the messages.
>
> Signed-off-by: Dan Bugnar 
> ---
>  log/syslog.c | 37 +++--
>  log/syslog.h |  2 +-
>  2 files changed, 16 insertions(+), 23 deletions(-)
>
> diff --git a/log/syslog.c b/log/syslog.c
> index e8b6774..d6cb868 100644
> --- a/log/syslog.c
> +++ b/log/syslog.c
> @@ -42,7 +42,7 @@
>  #define PAD(x) (x % 4) ? (((x) - (x % 4)) + 4) : (x)
>
>  static char *log_dev = LOG_DEFAULT_SOCKET;
> -static int log_size = LOG_DEFAULT_SIZE;
> +static int log_size = 0;
>  static struct log_head *log, *log_end, *oldest, *newest;
>  static int current_id = 0;
>  static regex_t pat_prio;
> @@ -237,34 +237,30 @@ log_list(int count, struct log_head *h)
>  }
>
>  int
> -log_buffer_init(int size)
> +log_buffer_reinit(int size)
>  {
> -   struct log_head *_log = malloc(size);
> +   if (size <= 0)
> +   size = LOG_DEFAULT_SIZE;
> +   if (log_size == size)
> +   return 0;
> +
> +   struct log_head *_log = realloc(log, size);
>
> if (!_log) {
> +   oldest = newest = log = NULL;
> fprintf(stderr, "Failed to initialize log buffer with size 
> %d\n", log_size);
> return -1;
> }
>
> -   memset(_log, 0, size);
> -
> if (log && ((log_size + sizeof(struct log_head)) < size)) {
> -   struct log_head *start = _log;
> -   struct log_head *end = ((void*) _log) + size;
> -   struct log_head *l;
> -
> -   l = log_list(0, NULL);
> -   while ((start < end) && l && l->size) {
> -   memcpy(start, l, PAD(sizeof(struct log_head) + 
> l->size));
> -   start = (struct log_head *) >data[PAD(l->size)];
> -   l = log_list(0, l);
> -   }
> -   free(log);
> -   newest = start;
> +   newest = (_log + (newest - log));
> newest->size = 0;
> -   oldest = log = _log;
> +   memset(newest, 0, size - log_size);
> +   oldest = (_log + (oldest - log));
> +   log = _log;
> log_end = ((void*) log) + size;
> } else {
> +   memset(_log, 0, size);
> oldest = newest = log = _log;
> log_end = ((void*) log) + size;
> }
> @@ -276,13 +272,10 @@ log_buffer_init(int size)
>  void
>  log_init(int _log_size)
>  {
> -   if (_log_size > 0)
> -   log_size = _log_size;
> -
> regcomp(_prio, "^<([0-9]*)>(.*)", REG_EXTENDED);
> regcomp(_tstamp, "^\[[ 0]*([0-9]*).([0-9]*)] (.*)", REG_EXTENDED);
>
> -   if (log_buffer_init(log_size)) {
> +   if (log_buffer_reinit(_log_size)) {
> fprintf(stderr, "Failed to allocate log memory\n");
> exit(-1);
> }
> diff --git a/log/syslog.h b/log/syslog.h
> index 81a039f..ed5a41b 100644
> --- a/log/syslog.h
> +++ b/log/syslog.h
> @@ -35,7 +35,7 @@ void log_shutdown(void);
>
>  typedef void (*log_list_cb)(struct log_head *h);
>  struct log_head* log_list(int count, struct log_head *h);
> -int log_buffer_init(int size);
> +int log_buffer_reinit(int size);
>  void log_add(char *buf, int size, int source);
>  void ubus_notify_log(struct log_head *l);
>
> --
> 2.8.1
>
>
> ___
> Lede-dev mailing list
> Lede-dev@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/lede-dev

So, then, let's move the discussion to this patch, about dynamically
increasing the log size.
For reference, this is the initial code (that tried to do dynamic log
size increase) and the change that actually does it.

Like I said on the other thread.
There are 2 approaches on this initial (malloc() + memcpy() code):
1) make it work
2) remove it

The issue with this code, is that it allocs a new buffer (for the
log), but looks like it tries to copy over the initial log buffer.
And I think the stop condition (of the while loop) is not completely
correct. I think it tries to also write after/outside the initial
buffer somewhere.

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

Re: [LEDE-DEV] [PATCH 2/2] [ubox] logd: add ubus reload method

[Alexandru Ardelean]

On Fri, May 13, 2016 at 8:55 PM, David Lang <da...@lang.hm> wrote:
> On Fri, 13 May 2016, Helmut Schaa wrote:
>
>> On Fri, May 13, 2016 at 12:03 PM, Bastian Bittorf
>> <bitt...@bluebottle.com> wrote:
>>>
>>> * Conor O'Gorman <i...@conorogorman.net> [13.05.2016 11:52]:
>>>>
>>>> On 13/05/16 07:23, Alexandru Ardelean wrote:
>>>>>
>>>>> Short version is: we have a configuration management system on top of
>>>>> OpenWrt ; system boots with default settings (on every boot), and
>>>>> config management applies config changes (whether it's right after
>>>>> boot, or during system's normal operation).
>>>>> Maybe other people do something similar.
>>>>
>>>> Not many. Most people store the config, which is the current model.
>>>> It has benefits.
>>>
>>>
>>> We also have an approach like Alexandru, but i dont see
>>> the problem: logd starts at START='12'. Our config-thingy
>>> fires at START=01 and uci-sets all the stuffs. So we can still
>>> "rotate the knobs" e.g. uci set system.@system[0].log_size=64
>>
>>
>> I was thinking of a different use-case:
>> Increasing the buffer size on the fly comes in handy during a debug
>> session
>> where you'd like to not interrupt logging (and thus potentially lose some
>> parts
>> of the syslog when restarting logd).
>>
>> Independent of how the implementation looks like I think the functionality
>> would be quite useful.
>
>
> I don't think it's very valuable. If you are debugging, you really don't
> want to be tweaking anything in the middle of trying to capture data. you
> want to set everything up and let it run, then analyze the data.
>
> I don't see that changing the log size in the middle of a capture run is a
> good idea, let alone one that is common enough to have to introduce uci
> specific knowledge into the logd daemon.
>
> You are better off sending to a remote logserver anyway.
>
> David Lang
>
>
> ___
> Lede-dev mailing list
> Lede-dev@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/lede-dev

First of all, let's agree on a few things:
1) the patch " [ubox] syslog: use realloc to change log buffer size ",
which precedes this, is an improvement over the existing code in logd
; the initial code of logd, includes a logic to dynamically increase
the log buffer (using malloc() + memcpy()) ; there are 2 logical
options regarding that code:
   1.1) make it work (as that patch does)
   1.2) remove it
2) there are people that don't need this ability to dynamically
increase the log buffer ; we do need it, but are not blocked by not
having it ; it would be neat to have in upstream ubox/logd, given that
logd was initially written with this ability partially intended;

I don't know if this pushback is also amplified by my snappy reply to KarlP.
If it is, well, c'est la vie :) .  I lost an argument because of a
snappy come-back that upset some people. Wouldn't be the first time.

I feel that this change [to dynamically increase the log-size] can be
achieved with minimal impact on code/binary size and logd behavior
(given point 1) ).
Normal operation should not be affected (or the patchset can be
adapted to less affect normal operation).
And then, if it's in, people can choose to use it or not.
Or, if it's too intrusive/bothersome, we can drop the patchset altogether.

I'm still unclear yet how patch submission works in LEDE, and how
patches are accepted/voted, or who has the final go.
At least this process can shed some on light on that (for us).

Thanks
Alex

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

Re: [LEDE-DEV] [PATCH 2/2] [ubox] logd: add ubus reload method

[Alexandru Ardelean]

On Fri, May 13, 2016 at 12:14 AM, Karl Palsson <ka...@tweak.net.au> wrote:
>
> Alexandru Ardelean <ardeleana...@gmail.com> wrote:
>>
>> Changing the log size dynamically at runtime is a use case that
>> we have.
>
> I'm sorry, but can you talk to me like I'm 5? What is this use
> case? I mean, I can understand it synthetically, like, "If I want
> to resize the log dynamically, I need to be able to reload the
> log config" But I'm simply failing to comprehend what the use
> case is for needing to resize the log dynamically.
>
> Cheers,
> Karl P

It almost feels like we're getting down a slippery slope towards a
never-ending "why ?" set of questions (like with a 5 year old) :)

Short version is: we have a configuration management system on top of
OpenWrt ; system boots with default settings (on every boot), and
config management applies config changes (whether it's right after
boot, or during system's normal operation).
Maybe other people do something similar.
But, I'd like to stop with the use-case stuff here, since it's getting
close to the business logic.

Coming back to the patches. Is it a problem with:
1) The UCI C changes ?
2) The dynamic log resize ?
3) The whole thing ?

Depending on what the problem is, we can adapt the patchset.
Or simply drop the whole thing if it's deemed un-needed by the community.
In the end, it boils down to a use-case that we do (as we do it)
because we want to do it (as we do it), and we've tried to find an
upstream-able approach.

Thanks :)
Alex

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

[LEDE-DEV] Fwd: [PATCH 2/2] [ubox] logd: add ubus reload method

[Alexandru Ardelean]

On Thu, May 12, 2016 at 5:30 PM, Karl Palsson  wrote:
>
>
> Dan Bugnar  wrote:
> > From: Dan Bugnar 
> >
> > Add logd link to uci library, to read the system config file
> > and get the buffer size. Remove the -S option support and use
> > just the option from the config file.
>
>
> Why do you need this exactly? It's already being parsed from the
> config file by the init script, and that is much less code than
> adding all of uci parsing into logd?
>
> Cheers,
> Karl P
> ___
> Lede-dev mailing list
> Lede-dev@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/lede-dev
>

[ Forwarding to lede-dev too ; my GMail complained about HTML not
being allowed ]

Doing a reload on logd (to increase the buffer size), should not clear
out the previous log info.
By default logd's reload, is a logd restart.
The end game is to do a proper reload, and keep whatever info was
logged, before the reload (if doing a log buffer increase).

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev