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 ssize_t and off_t definitions from sys/types.h #37712

Merged
merged 2 commits into from
Aug 26, 2021

Conversation

de-nordic
Copy link
Collaborator

The PR contains two commits:

  • removing fs.h local definitions of off_t ans ssize_t,, and using sys/types.h instead;
  • fixing littlefs sample printk formatting.

Fixes: #37665

@github-actions github-actions bot added area: API Changes to public APIs area: Samples Samples labels Aug 16, 2021
(ent.type == FS_DIR_ENTRY_FILE) ? 'F' : 'D',
ent.size,
Copy link
Member

Choose a reason for hiding this comment

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

"%zu" instead

Comment on lines 76 to 77
printk("FAIL: mount id %llu at %s: %d\n",
(unsigned long long)mp->storage_dev, mp->mnt_point,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
printk("FAIL: mount id %llu at %s: %d\n",
(unsigned long long)mp->storage_dev, mp->mnt_point,
printk("FAIL: mount id %p at %s: %d\n",
mp->storage_dev, mp->mnt_point,

Copy link
Collaborator Author

@de-nordic de-nordic Aug 16, 2021

Choose a reason for hiding this comment

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

Everywhere else yes, but here no: the storage_dev for LittleFS, unfortunately, carries identifier of storage partition which is actually unsigned int:

unsigned int area_id = (uintptr_t)mountp->storage_dev;
,
.storage_dev = (void *)DT_FIXED_PARTITION_ID(FS_PARTITION(inst)), \

Copy link
Member

Choose a reason for hiding this comment

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

In that case:

Suggested change
printk("FAIL: mount id %llu at %s: %d\n",
(unsigned long long)mp->storage_dev, mp->mnt_point,
printk("FAIL: mount id %" PRIuPTR " at %s: %d\n",
(uintptr_t)mp->storage_dev, mp->mnt_point,

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree; C pointers should only be converted to integer types via[u]intptr_t casts.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@stephanosio Argh. The clang does not want to play nice with this. Will go back to (unsigned long long) as safe option and will mark this as TODO to fix when the clang gets older.

Copy link
Member

Choose a reason for hiding this comment

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

error: format specifies type 'unsigned int' but the argument has type 'uintptr_t' (aka 'unsigned long') [-Werror,-Wformat]

Hold on, something is not right there ... we should not be seeing that warning.

Copy link
Member

Choose a reason for hiding this comment

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

This goes back to #16645, where we defined __INTPTR_TYPE__ and __UINTPTR_TYPE__ as long int and long unsigned int, respectively. These are used by the GCC and Clang headers to typedef intptr_t and uintptr_t.

#undef __INTPTR_TYPE__
#undef __UINTPTR_TYPE__
#define __INTPTR_TYPE__ long int
#define __UINTPTR_TYPE__ long unsigned int

By default, x86-32 Clang defines __INTPTR_TYPE__ as int, but we override that to long int in zephyr_stdint.h, so our intptr_t becomes long int (aka. long), and the Clang inttypes.h for x86-32 still defines PRIuPTR as %u and not %lu, so we get this warning/error.

This is yet another type system-related bug and needs to be fixed. I will create an issue for this.

Meanwhile, please feel free to go with your previous implementation for now (would be nice to add a FIXME comment though).

samples/subsys/fs/littlefs/src/main.c Outdated Show resolved Hide resolved
samples/subsys/fs/littlefs/src/main.c Outdated Show resolved Hide resolved
samples/subsys/fs/littlefs/src/main.c Outdated Show resolved Hide resolved
@de-nordic de-nordic force-pushed the fix-fs-ssize-off-types branch from e73702c to 7693ab8 Compare August 16, 2021 11:57
@de-nordic de-nordic force-pushed the fix-fs-ssize-off-types branch 2 times, most recently from 013c26f to eb0871e Compare August 16, 2021 14:57
The sys/types.h definitions of ssize_t and off_t will be used.

Signed-off-by: Dominik Ermel <dominik.ermel@nordicsemi.no>
The formatting options, passed to the printk, caused warnings when
compiling for native_posix_64.

Signed-off-by: Dominik Ermel <dominik.ermel@nordicsemi.no>
@de-nordic de-nordic force-pushed the fix-fs-ssize-off-types branch from d2a900d to 798ea6b Compare August 19, 2021 13:13
@Laczen Laczen removed their request for review August 19, 2021 18:10
@cfriedt cfriedt merged commit d1c0eb1 into zephyrproject-rtos:main Aug 26, 2021
@de-nordic de-nordic deleted the fix-fs-ssize-off-types branch November 30, 2021 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

File system: wrong type for ssize_t in fs.h for CONFIG_ARCH_POSIX
8 participants