Skip to content
This repository has been archived by the owner on Jul 25, 2022. It is now read-only.

PR-2211 Add llapi_path2fid support #2212

Merged
merged 2 commits into from
Sep 9, 2020
Merged

Conversation

beevans
Copy link
Contributor

@beevans beevans commented Sep 2, 2020

Signed-off-by: Ben Evans beevans@whamcloud.com


This change is Reviewable

let mut fid = Fid{ seq: 0, oid: 0, ver: 0,};

let rc = unsafe {
let rc = match self.lib.get(b"llapi_path2fid\0") {
Copy link
Contributor

Choose a reason for hiding this comment

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

This would be better as:

if let Ok(f) = self.lib.get(b"llapi_path2fid\0") {
...

Since we don't care about what the error is.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should add a SymbolNotFound variant to LiblustreError so it's clear to the caller what went wrong.

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'm just following the pattern of fid2path

Copy link
Member

Choose a reason for hiding this comment

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

yeah, looks like that should be updated as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So open a ticket to add SymbolNotFound to LiblustreError and handle all the cases in here?

Copy link
Member

Choose a reason for hiding this comment

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

New ticket and fix in here is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#2226 created

Copy link
Member

Choose a reason for hiding this comment

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

PR: #2228

liblustreapi/src/lib.rs Outdated Show resolved Hide resolved
liblustreapi/src/lib.rs Outdated Show resolved Hide resolved
@jgrund
Copy link
Member

jgrund commented Sep 5, 2020

Looks like this code needs rustfmt run on it.

utopiabound
utopiabound previously approved these changes Sep 8, 2020
@jgrund
Copy link
Member

jgrund commented Sep 8, 2020

This needs rebase on latest master due to: #2223

Signed-off-by: Ben Evans <beevans@whamcloud.com>
Signed-off-by: Ben Evans <beevans@whamcloud.com>
@jgrund jgrund merged commit 6154c69 into whamcloud:master Sep 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants