Hi Joel,
Thanks for the feedback, I'll tidy up the things you've mentioned, then
move onto the master patchset.
I moved the #include "lwip/sys.h" from sockets.c to sockets.h in the
submitted patchset, so it still effectively gets included. Would you
prefer it to still be in sockets.c as well to make it look more obvious?
On 11/09/17 17:26, Joel Cunningham wrote:
> Follow-up Comment #1, bug #51990 (project lwip):
>
> David,
>
> I did an initial code review, things looked good. Only a couple really minors
> things:
>
>
> --- a/src/api/sockets.c
> +++ b/src/api/sockets.c
> @@ -50,7 +50,6 @@
>
> #include "lwip/sockets.h"
> #include "lwip/api.h"
> -#include "lwip/sys.h"
> #include "lwip/igmp.h"
> #include "lwip/inet.h"
> #include "lwip/tcp.h"
>
>
> We'll want to keep lwip/sys.h as there is other code in sockets.c using things
> in the header
>
>
> @@ -77,6 +76,13 @@
> #define LWIP_NETCONN 0
> #endif
>
> +#define API_SELECT_CB_VAR_REF(name) API_VAR_REF(name)
> +#define API_SELECT_CB_VAR_DECLARE(name) API_VAR_DECLARE(struct
> lwip_select_cb, name)
> +#define API_SELECT_CB_VAR_ALLOC(name) API_VAR_ALLOC(struct
> lwip_select_cb, MEMP_SELECT_CB, name, ERR_MEM)
> +#define API_SELECT_CB_VAR_ALLOC_RETURN_NULL(name) API_VAR_ALLOC(struct
> lwip_select_cb, MEMP_SELECT_CB, name, NULL)
> +#define API_SELECT_CB_VAR_FREE(name)
> API_VAR_FREE(MEMP_SELECT_CB, name)
> +
> +
>
>
> Extra newline here
>
>
> /**
> + * MEMP_NUM_SELECT_CB: the number of struct lwip_select_cb.
> + * (only needed if you use the sequential API, like api_lib.c)
> + */
> +#if !defined MEMP_NUM_SELECT_CB || defined __DOXYGEN__
> +#define MEMP_NUM_SELECT_CB 4
> +#endif
> +
> +/**
>
>
> I think for the comment on this option, we'll want to mention 'only needed if
> you use the sockets API', since select is not available when only using
> netconn API
>
> Couple notes when porting this fix to master:
> 1) We'll want to put struct lwip_select_cb and SELECT_ macros in
> sockets_priv.h rather than public API
> 2) MEMP_NUM_SELECT_CB memory pool will need to be guarded by #if
> LWIP_SOCKET_SELECT
>
>
> _______________________________________________________
>
> Reply to this item at:
>
> <
http://savannah.nongnu.org/bugs/?51990>
>
> _______________________________________________
> Message sent via/by Savannah
>
http://savannah.nongnu.org/
>
>
> ______________________________________________________________________
> This email has been scanned by the Symantec Email Security.cloud service.
> For more information please visit
http://www.symanteccloud.com
> ______________________________________________________________________
>
>
______________________________________________________________________
This email has been scanned by the Symantec Email Security.cloud service.
For more information please visit
http://www.symanteccloud.com
______________________________________________________________________
_______________________________________________
lwip-devel mailing list
address@hidden
https://lists.nongnu.org/mailman/listinfo/lwip-devel