diff mbox series

[PATCHv8,05/15] net/lwip: implement tftp cmd

Message ID 20230908135320.7066-6-maxim.uvarov@linaro.org
State Superseded
Headers show
Series net/lwip: add lwip library for the network stack | expand

Commit Message

Maxim Uvarov Sept. 8, 2023, 1:53 p.m. UTC 
U-Boot recently got support for an alternative network stack using LWIP.
Replace tftp command with the LWIP variant while keeping the output and
error messages identical.

Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
---
 include/net/lwip.h             |  14 +++-
 net/lwip/Makefile              |   1 +
 net/lwip/apps/tftp/Makefile    |   7 ++
 net/lwip/apps/tftp/lwip-tftp.c | 129 +++++++++++++++++++++++++++++++++
 4 files changed, 150 insertions(+), 1 deletion(-)
 create mode 100644 net/lwip/apps/tftp/Makefile
 create mode 100644 net/lwip/apps/tftp/lwip-tftp.c

Comments

Ilias Apalodimas Sept. 13, 2023, 6:15 a.m. UTC | #1 
> +
> +/*
> + * (C) Copyright 2023 Linaro Ltd. <maxim.uvarov@linaro.org>
> + */
> +
> +#include <common.h>
> +#include <command.h>
> +#include <console.h>
> +#include <bootstage.h>
> +
> +#include "tftp_client.h"
> +#include "tftp_server.h"
> +#include <tftp_example.h>
> +
> +#include <string.h>
> +
> +#include <net/ulwip.h>
> +
> +static ulong daddr;
> +static ulong size;
> +
> +static void *tftp_open(const char *fname, const char *mode, u8_t is_write)
> +{
> +	return NULL;
> +}
> +
> +static void tftp_close(void *handle)
> +{
> +	log_info("\ndone\n");
> +	log_info("Bytes transferred = %ld (0x%lx hex)\n", size, size);
> +
> +	bootstage_mark_name(BOOTSTAGE_KERNELREAD_STOP, "tftp_done");
> +	if (env_set_hex("filesize", size)) {
> +		log_err("filesize not updated\n");
> +		ulwip_exit(-1);
> +		return;
> +	}
> +	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);
> +		daddr += q->len;
> +		size += q->len;
> +		log_info("#");
> +	}
> +
> +	return 0;
> +}
> +
> +static void tftp_error(void *handle, int err, const char *msg, int size)
> +{
> +	char message[100];
> +
> +	memset(message, 0, sizeof(message));
> +	memcpy(message, msg, LWIP_MIN(sizeof(message) - 1, (size_t)size));
> +
> +	log_info("TFTP error: %d (%s)", err, message);
> +}
> +
> +static const struct tftp_context tftp = {
> +	tftp_open,
> +	tftp_close,
> +	tftp_read,
> +	tftp_write,
> +	tftp_error
> +};
> +
> +int ulwip_tftp(ulong addr, char *fname)
> +{
> +	void *f = (void *)0x1; /* unused fake file handle*/

I haven't dug into lwip details yet, but I am not sure this is safe to use.
Simon, since you maintain the lwip part can you elaborate a bit more?

> +	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) {
> +		log_err("error: serverip variable has to be set\n");
> +		return CMD_RET_FAILURE;
> +	}
> +
> +	ret = ipaddr_aton(server_ip, &srv);
> +	if (!ret) {
> +		log_err("error: ipaddr_aton\n");
> +		return CMD_RET_FAILURE;
> +	}
> +
> +	log_info("TFTP from server %s; our IP address is %s\n",
> +		 server_ip, env_get("ipaddr"));
> +	log_info("Filename '%s'.\n", fname);
> +	log_info("Load address: 0x%lx\n", daddr);
> +	log_info("Loading:");
> +
> +	bootstage_mark_name(BOOTSTAGE_KERNELREAD_START, "tftp_start");
> +
> +	err = tftp_init_client(&tftp);
> +	if (!(err == ERR_OK || err == ERR_USE))
> +		log_err("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) {
> +		log_err("tftp_get err=%d\n", err);
> +		return CMD_RET_FAILURE;
> +	}
> +
> +	if (env_set_hex("fileaddr", addr)) {
> +		log_err("fileaddr not updated\n");
> +		return CMD_RET_FAILURE;
> +	}
> +
> +	return CMD_RET_SUCCESS;
> +}
> --
> 2.30.2
>

