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

fcntl adding F_LOG2PHYS* flags. #2483

Merged
merged 1 commit into from
Sep 13, 2024

Conversation

devnexen
Copy link
Contributor

@devnexen devnexen commented Sep 5, 2024

to get the current device address, in practice its current offset only.

@devnexen devnexen marked this pull request as ready for review September 6, 2024 07:33
src/fcntl.rs Outdated
/// only the file offset data is set.
/// difference with F_LOG2PHYS is the struct passed
/// is used as both IN/OUT.
F_LOG2PHYS_EXT(&'a mut libc::off_t),
Copy link
Member

Choose a reason for hiding this comment

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

The F_LOG2PHYS looks good to me, I am confused about the meaning and usage of F_LOG2PHYS_EXT:

From man page

The F_LOG2PHYS_EXT command operates on the same structure as F_LOG2PHYS but treats it as an in/out:

        struct log2phys {
            u_int32_t l2p_flags;        /* unused so far */
            off_t     l2p_contigbytes;  /* IN: number of bytes to be queried;
                                           OUT: number of contiguous bytes allocated at this position */
            off_t     l2p_devoffset;    /* IN: bytes into file;
                                           OUT: bytes into device */
        };
  1. The interface proposed in this PR only exposes that l2p_devoffset argument, but unlike F_LOG2PHYS, l2p_contigbytes is also used, should we expose it?

  2. What does "difference with F_LOG2PHYS is the struct passed is used as both IN/OUT" mean? From the unit test, looks like it is similar to lseek(2), if so, why are we using a mutable reference to off_t here? Or it is possible that the returned offset could be different from the value passed in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to be honest F_LOGPHYS_EXT was kind of "late addition" I only noticed it just before I commited :)

The interface proposed in this PR only exposes that l2p_devoffset argument, but unlike F_LOG2PHYS, l2p_contigbytes is also used, should we expose it?

Yes. Better just exposing the struct altogether in this case.

What does "difference with F_LOG2PHYS is the struct passed is used as both IN/OUT" mean? From the unit test, looks like it is similar to lseek(2), if so, why are we using a mutable reference to off_t here? Or it is possible that the returned offset could be different from the value passed in?

I may need to rephrase it, was just explaining the comment above about those fields being both IN/OUT for F_LOG2PHYS_EXT. Value of l2p_devoffset can change thus why I add mut qualifier.

Copy link
Member

Choose a reason for hiding this comment

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

to be honest F_LOGPHYS_EXT was kind of "late addition" I only noticed it just before I commited :)

Got it:)

Yes. Better just exposing the struct altogether in this case.

Yeah, that makes sense to me.

I may need to rephrase it,

Thanks!

Value of l2p_devoffset can change thus why I add mut qualifier.

Then what's the IN semantic here? i.e., the meaning of the value passed in🤔

Copy link
Member

Choose a reason for hiding this comment

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

Do you mind telling me the functionality of that _EXT flag, I think I am still confused about it:<

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is the same as LOG2PHYS only l2p_devoffset/l2p_contigbytes are set after call but also can be set before the call whereas LOG2PHYS set l2p_devoffset after the call.

Copy link
Member

Choose a reason for hiding this comment

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

Though I am still unclear about this flag, but it should not matter as we expose everything needed in this interface, so merging:)

to get the current device address, in practice its current offset only.
Copy link
Member

@SteveLauC SteveLauC left a comment

Choose a reason for hiding this comment

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

Thanks!

@SteveLauC SteveLauC added this pull request to the merge queue Sep 13, 2024
Merged via the queue into nix-rust:master with commit 12e1f5a Sep 13, 2024
38 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.

2 participants