Skip to content

Fix retrieve user value in IOCTL_VALSET_NUM case#359

Merged
jserv merged 1 commit intosysprog21:masterfrom
applepwc:fix
Mar 1, 2026
Merged

Fix retrieve user value in IOCTL_VALSET_NUM case#359
jserv merged 1 commit intosysprog21:masterfrom
applepwc:fix

Conversation

@applepwc
Copy link
Contributor

@applepwc applepwc commented Feb 10, 2026

The test program is:

    arg.val = 0xAA;
    ioctl(fd, IOCTL_VALSET_NUM, &arg);

    arg.val = 0;
    ioctl(fd, IOCTL_VALGET_NUM, &arg);
    printf("Read val: 0x%x\n", arg.val);

Summary by cubic

Switch IOCTL_VALSET_NUM to _IO so the integer is passed via the ioctl arg, matching the driver handler and example calls. This fixes the set/get number flow and keeps the convention consistent across the codebase.

Written for commit 113cd57. Summary will update on new commits.

Copy link

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

No issues found across 1 file

@EricccTaiwan EricccTaiwan requested a review from jserv February 10, 2026 13:32
Copy link
Contributor

@jserv jserv left a comment

Choose a reason for hiding this comment

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

Remove "fix: " in the subject of git commit messages.
Refine git commit message, making it more informative. See https://chris.beams.io/git-commit
Don't close this pull request. Use git force-push instead.

examples/ioctl.c Outdated

case IOCTL_VALSET_NUM:
ioctl_num = arg;
int user_val;
Copy link
Collaborator

Choose a reason for hiding this comment

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

To be compatible with the old version, which might still use the C89 standard.
Don't declare the variable inside the switch statement.
Or, you should declare the variable within the block scope.

Also, if you want to declare the variable inside the switch statement, please fix this:

ioctl.c:78:9: error: typename in expression
ioctl.c:78:13: error: Expected ; at end of statement
ioctl.c:78:13: error: got user_val
ioctl.c:78:9: error: undefined identifier 'int'
ioctl.c:79:13: error: undefined identifier 'user_val'
ioctl.c:81:21: error: undefined identifier 'user_val'

@applepwc
Copy link
Contributor Author

Hello, I force-pushed a new commit. Please review it.

@visitorckw
Copy link
Collaborator

Hello, I force-pushed a new commit. Please review it.

Github notifies the relevant people automatically, so there's no need to leave a comment like this.

@visitorckw
Copy link
Collaborator

Thanks for the patch.

While this addresses the pointer-to-value mismatch, should we consider whether IOCTL_VALSET_NUM is better suited for "pass-by-value" rather than "pass-by-reference"?

Currently, the macro is defined as:

#define IOCTL_VALSET_NUM _IOW(IOC_MAGIC, 3, int)

Using _IOW implies that arg is a user-space pointer. However, for a simple integer assignment, would it be more idiomatic to use pass-by-value by redefining the macro with _IO? This would allow us to keep the logic lightweight without the overhead of memory copying.

Copy link
Contributor

@jserv jserv left a comment

Choose a reason for hiding this comment

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

Ensure consistent code style with 'clang-format' in examples directory.

@jserv jserv changed the title fix: Retrieve user value in IOCTL_VALSET_NUM case Fix retrieve user value in IOCTL_VALSET_NUM case Feb 13, 2026
@applepwc
Copy link
Contributor Author

Yes, using _IO with pass-by-value is more idiomatic and avoids unnecessary memory copying.
But this would be an ABI change: existing user-space code calling ioctl(fd, IOCTL_VALSET_NUM, &value) would break.

@visitorckw
Copy link
Collaborator

Wouldn't the proposed patch also change the existing ABI?

@applepwc
Copy link
Contributor Author

Hello, I'm uncertain whether to add more code as shown below or simply rewrite the original.

#define IOCTL_VALSET_NUM2 _IO(IOC_MAGIC, 4)
#define IOCTL_VAL_MAXNR 4
...
case IOCTL_VALSET_NUM2:
    ioctl_num = (int)arg; 
    break;

@linD026
Copy link
Collaborator

linD026 commented Feb 19, 2026

Based on the userspace_ioctl.c:

int ioctl_get_nth_byte(int file_desc)  
{  
    int i, c; 

    printf("get_nth_byte message:");
    i = 0;  
    do {  
        c = ioctl(file_desc, IOCTL_GET_NTH_BYTE, i++);
    /* ... */

The test program in the LKMPG only passes the value to the ioctl.
I don't think this fix aligns with the existing testing program.

Please either change the test program or leave the ioctl logic unchanged.

@applepwc applepwc force-pushed the fix branch 2 times, most recently from 766280b to 68fc022 Compare February 20, 2026 06:40
Replace _IOW with _IO macro to reflect that IOCTL_VALSET_NUM passes
the integer value directly via arg, consistent with the calling
convention used throughout this codebase.
@jserv jserv merged commit f8c730d into sysprog21:master Mar 1, 2026
1 check passed
@jserv
Copy link
Contributor

jserv commented Mar 1, 2026

Thank @applepwc for contributing!

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.

5 participants