Re: [lwip-devel] [PATCHv5 05/13] net/lwip: implement tftp cmd

[Simon Goldschmidt]

On 10.08.2023 13:28, Maxim Uvarov wrote:
> On Thu, 3 Aug 2023 at 12:42, Ilias Apalodimas <ilias.apalodimas@linaro.org>
> wrote:
>
> > On Wed, Aug 02, 2023 at 08:06:50PM +0600, Maxim Uvarov wrote:
> > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
> > > ---
> > >   lib/lwip/Makefile              |   1 +
> > >   lib/lwip/apps/tftp/Makefile    |  16 +++++
> > >   lib/lwip/apps/tftp/lwip-tftp.c | 124 +++++++++++++++++++++++++++++++++
> > >   3 files changed, 141 insertions(+)
> > >   create mode 100644 lib/lwip/apps/tftp/Makefile
> > >   create mode 100644 lib/lwip/apps/tftp/lwip-tftp.c
> > >
> > > diff --git a/lib/lwip/Makefile b/lib/lwip/Makefile
> > > index a3521a9d18..1893162c1a 100644
> > > --- a/lib/lwip/Makefile
> > > +++ b/lib/lwip/Makefile
> > > @@ -67,3 +67,4 @@ obj-$(CONFIG_NET) += port/sys-arch.o
> > >
> > >   obj-$(CONFIG_CMD_DHCP) += apps/dhcp/lwip-dhcp.o
> > >   obj-$(CONFIG_CMD_DNS) += apps/dns/lwip-dns.o
> > > +obj-$(CONFIG_CMD_TFTPBOOT) += apps/tftp/
> > > diff --git a/lib/lwip/apps/tftp/Makefile b/lib/lwip/apps/tftp/Makefile
> > > new file mode 100644
> > > index 0000000000..299c95e9c0
> > > --- /dev/null
> > > +++ b/lib/lwip/apps/tftp/Makefile
> > > @@ -0,0 +1,16 @@
> > > +
> > > +ccflags-y += -I$(srctree)/lib/lwip/port/include
> > > +ccflags-y += -I$(srctree)/lib/lwip/lwip-external/src/include
> > -I$(srctree)/lib/lwip
> > > +ccflags-y += -I$(obj)
> > > +
> > > +$(obj)/tftp.o: $(obj)/tftp.c
> > > +.PHONY: $(obj)/tftp.c
> > > +$(obj)/tftp.c:
> > > +     cp $(srctree)/lib/lwip/lwip-external/src/apps/tftp/tftp.c
> > $(obj)/tftp.c
> > > +     cp
> > $(srctree)/lib/lwip/lwip-external/src/include/lwip/apps/tftp_client.h
> > $(obj)/tftp_client.h
> > > +     cp
> > $(srctree)/lib/lwip/lwip-external/src/include/lwip/apps/tftp_common.h
> > $(obj)/tftp_common.h
> > > +     cp
> > $(srctree)/lib/lwip/lwip-external/contrib/examples/tftp/tftp_example.h
> > $(obj)/tftp_example.h
> > > +
> > > +obj-$(CONFIG_CMD_TFTPBOOT) += tftp.o
> > > +obj-$(CONFIG_CMD_TFTPBOOT) += lwip-tftp.o
> > > +
> > > diff --git a/lib/lwip/apps/tftp/lwip-tftp.c
> > b/lib/lwip/apps/tftp/lwip-tftp.c
> > > new file mode 100644
> > > index 0000000000..511d82e600
> > > --- /dev/null
> > > +++ b/lib/lwip/apps/tftp/lwip-tftp.c
> > > @@ -0,0 +1,124 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +
> > > +/*
> > > + * (C) Copyright 2023 Linaro Ltd. <maxim.uvarov@linaro.org>
> > > + */
> > > +
> > > +#include <common.h>
> > > +#include <command.h>
> > > +#include <console.h>
> > > +
> > > +#include "lwip/apps/tftp_client.h"
> > > +#include "lwip/apps/tftp_server.h"
> > > +#include <tftp_example.h>
> > > +
> > > +#include <string.h>
> > > +
> > > +#include "../../../lwip/ulwip.h"
> > > +
> > > +#if LWIP_UDP
> > > +
> > > +static ulong daddr;
> > > +static ulong size;
> > > +
> > > +static void *tftp_open(const char *fname, const char *mode, u8_t
> > is_write)
> > > +{
> > > +     LWIP_UNUSED_ARG(mode);
> > > +     return NULL;
> > > +}
> > > +
> > > +static void tftp_close(void *handle)
> > > +{
> > > +     printf("\ndone\n");
> > > +     printf("Bytes transferred = %ld (0x%lx hex)\n", size, size);
> >
> > Can you replace all the printf occurences with log_XXXX ?
> >
> > > +
> > > +     env_set_ulong("filesize", size);
> > > +     ulwip_exit(0);
> > > +}
> > > +
> > > +static int tftp_read(void *handle, void *buf, int bytes)
> > > +{
> > > +     return 0;
> > > +}
> > > +
> > > +static int tftp_write(void *handle, struct pbuf *p)
> > > +{
> > > +     struct pbuf *q;
> > > +
> > > +     for (q = p; q != NULL; q = q->next) {
> > > +             memcpy((void *)daddr, q->payload, q->len);
> > > +             /* printf("downloaded chunk size %d, to addr 0x%lx\n",
> > q->len, daddr); */
> >
> > Remove all those debug comments on  the next version
> >
> > > +             daddr += q->len;
> > > +             size += q->len;
> > > +             printf("#");
> > > +     }
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +/* For TFTP client only */
> > > +static void tftp_error(void *handle, int err, const char *msg, int size)
> > > +{
> > > +     char message[100];
> > > +
> > > +     LWIP_UNUSED_ARG(handle);
> > > +
> > > +     memset(message, 0, sizeof(message));
> > > +     MEMCPY(message, msg, LWIP_MIN(sizeof(message)-1, (size_t)size));
> >
> > Why aren't we using the native memcpy?
> >
> > > +
> > > +     printf("TFTP error: %d (%s)", err, message);
> > > +}
> > > +
> > > +static const struct tftp_context tftp = {
> > > +     tftp_open,
> > > +     tftp_close,
> > > +     tftp_read,
> > > +     tftp_write,
> > > +     tftp_error
> > > +};
> > > +
> > > +int lwip_tftp(ulong addr, char *fname)
> > > +{
> > > +     void *f = (void *)0x1; /*fake handle*/
> >
> > If it's fake can't it just be NULL? What does 'fake' mean? is that safe to
> > be passed around the LWIP APIs?
> Here I reuse a tftp example from lwIP. f here is a file handle to write
> output. I do write to memory and do not use this value.
> But there is a check that his value can not be NULL. So something needs to
> be passed here to not do modifications to
> the example code.

Sorry I did not have the time to follow all your patches here throgouhly
enough. However, if things like this come up, please also do not
hesitate to come to us (lwip developers) with ideas to make our code
more easily integratable into target applications (like U-Boot here).

As I said before (to Maxim in a private mail I think), I think it would
be best to start targeting this integration based on lwIP's master
branch, so modifications in the upstream lwIP sources would be possible
to make this work neatly.

Regards,
Simon

> > > +     err_t err;
> > > +     ip_addr_t srv;
> > > +     int ret;
> > > +     char *server_ip;
> > > +
> > > +     if (!fname || addr == 0)
> > > +             return CMD_RET_FAILURE;
> > > +
> > > +     size = 0;
> > > +     daddr = addr;
> > > +     server_ip = env_get("serverip");
> > > +     if (!server_ip) {
> > > +             printf("error: serverip variable has to be set\n");
> > > +             return CMD_RET_FAILURE;
> > > +     }
> > > +
> > > +     ret = ipaddr_aton(server_ip, &srv);
> > > +     LWIP_ASSERT("ipaddr_aton failed", ret == 1);
> > > +
> > > +     printf("TFTP from server %s; our IP address is %s\n",
> > > +                     server_ip, env_get("ipaddr"));
> > > +     printf("Filename '%s'.\n", fname);
> > > +     printf("Load address: 0x%lx\n", daddr);
> > > +     printf("Loading:");
> > > +
> > > +     err = tftp_init_client(&tftp);
> > > +     if (!(err == ERR_OK || err == ERR_USE))
> > > +             printf("tftp_init_client err: %d\n", err);
> > > +
> > > +     err = tftp_get(f, &srv, TFTP_PORT, fname, TFTP_MODE_OCTET);
> > > +     /* might return different errors, like routing problems */
> > > +     if (err != ERR_OK) {
> > > +             printf("tftp_get err=%d\n", err);
> > > +     }
> > > +     LWIP_ASSERT("tftp_get failed", err == ERR_OK);
> > > +
> > > +     env_set_hex("fileaddr", addr);
> > > +     return err;
> > > +}
> > > +#else
> > > +#error "UDP has to be supported"
> > > +#endif /* LWIP_UDP */
> > > --
> > > 2.30.2