Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

apps/nshlib: Using lib_get_tempbuffer() to save stack space #65

Conversation

JianyuWang0623
Copy link
Member

@JianyuWang0623 JianyuWang0623 commented Jan 16, 2025

Pick from

  1. nshlib/nsh_parse: Closing fds opened for redirection if necessary  apache/nuttx-apps#2903
  2. apps/nshlib: Using lib_get_tempbuffer() to save stack space apache/nuttx-apps#2943
  3. apps/nshlib: Save result and return ERROR if lib_get_tempbuffer() fails apache/nuttx-apps#2950

Summary

Using lib_get_tempbuffer() to save stack space

  1. apps/nshlib: Using lib_get_tempbuffer() to save stack space
  2. apps/nshlib: replace CONFIG_NSH_LINELEN to LINE_MAX
  3. apps/system: replace CONFIG_NSH_LINELEN to LINE_MAX (Picked from apps/system: replace CONFIG_NSH_LINELEN to LINE_MAX apache/nuttx-apps#2918 by @anchao )

Impact

  • apps/nshlib
  • apps/system

Testing

  1. Selftest as commit message shown
    • Config: "esp32s3-devkit:adb" with CONFIG_LINE_MAX=512

    • Test CMD: ls | cat | cat | cat

    • Without this patch

      Before Test CMD After Test CMD
      nsh_main.STACK 0002624/0008096 0002624/0008096
      sh.STACK 0003360/0004008 0003360/0004008
      Free/Total 355288/403116 355288/403116
    • With this patch

      Before Test CMD After Test CMD
      nsh_main.STACK 0001616/0008096 0001616/0008096
      sh.STACK 0002352/0004008 0002352/0004008
      Free/Total 355288/403116 354760/403116
  2. CI

anchao and others added 4 commits January 16, 2025 21:54
Applications should not depend on any properties of nshlib

Signed-off-by: chao an <[email protected]>
Comparison

  Config: "esp32s3-devkit:adb" with `CONFIG_LINE_MAX=512`

  Test CMD: `ls | cat | cat | cat`

  Without this patch

    |                | Before Test CMD | After Test CMD  |
    |---------------:|----------------:|----------------:|
    | nsh_main.STACK | 0002624/0008096 | 0002624/0008096 |
    |       sh.STACK | 0003360/0004008 | 0003360/0004008 |
    |     Free/Total |   355288/403116 |   355288/403116 |

  With this patch

    |                | Before Test CMD | After Test CMD  |
    |---------------:|----------------:|----------------:|
    | nsh_main.STACK | 0001616/0008096 | 0001616/0008096 |
    |       sh.STACK | 0002352/0004008 | 0002352/0004008 |
    |     Free/Total |   355288/403116 |   354760/403116 |

Signed-off-by: wangjianyu3 <[email protected]>
@CLAassistant
Copy link

CLAassistant commented Jan 16, 2025

CLA assistant check
All committers have signed the CLA.

@JianyuWang0623
Copy link
Member Author

CLA assistant check Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.1 out of 2 committers have signed the CLA.✅ JianyuWang0623❌ anchaoYou have signed the CLA already but the status is still pending? Let us recheck it.

@anchao Could you take a look about the CLA please ;-)

@JianyuWang0623 JianyuWang0623 marked this pull request as draft January 16, 2025 14:12
Coverity Log

  CID 1612743: (open-vela#1 of 1): Resource leak (RESOURCE_LEAK)
  12. leaked_handle: The handle variable fd_out goes out of scope and leaks the handle.

Signed-off-by: wangjianyu3 <[email protected]>
@JianyuWang0623 JianyuWang0623 marked this pull request as ready for review January 16, 2025 14:37
@anchao
Copy link
Contributor

anchao commented Jan 16, 2025

@anchao Could you take a look about the CLA please ;-)

Done

@xiaoxiang781216 xiaoxiang781216 merged commit 86e7e17 into open-vela:dev Jan 16, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants