这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@jstucke
Copy link
Collaborator

@jstucke jstucke commented Oct 15, 2024

No description provided.

@jstucke jstucke requested a review from maringuu October 15, 2024 16:02
@jstucke jstucke self-assigned this Oct 15, 2024
@jstucke jstucke force-pushed the link-symlink-target branch from 270b8d8 to 24cf135 Compare October 29, 2024 10:09
Copy link
Collaborator

@maringuu maringuu left a comment

Choose a reason for hiding this comment

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

Overall I am hesitating to merge this as FACT's internals are not really built with support for symlinks in mind as can be seen by the above comments.
Of course, this feature is useful and desirable, but not possible to implement correctly atm.
I'll leave it to you or @dorpvom to merge this.

Comment on lines +486 to +487
for uid in session.execute(query.limit(1)).scalars():
return uid
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am aware that we, by design, can not get this right, but I'd just like to note that using limit(1) to get a random target file is not necessarily correct.

Copy link
Collaborator Author

@jstucke jstucke Dec 19, 2024

Choose a reason for hiding this comment

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

Normally, there should only be one result at this point (but there may be so weird edge cases that I haven't thought of)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To elaborate a bit on the underlying idea: at this point we know the root FW of the file and the path that is mentioned in the file type ("link to '/a/b/c'). The idea is to look in the "virtual path" for paths that line up with this. Usually, this should be only one file. But there are two other possibilities: the file is missing from the file system or there are multiple file systems or containers with this path contained in the firmware. In the latter case, there isn't really a right or wrong result, since the file object represents all of these files at once. We could display all link targets, though.

@jstucke jstucke force-pushed the link-symlink-target branch 2 times, most recently from ea4c2b5 to e255107 Compare January 9, 2025 09:38
@jstucke jstucke requested a review from dorpvom January 9, 2025 09:57
@codecov-commenter
Copy link

codecov-commenter commented Jan 9, 2025

Codecov Report

Attention: Patch coverage is 85.71429% with 6 lines in your changes missing coverage. Please review.

Project coverage is 91.67%. Comparing base (5cc8780) to head (adb1e79).
Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
src/web_interface/components/analysis_routes.py 71.42% 4 Missing ⚠️
src/storage/db_interface_frontend.py 83.33% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1282      +/-   ##
==========================================
- Coverage   92.46%   91.67%   -0.80%     
==========================================
  Files         383      376       -7     
  Lines       24537    21089    -3448     
==========================================
- Hits        22689    19333    -3356     
+ Misses       1848     1756      -92     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@maringuu maringuu self-requested a review February 13, 2025 07:43
@jstucke jstucke force-pushed the link-symlink-target branch from e255107 to adb1e79 Compare February 14, 2025 14:27
@jstucke
Copy link
Collaborator Author

jstucke commented Feb 14, 2025

rebased on master to fix the bug that let the CI fail

@jstucke jstucke merged commit 9010149 into master Feb 17, 2025
10 checks passed
@jstucke jstucke deleted the link-symlink-target branch February 17, 2025 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants