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

use uint32_t & uint64_t instead of u32 & u64, and fix #includes #164

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

RokerHRO
Copy link
Contributor

@RokerHRO RokerHRO commented May 15, 2024

I'd favor to get rid of the non-standard typedefs u32 and u64 and use C99 standard types uint32_t and uint64_t everywhere. These types are already used in the project, but mixed wildly with the old types.

During this I also moved some #include directives so each .h includes only the headers to fulfill its own needs (e.g. to use foreign declarations for its own declarations). Headers that are only needed for the implementation are included only in the .c file.

Redundant or unneeded includes are removed at all.

@ebblake
Copy link
Contributor

ebblake commented May 15, 2024

I would suggest breaking this into multiple commits (each patch should do one thing well, rather than doing multiple things mashed together); it makes it easier if someone wants to backport one change but not the other. But that's not a hard requirement. In this case, an obvious split would be:
patch 1: clean up #include directives
patch 2: consistent use of uint32_t/uint64_t

@@ -180,6 +181,7 @@ static void netlink_configure(int index, int *sockfds, int num_connects,
NBD_CMD_CONNECT, 0);
if (index >= 0)
NLA_PUT_U32(msg, NBD_ATTR_INDEX, index);

NLA_PUT_U64(msg, NBD_ATTR_SIZE_BYTES, size64);
Copy link
Contributor

Choose a reason for hiding this comment

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

Spurious whitespace change?

@ebblake
Copy link
Contributor

ebblake commented May 15, 2024

I'm not sure if Wouter has a Signed-off-by policy for userspace nbd; but given that the Linux kernel has a policy on S-o-b and this interacts closely with the kernel nbd.ko, I personally like using S-o-b on all my commits in this project.

@ebblake
Copy link
Contributor

ebblake commented May 15, 2024

Overall, the changes look sane to me.

@RokerHRO
Copy link
Contributor Author

RokerHRO commented May 15, 2024

I would suggest breaking this into multiple commits (each patch should do one thing well, rather than doing multiple things mashed together); it makes it easier if someone wants to backport one change but not the other. But that's not a hard requirement. In this case, an obvious split would be: patch 1: clean up #include directives patch 2: consistent use of uint32_t/uint64_t

Good point. I'll try, if the #include dependencies would not change again by the removal of u64 & u32, because the header where they are defined are no longer necessary after the change. I'll see.

Btw, do github's merge requests work well with reset & rebase when I replace the one commit by two different commits?

@RokerHRO
Copy link
Contributor Author

RokerHRO commented May 15, 2024

I'm not sure if Wouter has a Signed-off-by policy for userspace nbd; but given that the Linux kernel has a policy on S-o-b and this interacts closely with the kernel nbd.ko, I personally like using S-o-b on all my commits in this project.

What is that?
I don't know either. But of course I can add such a Signed-off-by line in my commit(s) if desired or necessary.

@RokerHRO RokerHRO marked this pull request as draft May 17, 2024 14:45
@yoe
Copy link
Member

yoe commented Dec 20, 2024

This looks sane to me and is fine to merge, but it's still marked as draft :)

@RokerHRO
Copy link
Contributor Author

This looks sane to me and is fine to merge, but it's still marked as draft :)

I'll make a new PR ASAP, during holidays I'd say.

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.

3 participants