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

AP_HAL_Linux: file descriptor leaking and other issues in "Storage" of Linux #28052

Merged
merged 1 commit into from
Sep 17, 2024

Conversation

junan76
Copy link
Contributor

@junan76 junan76 commented Sep 9, 2024

  • Fixed the fd leaking issue in "_storage_create" found by @peterbarker
  • Remove the unnecessary call of "unlinkat" in "_storage_create"
  • Simplify the implementation of "init"

…f Linux.

- Fixed the fd leaking issue in "_storage_create" found by @peterbarker
- Remove the unnecessary call of "unlinkat" in "_storage_create"
- Simplify the implementation of "init"

Signed-off-by: junan <[email protected]>
@peterbarker
Copy link
Contributor

This looks like a nice change.

How has it been tested?

@peterbarker peterbarker self-requested a review September 11, 2024 00:42
@junan76
Copy link
Contributor Author

junan76 commented Sep 11, 2024

This looks like a nice change.

How has it been tested?

I tested it on beaglebone black board using gdb, will post the screenshots this evening, you can check the result then.

@@ -145,26 +145,16 @@ void Storage::init()
dpath = HAL_BOARD_STORAGE_DIRECTORY;
}

int fd = open(dpath, O_RDWR|O_CLOEXEC);
int fd = _storage_create(dpath);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should rename this to_storage_open_or_create. Just wondering.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can rename it to _storage_open maybe, we call openat with O_CREAT flag which will create the file if it does not exist.

@junan76
Copy link
Contributor Author

junan76 commented Sep 11, 2024

  1. start gdb server on beaglebone board
    Screenshot from 2024-09-11 19-09-08
  2. during the call of _storage_create
  • "dpath" and storage file is opened successfuly
  • ftruncate and fsync is ok
  • dfd is closed and fd is returned
    Screenshot from 2024-09-11 19-12-43
  1. back to init
  • we read exactly 16384 bytes data from fd
  • _fd and _initialized are both set with correct value
    Screenshot from 2024-09-11 19-14-21
  1. connecting the board with qgroundcontrol and set SCHED_LOOP_RATE to 100Hz
    Screenshot from 2024-09-11 19-17-55

@junan76 junan76 requested a review from peterbarker September 11, 2024 23:18
Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@junan76
Copy link
Contributor Author

junan76 commented Sep 16, 2024

Hi peter, What should I do with the DevCallTopic label?

@Williangalvani
Copy link
Contributor

Just tried it on a Navigator and and everything worked as expectd

@tridge tridge merged commit 71a6936 into ArduPilot:master Sep 17, 2024
61 checks passed
@junan76 junan76 deleted the pr/storage-linux branch September 18, 2024 01:16
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.

5 participants