Thanks
/Ilias
Simon Goldschmidt Sept. 13, 2023, 8:38 a.m. UTC | #2 
On 13.09.2023 08:15, Ilias Apalodimas wrote:
>> +
>> +/*
>> + * (C) Copyright 2023 Linaro Ltd. <maxim.uvarov@linaro.org>
>> + */
>> +
>> +#include <common.h>
>> +#include <command.h>
>> +#include <console.h>
>> +#include <bootstage.h>
>> +
>> +#include "tftp_client.h"
>> +#include "tftp_server.h"
>> +#include <tftp_example.h>
>> +
>> +#include <string.h>
>> +
>> +#include <net/ulwip.h>
>> +
>> +static ulong daddr;
>> +static ulong size;
>> +
>> +static void *tftp_open(const char *fname, const char *mode, u8_t is_write)
>> +{
>> +	return NULL;
>> +}
>> +
>> +static void tftp_close(void *handle)
>> +{
>> +	log_info("\ndone\n");
>> +	log_info("Bytes transferred = %ld (0x%lx hex)\n", size, size);
>> +
>> +	bootstage_mark_name(BOOTSTAGE_KERNELREAD_STOP, "tftp_done");
>> +	if (env_set_hex("filesize", size)) {
>> +		log_err("filesize not updated\n");
>> +		ulwip_exit(-1);
>> +		return;
>> +	}
>> +	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);
>> +		daddr += q->len;
>> +		size += q->len;
>> +		log_info("#");
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void tftp_error(void *handle, int err, const char *msg, int size)
>> +{
>> +	char message[100];
>> +
>> +	memset(message, 0, sizeof(message));
>> +	memcpy(message, msg, LWIP_MIN(sizeof(message) - 1, (size_t)size));
>> +
>> +	log_info("TFTP error: %d (%s)", err, message);
>> +}
>> +
>> +static const struct tftp_context tftp = {
>> +	tftp_open,
>> +	tftp_close,
>> +	tftp_read,
>> +	tftp_write,
>> +	tftp_error
>> +};
>> +
>> +int ulwip_tftp(ulong addr, char *fname)
>> +{
>> +	void *f = (void *)0x1; /* unused fake file handle*/
>
> I haven't dug into lwip details yet, but I am not sure this is safe to use.
> Simon, since you maintain the lwip part can you elaborate a bit more?

 From the lwIP design, using an invalid pointer here is ok: that pointer
is only used as a client handle which is never dereferenced internally.
The only requirement is that it is not NULL, as that is the validity
check for the tftp client to know the handle is valid (or e.g. open
failed etc.).

So while we can leave 0x1 here, using a static uint8_t would probably be
better, at the expense of using a byte for nothing.

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) {
>> +		log_err("error: serverip variable has to be set\n");
>> +		return CMD_RET_FAILURE;
>> +	}
>> +
>> +	ret = ipaddr_aton(server_ip, &srv);
>> +	if (!ret) {
>> +		log_err("error: ipaddr_aton\n");
>> +		return CMD_RET_FAILURE;
>> +	}
>> +
>> +	log_info("TFTP from server %s; our IP address is %s\n",
>> +		 server_ip, env_get("ipaddr"));
>> +	log_info("Filename '%s'.\n", fname);
>> +	log_info("Load address: 0x%lx\n", daddr);
>> +	log_info("Loading:");
>> +
>> +	bootstage_mark_name(BOOTSTAGE_KERNELREAD_START, "tftp_start");
>> +
>> +	err = tftp_init_client(&tftp);
>> +	if (!(err == ERR_OK || err == ERR_USE))
>> +		log_err("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) {
>> +		log_err("tftp_get err=%d\n", err);
>> +		return CMD_RET_FAILURE;
>> +	}
>> +
>> +	if (env_set_hex("fileaddr", addr)) {
>> +		log_err("fileaddr not updated\n");
>> +		return CMD_RET_FAILURE;
>> +	}
>> +
>> +	return CMD_RET_SUCCESS;
>> +}
>> --
>> 2.30.2
>>
>
> Thanks
> /Ilias
Maxim Uvarov Sept. 13, 2023, 1:38 p.m. UTC | #3 
On Wed, 13 Sept 2023 at 14:38, Simon Goldschmidt <goldsimon@gmx.de> wrote:

>
>
> On 13.09.2023 08:15, Ilias Apalodimas wrote:
> >> +
> >> +/*
> >> + * (C) Copyright 2023 Linaro Ltd. <maxim.uvarov@linaro.org>
> >> + */
> >> +
> >> +#include <common.h>
> >> +#include <command.h>
> >> +#include <console.h>
> >> +#include <bootstage.h>
> >> +
> >> +#include "tftp_client.h"
> >> +#include "tftp_server.h"
> >> +#include <tftp_example.h>
> >> +
> >> +#include <string.h>
> >> +
> >> +#include <net/ulwip.h>
> >> +
> >> +static ulong daddr;
> >> +static ulong size;
> >> +
> >> +static void *tftp_open(const char *fname, const char *mode, u8_t
> is_write)
> >> +{
> >> +    return NULL;
> >> +}
> >> +
> >> +static void tftp_close(void *handle)
> >> +{
> >> +    log_info("\ndone\n");
> >> +    log_info("Bytes transferred = %ld (0x%lx hex)\n", size, size);
> >> +
> >> +    bootstage_mark_name(BOOTSTAGE_KERNELREAD_STOP, "tftp_done");
> >> +    if (env_set_hex("filesize", size)) {
> >> +            log_err("filesize not updated\n");
> >> +            ulwip_exit(-1);
> >> +            return;
> >> +    }
> >> +    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);
> >> +            daddr += q->len;
> >> +            size += q->len;
> >> +            log_info("#");
> >> +    }
> >> +
> >> +    return 0;
> >> +}
> >> +
> >> +static void tftp_error(void *handle, int err, const char *msg, int
> size)
> >> +{
> >> +    char message[100];
> >> +
> >> +    memset(message, 0, sizeof(message));
> >> +    memcpy(message, msg, LWIP_MIN(sizeof(message) - 1, (size_t)size));
> >> +
> >> +    log_info("TFTP error: %d (%s)", err, message);
> >> +}
> >> +
> >> +static const struct tftp_context tftp = {
> >> +    tftp_open,
> >> +    tftp_close,
> >> +    tftp_read,
> >> +    tftp_write,
> >> +    tftp_error
> >> +};
> >> +
> >> +int ulwip_tftp(ulong addr, char *fname)
> >> +{
> >> +    void *f = (void *)0x1; /* unused fake file handle*/
> >
> > I haven't dug into lwip details yet, but I am not sure this is safe to
> use.
> > Simon, since you maintain the lwip part can you elaborate a bit more?
>
>  From the lwIP design, using an invalid pointer here is ok: that pointer
> is only used as a client handle which is never dereferenced internally.
> The only requirement is that it is not NULL, as that is the validity
> check for the tftp client to know the handle is valid (or e.g. open
> failed etc.).
>
> So while we can leave 0x1 here, using a static uint8_t would probably be
> better, at the expense of using a byte for nothing.
>
> Regards,
> Simon
>

U-Boot does not like statics due to the fact that it can relocate itself to
another address.

BR,
Maxim.


>
> >
> >> +    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) {
> >> +            log_err("error: serverip variable has to be set\n");
> >> +            return CMD_RET_FAILURE;
> >> +    }
> >> +
> >> +    ret = ipaddr_aton(server_ip, &srv);
> >> +    if (!ret) {
> >> +            log_err("error: ipaddr_aton\n");
> >> +            return CMD_RET_FAILURE;
> >> +    }
> >> +
> >> +    log_info("TFTP from server %s; our IP address is %s\n",
> >> +             server_ip, env_get("ipaddr"));
> >> +    log_info("Filename '%s'.\n", fname);
> >> +    log_info("Load address: 0x%lx\n", daddr);
> >> +    log_info("Loading:");
> >> +
> >> +    bootstage_mark_name(BOOTSTAGE_KERNELREAD_START, "tftp_start");
> >> +
> >> +    err = tftp_init_client(&tftp);
> >> +    if (!(err == ERR_OK || err == ERR_USE))
> >> +            log_err("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) {
> >> +            log_err("tftp_get err=%d\n", err);
> >> +            return CMD_RET_FAILURE;
> >> +    }
> >> +
> >> +    if (env_set_hex("fileaddr", addr)) {
> >> +            log_err("fileaddr not updated\n");
> >> +            return CMD_RET_FAILURE;
> >> +    }
> >> +
> >> +    return CMD_RET_SUCCESS;
> >> +}
> >> --
> >> 2.30.2
> >>
> >
> > Thanks
> > /Ilias
>
diff mbox series

Patch

diff --git a/include/net/lwip.h b/include/net/lwip.h
index 6a8fcef392..c9e2982a78 100644
--- a/include/net/lwip.h
+++ b/include/net/lwip.h
@@ -28,4 +28,16 @@  int ulwip_dns(char *name, char *varname);
  * Returns: 0 if success
  *         Other value < 0, if error
 */
