[PATCH v2] mount: mount--move with mtab support fix
Roman Borisov
ext-roman.borisov at nokia.com
Tue Feb 22 14:10:57 UTC 2011
On 02/22/2011 04:01 PM, ext Rob Landley wrote:
> I thought Nokia was a subsidiary of Microsoft now? (Didn't all the
> Linux guys quit?)
>
> On 02/22/2011 05:51 AM, Roman Borisov wrote:
>> added new update_mtab_entry() function to update /etc/mtab when new mounted
>> entry is created by mount--move. In such case update_mtab_entry() is used in
>> mount.c/mount_it_now() function instead of addmntent().
>>
>> fixes: http://lists.busybox.net/pipermail/busybox/2011-January/074510.html
>>
>> v2: added #if ENABLE_FEATURE_MTAB_SUPPORT in functions declarations;
>> added ENABLE_FEATURE_CLEAN_UP condition check for cleaning
>
> If this is only ever used in one place, why does it live in libbb
> instead of in mount.c?
>
> The erase_mtab() function was shared by mount and umount. This isn't.
>
Thanks for historic tour; so update_mtab() must be moved to mount.c like
erase_mtab() must be moved to umount.c; Another way - to combine this
functionality to one function.
>> Signed-off-by: Roman Borisov<ext-roman.borisov at nokia.com>
>> ---
>> include/libbb.h | 3 ++
>> libbb/mtab.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>> util-linux/mount.c | 6 +++-
>> 3 files changed, 61 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/libbb.h b/include/libbb.h
>> index 587a5f9..7cbe0a1 100644
>> --- a/include/libbb.h
>> +++ b/include/libbb.h
>> @@ -1099,7 +1099,10 @@ extern void run_applet_no_and_exit(int a, char **argv) NORETURN FAST_FUNC;
>> extern int match_fstype(const struct mntent *mt, const char *fstypes) FAST_FUNC;
>> extern struct mntent *find_mount_point(const char *name, int subdir_too) FAST_FUNC;
>> #endif
>> +#if ENABLE_FEATURE_MTAB_SUPPORT
>> extern void erase_mtab(const char * name) FAST_FUNC;
>> +extern void update_mtab_entry(const struct mntent *mp) FAST_FUNC;
>> +#endif
>
> No, the prototype doesn't get #ifdefed. This is so you can
> conditionally call the code in something like:
>
> if (ENABLE_FEATURE_MTAB_SUPPORT) erase_mtab(blah);
>
> And then the compiler's dead code elimination deletes the reference
> (this is why we have function-secitons and gc-sections in the compiler),
> but the code is always syntax checked the same way and changing the
> config doesn't introduce build breaks.
>
Thanks, I tried to change something after the comment on initial patch.
Please review "[PATCH] mount: mount--move with mtab support fix". It
doesn't contain "#if ENABLE_FEATURE_MTAB_SUPPORT".
>> extern unsigned int tty_baud_to_value(speed_t speed) FAST_FUNC;
>> extern speed_t tty_value_to_baud(unsigned int value) FAST_FUNC;
>> #if ENABLE_DESKTOP
>> diff --git a/libbb/mtab.c b/libbb/mtab.c
>> index 22bff64..3af9379 100644
>> --- a/libbb/mtab.c
>> +++ b/libbb/mtab.c
>> @@ -52,4 +52,58 @@ void FAST_FUNC erase_mtab(const char *name)
>> } else if (errno != EROFS)
>> bb_perror_msg(bb_path_mtab_file);
>> }
>> +
>> +/*
>> +update_mtab_entry() is used to update entry in case of mount--move.
>> +we are looking for existing entries mnt_dir which is equal to mnt_fsname of
>> +input mntent and replace it by new one.
>> +*/
>> +void FAST_FUNC update_mtab_entry(const struct mntent *mp)
>> +{
>> + struct mntent *entries, *m;
>> + int i, count;
>> + FILE *mountTable;
>> +
>> + mountTable = setmntent(bb_path_mtab_file, "r");
>> + if (!mountTable) {
>> + bb_perror_msg(bb_path_mtab_file);
>> + return;
>> + }
>> +
>> + entries = NULL;
>> + count = 0;
>> + while ((m = getmntent(mountTable)) != 0) {
>> + entries = xrealloc_vector(entries, 3, count);
>> + entries[count].mnt_fsname = xstrdup(m->mnt_fsname);
>> + entries[count].mnt_dir = xstrdup(m->mnt_dir);
>> + entries[count].mnt_type = xstrdup(m->mnt_type);
>> + entries[count].mnt_opts = xstrdup(m->mnt_opts);
>> + entries[count].mnt_freq = m->mnt_freq;
>> + entries[count].mnt_passno = m->mnt_passno;
>> + count++;
>> + }
>> + endmntent(mountTable);
>> +
>> + mountTable = setmntent(bb_path_mtab_file, "w");
>> + if (mountTable) {
>> + for (i = 0; i< count; i++) {
>> + if (strcmp(entries[i].mnt_dir, mp->mnt_fsname) != 0)
>> + addmntent(mountTable,&entries[i]);
>> + else
>> + addmntent(mountTable, mp);
>> + }
>> + endmntent(mountTable);
>> + } else if (errno != EROFS)
>> + bb_perror_msg(bb_path_mtab_file);
>> +
>> + if (ENABLE_FEATURE_CLEAN_UP) {
>> + for (i = 0; i< count; i++) {
>> + free(entries[i].mnt_fsname);
>> + free(entries[i].mnt_dir);
>> + free(entries[i].mnt_type);
>> + free(entries[i].mnt_opts);
>> + }
>> + }
>> +}
>> #endif
>
> There is still a situation where symlinking /etc/mtab to /proc/mounts
> isn't the right thing to do? All these years after the introduction of
> shared subtrees?
>
> Isn't the /etc/mtab support broken legacy crap, to which you're patching
> a single weird corner case out of the 30 different ways it's broken at
> the _design_ level?
>
We suppose that all code distributed must be working. Do you think to
exclude --move and --bind functionality in case of MTAB_SUPPORT? Ok.
Let's return proper error code or/and document it.
Maybe I didn't understand you clearly. If so please rephrase.
>> diff --git a/util-linux/mount.c b/util-linux/mount.c
>> index 0f213bb..c0c828b 100644
>> --- a/util-linux/mount.c
>> +++ b/util-linux/mount.c
>> @@ -524,8 +524,10 @@ static int mount_it_now(struct mntent *mp, long vfsflags, char *filteropts)
>> mp->mnt_freq = mp->mnt_passno = 0;
>>
>> // Write and close.
>> -
>> - addmntent(mountTable, mp);
>> + if (vfsflags& MS_MOVE)
>> + update_mtab_entry(mp);
>> + else
>> + addmntent(mountTable, mp);
>> endmntent(mountTable);
>> if (ENABLE_FEATURE_CLEAN_UP) {
>> free(mp->mnt_dir);
>
> Oh _wow_ the mount code has code's gotten ugly. Dunno why I'm
> bothering, but:
>
> If you look at this bit up near the top:
>
> #if ENABLE_FEATURE_MTAB_SUPPORT
> #define USE_MTAB (!(option_mask32& OPT_n))
> #else
> #define USE_MTAB 0
> #endif
>
> And then notice this chunk:
>
> mtab:
> if (USE_MTAB&& !rc&& !(vfsflags& MS_REMOUNT)) {
> char *fsname;
>
> (Don't ask me why the goto label is indented away from the left edge
> like Kernighan intended.)
>
> But anyway, the point is this is a larger version of the if (ENABLE)
> trick above, meaning the prototype is going to be used to verify the
> function call all the time, but this code will be if (0) and the
> compiler will eliminiate it via dead code elimination in certain config
> situations, due to the comparison against a compile-time constant.
>
> By adding an #ifdef around the prototype, you introduce a compiler
> warning when mtab is configured out (I.E. "the normal case"), for no
> obvious reason. if (0) has_no_prototype();
>
You are absolutely right about wrong ENABLE_FEATURE_MTAB_SUPPORT using.
Thanks. Could you please review the first version of patch?
Also what do you think is it really needed to check
ENABLE_FEATURE_CLEAN_UP before cleaning?
> Rob
>
Roman
More information about the busybox
mailing list