lib/vfscore: Fix warnings in automount.c #1398
Conversation
- remove const from path in vfscore_mount_volume - use the device name for the message in uk_list_for_each_entry_reverse Signed-off-by: Radu Coriu <radu.gabrielc@gmail.com>
github-actions
bot
added
area/lib
lib/vfscore/automount.c
Outdated
| @@ -625,7 +628,7 @@ static int vfscore_ukopt_mkmp(char *path) | |||
| */ | |||
| static inline int vfscore_mount_volume(const struct vfscore_volume *vv) | |||
| { | |||
| const char *path; | |||
| char *path; | |||
There was a problem hiding this comment.
What is the issue with const here? The path was treated as const on purpose because struct vfscore_volume is handed over as const as well so that we can support a const compiled-in filesystem table.
There was a problem hiding this comment.
The path string is actually modified in vfscore_ukopt_mkmp at line 686. I initially wanted to strdup vv->path because of that constness promise in the input param, but after talking to @StefanJum we decided to only remove const.
Now I see that vv->path is also used at line 693 in vfscore_extract_volume, so don't know if a strdup would actually be ok and how the LIBVFSCORE_UKOPT_MKMP flag should interact with CONFIG_LIBUKCPIO
There was a problem hiding this comment.
Hum..., right. The proper patch should do strdup and free within vfscore_ukopt_mkmp() and path should be handed over as const. Because we also compile in paths, these are on purpose on read-only memory, any modification would lead to a crash (that's why also the const) at the same time, exporting handling of a dupped string allocation is typically making interfaces more complicated to use correctly.
There was a problem hiding this comment.
@skuenzer I don't really understand why you're saying that we shoud do strdup and free within vfscore_ukopt_mkmp(). For vfscore_ukopt_mkmp it is an in/out parameter (as it's uses further down inside mount at line 695/698)
so we should do:
` if (vv->ukopts & LIBVFSCORE_UKOPT_MKMP) {
char *tmpPath = strdup(path);
rc = vfscore_ukopt_mkmp(tmpPath);
if (unlikely(rc < 0))
{
free(tmpPath);
return rc;
}
path = tmpPath;
}
// plus some extra code to properly free before the last return in case path was strduped
Another option would be to make the existing parameterconstand add an out parameter for thevfscore_ukopt_mkmp`, but that wouldn't make things any nicer IMHO.
Let me know if I'm missing something.
There was a problem hiding this comment.
Nevermind, I noticed just now that the function actually restores the replaced slashes. I will do as you suggested and also make the param const.
- make path const again in vfscore_mount_volume - make path param const in vfscore_ukopt_mkmp and use strdup/free to avoid writing to readonly input string Signed-off-by: Radu Coriu <radu.gabrielc@gmail.com>
Signed-off-by: Radu Coriu <radu.gabrielc@gmail.com>
Prerequisite checklist
checkpatch.ukon your commit series before opening this PR;Base target
Additional configuration
Description of changes