-int ulwip_dhcp(void);
+
+/**
+ * ulwip_tftp() - load file with tftp
+ *
+ * Load file with tftp to specific address
+ *
+ * @addr: Address to store downloaded file
+ * @filename: File name on remote tftp server to download
+ *
+ *
+ * Returns:  0 if success, !0 if error
+ */
+int ulwip_tftp(ulong addr, const char *filename);
diff --git a/net/lwip/Makefile b/net/lwip/Makefile
index a3a33b7f71..b348e5ca31 100644
--- a/net/lwip/Makefile
+++ b/net/lwip/Makefile
@@ -65,3 +65,4 @@  obj-$(CONFIG_NET) += port/sys-arch.o
 
 obj-y += apps/dhcp/lwip-dhcp.o
 obj-y += apps/dns/lwip-dns.o
+obj-y += apps/tftp/
diff --git a/net/lwip/apps/tftp/Makefile b/net/lwip/apps/tftp/Makefile
new file mode 100644
index 0000000000..c3ad3c6353
--- /dev/null
+++ b/net/lwip/apps/tftp/Makefile
@@ -0,0 +1,7 @@ 
+ccflags-y += -I$(srctree)/net/lwip/port/include
+ccflags-y += -I$(srctree)/net/lwip/lwip-external/src/include -I$(srctree)/net/lwip
+ccflags-y += -I$(srctree)/net/lwip/lwip-external/src/include/lwip/apps
+ccflags-y += -I$(srctree)/net/lwip/lwip-external/contrib/examples/tftp/
+
+obj-y += ../../lwip-external/src/apps/tftp/tftp.o
+obj-y += lwip-tftp.o
diff --git a/net/lwip/apps/tftp/lwip-tftp.c b/net/lwip/apps/tftp/lwip-tftp.c
new file mode 100644
index 0000000000..56eed3030b
--- /dev/null
+++ b/net/lwip/apps/tftp/lwip-tftp.c
@@ -0,0 +1,129 @@ 
+// 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 <bootstage.h>
+
+#include "tftp_client.h"
+#include "tftp_server.h"
+#include <tftp_example.h>
+
+#include <string.h>
+
+#include <net/ulwip.h>
+
+static ulong daddr;
+static ulong size;
+
+static void *tftp_open(const char *fname, const char *mode, u8_t is_write)
+{
+	return NULL;
+}
+
+static void tftp_close(void *handle)
+{
+	log_info("\ndone\n");
+	log_info("Bytes transferred = %ld (0x%lx hex)\n", size, size);
+
+	bootstage_mark_name(BOOTSTAGE_KERNELREAD_STOP, "tftp_done");
+	if (env_set_hex("filesize", size)) {
+		log_err("filesize not updated\n");
+		ulwip_exit(-1);
+		return;
+	}
+	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);
+		daddr += q->len;
+		size += q->len;
+		log_info("#");
+	}
+
+	return 0;
+}
+
+static void tftp_error(void *handle, int err, const char *msg, int size)
+{
+	char message[100];
+
+	memset(message, 0, sizeof(message));
+	memcpy(message, msg, LWIP_MIN(sizeof(message) - 1, (size_t)size));
+
+	log_info("TFTP error: %d (%s)", err, message);
+}
+
+static const struct tftp_context tftp = {
+	tftp_open,
+	tftp_close,
+	tftp_read,
+	tftp_write,
+	tftp_error
+};
+
+int ulwip_tftp(ulong addr, char *fname)
+{
+	void *f = (void *)0x1; /* unused fake file handle*/
+	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) {
+		log_err("error: serverip variable has to be set\n");
+		return CMD_RET_FAILURE;
+	}
+
+	ret = ipaddr_aton(server_ip, &srv);
+	if (!ret) {
+		log_err("error: ipaddr_aton\n");
+		return CMD_RET_FAILURE;
+	}
+
+	log_info("TFTP from server %s; our IP address is %s\n",
+		 server_ip, env_get("ipaddr"));
+	log_info("Filename '%s'.\n", fname);
+	log_info("Load address: 0x%lx\n", daddr);
+	log_info("Loading:");
+
+	bootstage_mark_name(BOOTSTAGE_KERNELREAD_START, "tftp_start");
+
+	err = tftp_init_client(&tftp);
+	if (!(err == ERR_OK || err == ERR_USE))
+		log_err("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) {
+		log_err("tftp_get err=%d\n", err);
+		return CMD_RET_FAILURE;
+	}
+
+	if (env_set_hex("fileaddr", addr)) {
+		log_err("fileaddr not updated\n");
+		return CMD_RET_FAILURE;
+	}
+
+	return CMD_RET_SUCCESS;
+}