Skip to content
New issue

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

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

Already on GitHub? # to your account

Correct defective return value in Posix::ls_with_sizes #5037

Merged
merged 2 commits into from
Jun 3, 2024

Conversation

eric-hughes-tiledb
Copy link
Contributor

@eric-hughes-tiledb eric-hughes-tiledb commented Jun 3, 2024

PR #2671 introduced ls_with_sizes. There was a defect in class Posix where an empty directory returned nullopt instead of an empty vector. This PR corrects that defect.

I audited the original PR for possible related errors. This was the only one.

[sc-48646]


TYPE: BUG
DESC: Correct defective return value in Posix::ls_with_sizes

PR #2671 introduced `ls_with_sizes`. There was a defect in `class Posix` where an empty directory return `nullopt` instead of an empty vector. This PR corrects that defect.

I audited the original PR for possible related errors. This was the only one.
Copy link
Member

@teo-tsirpanis teo-tsirpanis left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. Left one comment.

tiledb/sm/filesystem/posix.cc Outdated Show resolved Hide resolved
@KiterLuc KiterLuc marked this pull request as ready for review June 3, 2024 17:33
Copy link
Member

@teo-tsirpanis teo-tsirpanis left a comment

Choose a reason for hiding this comment

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

Thanks!

@KiterLuc KiterLuc merged commit 18cfeb1 into dev Jun 3, 2024
62 checks passed
@KiterLuc KiterLuc deleted the eh/correct_posix_ls_with_sizes_incorrect_return branch June 3, 2024 21:51
@ihnorton
Copy link
Member

ihnorton commented Jun 3, 2024

Thanks. This is worth back-porting IMO (but no need for a standalone patch release).

KiterLuc added a commit that referenced this pull request Jun 6, 2024
…X. (#5043)

[SC-23179](https://app.shortcut.com/tiledb-inc/story/23179)
[SC-48851](https://app.shortcut.com/tiledb-inc/story/48851)
[SC-48854](https://app.shortcut.com/tiledb-inc/story/48854)

This PR updates the signature of `ls_with_sizes` in the `VFS` class and
the specific VFS implementations[^1] to indicate errors by throwing
exceptions instead of returning `Status`.

On top of that, `Posix::ls_with_sizes` was updated to fail if `opendir`
failed with an error code other than `ENOENT` (see also
#5037 (comment)).

Also in `Posix::ls_with_sizes`, the pointer returned by `opendir` was
updated to be placed inside a smart pointer, making sure it gets freed.
This caused errors in the `find_head_api_violations.py` script, which
were fixed.

[^1]: HDFS was not updated to minimize risk.

---
TYPE: BUG
DESC: Do not mask failures when listing a directory fails on POSIX.

---------

Co-authored-by: Luc Rancourt <lucrancourt@gmail.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants