| Message ID | 20250415193436.1240804-3-jerome.forissier@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | NET_LWIP LMB fixes | expand |
On Wed, 16 Apr 2025 at 01:04, Jerome Forissier <jerome.forissier@linaro.org> wrote: > > Functions called from EFI applications should not do console output. > Refactor the wget code to implement this requirement. The wget_http_info > struct is used to hold the boolean that signifies whether the output is > allowed or not. > > Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> > Reported-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > --- > > Changes in v4: > - Patch renamed, deals with NET in addition to NET_LWIP > > Changes in v3: > - New patch > > include/net-common.h | 2 ++ > lib/efi_loader/efi_net.c | 2 +- > net/lwip/wget.c | 33 +++++++++++++++++++++++---------- > net/wget.c | 23 +++++++++++++++++------ > 4 files changed, 43 insertions(+), 17 deletions(-) > > diff --git a/include/net-common.h b/include/net-common.h > index 30860f5975a..1043b24d0b9 100644 > --- a/include/net-common.h > +++ b/include/net-common.h > @@ -555,6 +555,7 @@ enum wget_http_method { > * Filled by client. > * @hdr_cont_len: content length according to headers. Filled by wget > * @headers: buffer for headers. Filled by wget. > + * @silent: do not print anything to the console. Filled by client. > */ > struct wget_http_info { > enum wget_http_method method; > @@ -565,6 +566,7 @@ struct wget_http_info { > bool check_buffer_size; > u32 hdr_cont_len; > char *headers; > + bool silent; > }; > > extern struct wget_http_info default_wget_info; > diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c > index b3291b4f1d5..9ff0b691ee1 100644 > --- a/lib/efi_loader/efi_net.c > +++ b/lib/efi_loader/efi_net.c > @@ -51,7 +51,7 @@ static int next_dp_entry; > static struct wget_http_info efi_wget_info = { > .set_bootdev = false, > .check_buffer_size = true, > - > + .silent = true, > }; > #endif > > diff --git a/net/lwip/wget.c b/net/lwip/wget.c > index 2b512a1bc84..3529768eb06 100644 > --- a/net/lwip/wget.c > +++ b/net/lwip/wget.c > @@ -217,7 +217,8 @@ static err_t httpc_recv_cb(void *arg, struct altcp_pcb *pcb, struct pbuf *pbuf, > memcpy((void *)ctx->daddr, buf->payload, buf->len); > ctx->daddr += buf->len; > ctx->size += buf->len; > - if (ctx->size - ctx->prevsize > PROGRESS_PRINT_STEP_BYTES) { > + if (!wget_info->silent && > + ctx->size - ctx->prevsize > PROGRESS_PRINT_STEP_BYTES) { > printf("#"); > ctx->prevsize = ctx->size; > } > @@ -255,11 +256,15 @@ static void httpc_result_cb(void *arg, httpc_result_t httpc_result, > elapsed = get_timer(ctx->start_time); > if (!elapsed) > elapsed = 1; > - if (rx_content_len > PROGRESS_PRINT_STEP_BYTES) > - printf("\n"); > - printf("%u bytes transferred in %lu ms (", rx_content_len, elapsed); > - print_size(rx_content_len / elapsed * 1000, "/s)\n"); > - printf("Bytes transferred = %lu (%lx hex)\n", ctx->size, ctx->size); > + if (!wget_info->silent) { > + if (rx_content_len > PROGRESS_PRINT_STEP_BYTES) > + printf("\n"); > + printf("%u bytes transferred in %lu ms (", rx_content_len, > + elapsed); > + print_size(rx_content_len / elapsed * 1000, "/s)\n"); > + printf("Bytes transferred = %lu (%lx hex)\n", ctx->size, > + ctx->size); > + } > if (wget_info->set_bootdev) > efi_set_bootdev("Http", ctx->server_name, ctx->path, map_sysmem(ctx->saved_daddr, 0), > rx_content_len); > @@ -339,7 +344,8 @@ static int _set_cacert(const void *addr, size_t sz) > mbedtls_x509_crt_init(&crt); > ret = mbedtls_x509_crt_parse(&crt, cacert, cacert_size); > if (ret) { > - printf("Could not parse certificates (%d)\n", ret); > + if (!wget_info->silent) > + printf("Could not parse certificates (%d)\n", ret); > free(cacert); > cacert = NULL; > cacert_size = 0; > @@ -392,6 +398,12 @@ int wget_do_request(ulong dst_addr, char *uri) > ctx.prevsize = 0; > ctx.start_time = 0; > > + if (!wget_info) > + wget_info = &default_wget_info; > + > + net_lwip_set_current(); > + udev = eth_get_dev(); > + Wasn't this already done in the first patch? Looks like this hunk is superfluous. Apart from that, it looks fine to me. Thanks for taking up this change. -sughosh > if (parse_url(uri, ctx.server_name, &ctx.port, &path, &is_https)) > return CMD_RET_USAGE; > > @@ -421,9 +433,10 @@ int wget_do_request(ulong dst_addr, char *uri) > > if (cacert_auth_mode == AUTH_REQUIRED) { > if (!ca || !ca_sz) { > - printf("Error: cacert authentication mode is " > - "'required' but no CA certificates " > - "given\n"); > + if (!wget_info->silent) > + printf("Error: cacert authentication " > + "mode is 'required' but no CA " > + "certificates given\n"); > return CMD_RET_FAILURE; > } > } else if (cacert_auth_mode == AUTH_NONE) { > diff --git a/net/wget.c b/net/wget.c > index c73836cbc9d..3c0fff488eb 100644 > --- a/net/wget.c > +++ b/net/wget.c > @@ -59,8 +59,10 @@ static inline int store_block(uchar *src, unsigned int offset, unsigned int len) > if (CONFIG_IS_ENABLED(LMB) && wget_info->set_bootdev) { > if (store_addr < image_load_addr || > lmb_read_check(store_addr, len)) { > - printf("\nwget error: "); > - printf("trying to overwrite reserved memory...\n"); > + if (!wget_info->silent) { > + printf("\nwget error: "); > + printf("trying to overwrite reserved memory\n"); > + } > return -1; > } > } > @@ -76,6 +78,9 @@ static void show_block_marker(u32 packets) > { > int cnt; > > + if (wget_info->silent) > + return; > + > if (content_length != -1) { > if (net_boot_file_size > content_length) > content_length = net_boot_file_size; > @@ -101,11 +106,15 @@ static void tcp_stream_on_closed(struct tcp_stream *tcp) > net_set_state(wget_loop_state); > if (wget_loop_state != NETLOOP_SUCCESS) { > net_boot_file_size = 0; > - printf("\nwget: Transfer Fail, TCP status - %d\n", tcp->status); > + if (!wget_info->silent) > + printf("\nwget: Transfer Fail, TCP status - %d\n", > + tcp->status); > return; > } > > - printf("\nPackets received %d, Transfer Successful\n", tcp->rx_packets); > + if (!wget_info->silent) > + printf("\nPackets received %d, Transfer Successful\n", > + tcp->rx_packets); > wget_info->file_size = net_boot_file_size; > if (wget_info->method == WGET_HTTP_METHOD_GET && wget_info->set_bootdev) { > efi_set_bootdev("Http", NULL, image_url, > @@ -139,7 +148,8 @@ static void tcp_stream_on_rcv_nxt_update(struct tcp_stream *tcp, u32 rx_bytes) > tcp->state == TCP_ESTABLISHED) > goto end; > > - printf("ERROR: misssed HTTP header\n"); > + if (!wget_info->silent) > + printf("ERROR: misssed HTTP header\n"); > tcp_stream_close(tcp); > goto end; > } > @@ -346,7 +356,8 @@ void wget_start(void) > tcp_stream_set_on_create_handler(tcp_stream_on_create); > tcp = tcp_stream_connect(web_server_ip, server_port); > if (!tcp) { > - printf("No free tcp streams\n"); > + if (!wget_info->silent) > + printf("No free tcp streams\n"); > net_set_state(NETLOOP_FAIL); > return; > } > -- > 2.43.0 >
Hi Sughosh, On 4/16/25 12:22, Sughosh Ganu wrote: > On Wed, 16 Apr 2025 at 01:04, Jerome Forissier > <jerome.forissier@linaro.org> wrote: >> >> Functions called from EFI applications should not do console output. >> Refactor the wget code to implement this requirement. The wget_http_info >> struct is used to hold the boolean that signifies whether the output is >> allowed or not. >> >> Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> >> Reported-by: Heinrich Schuchardt <xypron.glpk@gmx.de> >> --- >> >> Changes in v4: >> - Patch renamed, deals with NET in addition to NET_LWIP >> >> Changes in v3: >> - New patch >> >> include/net-common.h | 2 ++ >> lib/efi_loader/efi_net.c | 2 +- >> net/lwip/wget.c | 33 +++++++++++++++++++++++---------- >> net/wget.c | 23 +++++++++++++++++------ >> 4 files changed, 43 insertions(+), 17 deletions(-) >> >> diff --git a/include/net-common.h b/include/net-common.h >> index 30860f5975a..1043b24d0b9 100644 >> --- a/include/net-common.h >> +++ b/include/net-common.h >> @@ -555,6 +555,7 @@ enum wget_http_method { >> * Filled by client. >> * @hdr_cont_len: content length according to headers. Filled by wget >> * @headers: buffer for headers. Filled by wget. >> + * @silent: do not print anything to the console. Filled by client. >> */ >> struct wget_http_info { >> enum wget_http_method method; >> @@ -565,6 +566,7 @@ struct wget_http_info { >> bool check_buffer_size; >> u32 hdr_cont_len; >> char *headers; >> + bool silent; >> }; >> >> extern struct wget_http_info default_wget_info; >> diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c >> index b3291b4f1d5..9ff0b691ee1 100644 >> --- a/lib/efi_loader/efi_net.c >> +++ b/lib/efi_loader/efi_net.c >> @@ -51,7 +51,7 @@ static int next_dp_entry; >> static struct wget_http_info efi_wget_info = { >> .set_bootdev = false, >> .check_buffer_size = true, >> - >> + .silent = true, >> }; >> #endif >> >> diff --git a/net/lwip/wget.c b/net/lwip/wget.c >> index 2b512a1bc84..3529768eb06 100644 >> --- a/net/lwip/wget.c >> +++ b/net/lwip/wget.c >> @@ -217,7 +217,8 @@ static err_t httpc_recv_cb(void *arg, struct altcp_pcb *pcb, struct pbuf *pbuf, >> memcpy((void *)ctx->daddr, buf->payload, buf->len); >> ctx->daddr += buf->len; >> ctx->size += buf->len; >> - if (ctx->size - ctx->prevsize > PROGRESS_PRINT_STEP_BYTES) { >> + if (!wget_info->silent && >> + ctx->size - ctx->prevsize > PROGRESS_PRINT_STEP_BYTES) { >> printf("#"); >> ctx->prevsize = ctx->size; >> } >> @@ -255,11 +256,15 @@ static void httpc_result_cb(void *arg, httpc_result_t httpc_result, >> elapsed = get_timer(ctx->start_time); >> if (!elapsed) >> elapsed = 1; >> - if (rx_content_len > PROGRESS_PRINT_STEP_BYTES) >> - printf("\n"); >> - printf("%u bytes transferred in %lu ms (", rx_content_len, elapsed); >> - print_size(rx_content_len / elapsed * 1000, "/s)\n"); >> - printf("Bytes transferred = %lu (%lx hex)\n", ctx->size, ctx->size); >> + if (!wget_info->silent) { >> + if (rx_content_len > PROGRESS_PRINT_STEP_BYTES) >> + printf("\n"); >> + printf("%u bytes transferred in %lu ms (", rx_content_len, >> + elapsed); >> + print_size(rx_content_len / elapsed * 1000, "/s)\n"); >> + printf("Bytes transferred = %lu (%lx hex)\n", ctx->size, >> + ctx->size); >> + } >> if (wget_info->set_bootdev) >> efi_set_bootdev("Http", ctx->server_name, ctx->path, map_sysmem(ctx->saved_daddr, 0), >> rx_content_len); >> @@ -339,7 +344,8 @@ static int _set_cacert(const void *addr, size_t sz) >> mbedtls_x509_crt_init(&crt); >> ret = mbedtls_x509_crt_parse(&crt, cacert, cacert_size); >> if (ret) { >> - printf("Could not parse certificates (%d)\n", ret); >> + if (!wget_info->silent) >> + printf("Could not parse certificates (%d)\n", ret); >> free(cacert); >> cacert = NULL; >> cacert_size = 0; >> @@ -392,6 +398,12 @@ int wget_do_request(ulong dst_addr, char *uri) >> ctx.prevsize = 0; >> ctx.start_time = 0; >> >> + if (!wget_info) >> + wget_info = &default_wget_info; >> + >> + net_lwip_set_current(); >> + udev = eth_get_dev(); >> + > > Wasn't this already done in the first patch? Looks like this hunk is > superfluous. Apart from that, it looks fine to me. Thanks for taking > up this change. Indeed, I messed up when rebasing. I'll remove this hunk. Thank you,
diff --git a/include/net-common.h b/include/net-common.h index 30860f5975a..1043b24d0b9 100644 --- a/include/net-common.h +++ b/include/net-common.h @@ -555,6 +555,7 @@ enum wget_http_method { * Filled by client. * @hdr_cont_len: content length according to headers. Filled by wget * @headers: buffer for headers. Filled by wget. + * @silent: do not print anything to the console. Filled by client. */ struct wget_http_info { enum wget_http_method method; @@ -565,6 +566,7 @@ struct wget_http_info { bool check_buffer_size; u32 hdr_cont_len; char *headers; + bool silent; }; extern struct wget_http_info default_wget_info; diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c index b3291b4f1d5..9ff0b691ee1 100644 --- a/lib/efi_loader/efi_net.c +++ b/lib/efi_loader/efi_net.c @@ -51,7 +51,7 @@ static int next_dp_entry; static struct wget_http_info efi_wget_info = { .set_bootdev = false, .check_buffer_size = true, - + .silent = true, }; #endif diff --git a/net/lwip/wget.c b/net/lwip/wget.c index 2b512a1bc84..3529768eb06 100644 --- a/net/lwip/wget.c +++ b/net/lwip/wget.c @@ -217,7 +217,8 @@ static err_t httpc_recv_cb(void *arg, struct altcp_pcb *pcb, struct pbuf *pbuf, memcpy((void *)ctx->daddr, buf->payload, buf->len); ctx->daddr += buf->len; ctx->size += buf->len; - if (ctx->size - ctx->prevsize > PROGRESS_PRINT_STEP_BYTES) { + if (!wget_info->silent && + ctx->size - ctx->prevsize > PROGRESS_PRINT_STEP_BYTES) { printf("#"); ctx->prevsize = ctx->size; } @@ -255,11 +256,15 @@ static void httpc_result_cb(void *arg, httpc_result_t httpc_result, elapsed = get_timer(ctx->start_time); if (!elapsed) elapsed = 1; - if (rx_content_len > PROGRESS_PRINT_STEP_BYTES) - printf("\n"); - printf("%u bytes transferred in %lu ms (", rx_content_len, elapsed); - print_size(rx_content_len / elapsed * 1000, "/s)\n"); - printf("Bytes transferred = %lu (%lx hex)\n", ctx->size, ctx->size); + if (!wget_info->silent) { + if (rx_content_len > PROGRESS_PRINT_STEP_BYTES) + printf("\n"); + printf("%u bytes transferred in %lu ms (", rx_content_len, + elapsed); + print_size(rx_content_len / elapsed * 1000, "/s)\n"); + printf("Bytes transferred = %lu (%lx hex)\n", ctx->size, + ctx->size); + } if (wget_info->set_bootdev) efi_set_bootdev("Http", ctx->server_name, ctx->path, map_sysmem(ctx->saved_daddr, 0), rx_content_len); @@ -339,7 +344,8 @@ static int _set_cacert(const void *addr, size_t sz) mbedtls_x509_crt_init(&crt); ret = mbedtls_x509_crt_parse(&crt, cacert, cacert_size); if (ret) { - printf("Could not parse certificates (%d)\n", ret); + if (!wget_info->silent) + printf("Could not parse certificates (%d)\n", ret); free(cacert); cacert = NULL; cacert_size = 0; @@ -392,6 +398,12 @@ int wget_do_request(ulong dst_addr, char *uri) ctx.prevsize = 0; ctx.start_time = 0; + if (!wget_info) + wget_info = &default_wget_info; + + net_lwip_set_current(); + udev = eth_get_dev(); + if (parse_url(uri, ctx.server_name, &ctx.port, &path, &is_https)) return CMD_RET_USAGE; @@ -421,9 +433,10 @@ int wget_do_request(ulong dst_addr, char *uri) if (cacert_auth_mode == AUTH_REQUIRED) { if (!ca || !ca_sz) { - printf("Error: cacert authentication mode is " - "'required' but no CA certificates " - "given\n"); + if (!wget_info->silent) + printf("Error: cacert authentication " + "mode is 'required' but no CA " + "certificates given\n"); return CMD_RET_FAILURE; } } else if (cacert_auth_mode == AUTH_NONE) { diff --git a/net/wget.c b/net/wget.c index c73836cbc9d..3c0fff488eb 100644 --- a/net/wget.c +++ b/net/wget.c @@ -59,8 +59,10 @@ static inline int store_block(uchar *src, unsigned int offset, unsigned int len) if (CONFIG_IS_ENABLED(LMB) && wget_info->set_bootdev) { if (store_addr < image_load_addr || lmb_read_check(store_addr, len)) { - printf("\nwget error: "); - printf("trying to overwrite reserved memory...\n"); + if (!wget_info->silent) { + printf("\nwget error: "); + printf("trying to overwrite reserved memory\n"); + } return -1; } } @@ -76,6 +78,9 @@ static void show_block_marker(u32 packets) { int cnt; + if (wget_info->silent) + return; + if (content_length != -1) { if (net_boot_file_size > content_length) content_length = net_boot_file_size; @@ -101,11 +106,15 @@ static void tcp_stream_on_closed(struct tcp_stream *tcp) net_set_state(wget_loop_state); if (wget_loop_state != NETLOOP_SUCCESS) { net_boot_file_size = 0; - printf("\nwget: Transfer Fail, TCP status - %d\n", tcp->status); + if (!wget_info->silent) + printf("\nwget: Transfer Fail, TCP status - %d\n", + tcp->status); return; } - printf("\nPackets received %d, Transfer Successful\n", tcp->rx_packets); + if (!wget_info->silent) + printf("\nPackets received %d, Transfer Successful\n", + tcp->rx_packets); wget_info->file_size = net_boot_file_size; if (wget_info->method == WGET_HTTP_METHOD_GET && wget_info->set_bootdev) { efi_set_bootdev("Http", NULL, image_url, @@ -139,7 +148,8 @@ static void tcp_stream_on_rcv_nxt_update(struct tcp_stream *tcp, u32 rx_bytes) tcp->state == TCP_ESTABLISHED) goto end; - printf("ERROR: misssed HTTP header\n"); + if (!wget_info->silent) + printf("ERROR: misssed HTTP header\n"); tcp_stream_close(tcp); goto end; } @@ -346,7 +356,8 @@ void wget_start(void) tcp_stream_set_on_create_handler(tcp_stream_on_create); tcp = tcp_stream_connect(web_server_ip, server_port); if (!tcp) { - printf("No free tcp streams\n"); + if (!wget_info->silent) + printf("No free tcp streams\n"); net_set_state(NETLOOP_FAIL); return; }
Functions called from EFI applications should not do console output. Refactor the wget code to implement this requirement. The wget_http_info struct is used to hold the boolean that signifies whether the output is allowed or not. Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> Reported-by: Heinrich Schuchardt <xypron.glpk@gmx.de> --- Changes in v4: - Patch renamed, deals with NET in addition to NET_LWIP Changes in v3: - New patch include/net-common.h | 2 ++ lib/efi_loader/efi_net.c | 2 +- net/lwip/wget.c | 33 +++++++++++++++++++++++---------- net/wget.c | 23 +++++++++++++++++------ 4 files changed, 43 insertions(+), 17 deletions(-)