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

lib/posix-sysinfo: Provide _SC_GETPW_R_SIZE_MAX #936

Closed
wants to merge 2 commits into from

Conversation

andreittr
Copy link
Contributor

Description of changes

Calling code expects sysconf(_SC_GETPW_R_SIZE_MAX) to be either -1 or a non-zero value for an initial buffer size. The previous default value of 0 is outside of specifications and can cause unintended behavior. This change makes sysconf(_SC_GETPW_R_SIZE_MAX) return -1.

See man 3 getpwuid for the specifications for sysconf(_SC_GETPW_R_SIZE_MAX).

Prerequisite checklist

  • Read the contribution guidelines regarding submitting new changes to the project;
  • Tested your changes against relevant architectures and platforms;
  • Ran the checkpatch.uk on your commit series before opening this PR;
  • Updated relevant documentation.

Base target

  • Architecture(s): N/A
  • Platform(s): N/A
  • Application(s): N/A

Additional configuration

CONFIG_LIBPOSIX_USER=y and CONFIG_LIBPOSIX_SYSINFO=y.

@andreittr andreittr requested a review from a team as a code owner June 7, 2023 11:30
@unikraft-bot
Copy link
Member

Checkpatch passed

Beep boop! I ran Unikraft's checkpatch.pl support script on your pull request and it all looks good!

SHA commit checkpatch
5eecf9c lib/posix-sysinfo: Provide _SC_GETPW_R_SIZE_MAX

@unikraft-bot unikraft-bot added area/lib Internal Unikraft Microlibrary lang/c Issues or PRs to do with C/C++ lib/posix-sysinfo labels Jun 7, 2023
@razvand razvand assigned razvand and unassigned nderjung Jun 15, 2023
@razvand razvand requested review from John-Ted and StefanJum and removed request for a team and dragosargint June 15, 2023 16:14
@razvand razvand added this to the v0.14.0 (Prometheus) milestone Jun 15, 2023
Copy link
Member

@StefanJum StefanJum left a comment

Choose a reason for hiding this comment

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

@andreittr we should define _SC_GETPW_R_SIZE_MAX in nolibc/include/unistd.h, otherwise the build fails when using nolibc.

Calling code expects sysconf(_SC_GETPW_R_SIZE_MAX) to be either -1 or a
non-zero value for an initial buffer size. The previous default value of
0 is outside of specifications and can cause unintended behavior.
This change makes sysconf(_SC_GETPW_R_SIZE_MAX) return -1.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
@andreittr
Copy link
Contributor Author

Update: added define of _SC_GETPW_R_SIZE_MAX to nolibc.

Copy link
Member

@StefanJum StefanJum left a comment

Choose a reason for hiding this comment

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

Looks good now. I'll add the tag after @John-Ted and @razvand take a look.

@razvand
Copy link
Contributor

razvand commented Jul 24, 2023

@John-Ted , please take a look, so I can then approve it and have it merged.

@John-Ted
Copy link
Contributor

I'm trying to compile the helloworld app to print the sysconf value, but I cannot get it to compile. I have selected the 2 options in the config, and I included unistd.h. I get the following errors:

  CC      apphelloworld: main.o
/home/teodor/Documents/ac/unikraft/pr_reviews/rev1/apps/app-helloworld/main.c: In function ‘main’:
/home/teodor/Documents/ac/unikraft/pr_reviews/rev1/apps/app-helloworld/main.c:34:37: warning: implicit declaration of function ‘sysconf’; did you mean ‘sscanf’? [-Wimplicit-function-declaration]
   34 |         printf("Hello world %d!\n", sysconf(_SC_GETPW_R_SIZE_MAX));
      |                                     ^~~~~~~
      |                                     sscanf
/home/teodor/Documents/ac/unikraft/pr_reviews/rev1/apps/app-helloworld/main.c:34:45: error: ‘_SC_GETPW_R_SIZE_MAX’ undeclared (first use in this function)
   34 |         printf("Hello world %d!\n", sysconf(_SC_GETPW_R_SIZE_MAX));
      |                                             ^~~~~~~~~~~~~~~~~~~~
/home/teodor/Documents/ac/unikraft/pr_reviews/rev1/apps/app-helloworld/main.c:34:45: note: each undeclared identifier is reported only once for each function it appears in
make[3]: *** [/home/teodor/Documents/ac/unikraft/pr_reviews/rev1/apps/app-helloworld/.unikraft/unikraft/support/build/Makefile.build:27: /home/teodor/Documents/ac/unikraft/pr_reviews/rev1/apps/app-helloworld/build/apphelloworld/main.o] Error 1

config.txt
main.c.txt

@andreittr
Copy link
Contributor Author

I'm trying to compile the helloworld app to print the sysconf value, but I cannot get it to compile. I have selected the 2 options in the config, and I included unistd.h. I get the following errors:

Whoops, seems sysconf is not declared in nolibc's unistd.h; will push a new commit with a fix.

This change adds a missing declaration of `sysconf()` in the unistd.h
header provided by nolibc, complementing the _SC_* constants.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
Copy link
Contributor

@John-Ted John-Ted left a comment

Choose a reason for hiding this comment

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

Everything works fine now. Thanks!

Reviewed-by: Ioan-Teodor Teugea ioan_teodor.teugea@stud.acs.upb.ro

Copy link
Contributor

@razvand razvand left a comment

Choose a reason for hiding this comment

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

Approved-by: Razvan Deaconescu razvand@unikraft.io

unikraft-bot pushed a commit that referenced this pull request Aug 7, 2023
This change adds a missing declaration of `sysconf()` in the unistd.h
header provided by nolibc, complementing the _SC_* constants.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
Reviewed-by: Ioan-Teodor Teugea <ioan_teodor.teugea@stud.acs.upb.ro>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #936
@unikraft-bot unikraft-bot added the ci/merged Merged by CI label Aug 7, 2023
@andreittr andreittr deleted the ttr/pwsize-sysconf branch August 7, 2023 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/lib Internal Unikraft Microlibrary ci/merged Merged by CI lang/c Issues or PRs to do with C/C++ lib/posix-sysinfo
Projects
Status: Done!
Development

Successfully merging this pull request may close these issues.

None yet

6 participants