On Sun, Oct 24, 2010 at 2:17 PM, Michael Biebl <[email protected]> wrote: > 2010/10/24 Gustavo Sverzut Barbieri <[email protected]>: >> On Sun, Oct 24, 2010 at 2:01 PM, Michael Biebl <[email protected]> wrote: >>> 2010/10/24 Gustavo Sverzut Barbieri <[email protected]>: >>>> On Sun, Oct 24, 2010 at 12:41 PM, Michael Biebl <[email protected]> wrote: >>>>> Together with fidencio I tracked down the bug which caused / *not* to >>>>> be remounted ro on shutdown. >>>>> >>>>> The relevant code is in src/umount.c >>>>> >>>>> 2010/10/7 <[email protected]>: >>>>>> +int umount_all(void) { >>>>>> + int r; >>>>>> + LIST_HEAD(MountPoint, mp_list_head); >>>>>> + >>>>>> + LIST_HEAD_INIT(MountPoint, mp_list_head); >>>>>> + >>>>>> + r = mount_points_list_get(&mp_list_head); >>>>>> + if (r < 0) >>>>>> + goto end; >>>>>> + >>>>>> + r = mount_points_list_umount(&mp_list_head); >>>>>> + if (r <= 0) >>>>>> + goto end; >>>>> >>>>> r is the number of mount points which failed to be unmounted. >>>>> We always skip / in mount_points_list_umount though, >>>>> I.e. if there is no failed unmount attempt, we will never try to remount >>>>> / ro. >>>>> >>>>> Maybe we should just call mount_points_list_remount_read_only >>>>> unconditionally. >>>>> or increment the failed counter in mount_points_list_umount if we skip "/" >>>> >>>> I guess either we run it unconditionally or we just call the remount >>>> of / to "ro" explicitly if r == 0 (clear I guess) >>>> >>>> >>>> >>>>> +static int mount_points_list_umount(MountPoint **mount_point_list_head) { >>>>> + MountPoint *mp, *mp_next; >>>>> + int failed = 0; >>>>> + >>>>> + LIST_FOREACH_SAFE(mount_point, mp, mp_next, >>>>> *mount_point_list_head) { >>>>> + if (streq(mp->path, "/")) >>>>> + continue; >>>>> ^ add failed++ to make sure / is remounted-ro? >>>> >>>> it would be a bit misleading. >>> >>> Well, not really. Skipping / is just an optimisation as we know that >>> unmounting it would fail anyway. >> >> sure, but I guess just making it explicit on the caller side is good >> as well, instead of returning a failed state. We can also more easily >> know what to return to main loop as r == 0 -> remount / ro -> return >> nothing else to do. > > If r == 0, it can also mean though, that "/" was not in > **mount_point_list_head, which makes the return code of > mount_points_list_umount ambiguous. > > Also, incremeting n_failed is a very simple fix, whereas handling it > it umount_all() adds more special cases and duplication.
well, just do the patch, test to see if you handled it right (ie: you're not looping forever in code) and mail, Lennart might like that option you propose. -- Gustavo Sverzut Barbieri http://profusion.mobi embedded systems -------------------------------------- MSN: [email protected] Skype: gsbarbieri Mobile: +55 (19) 9225-2202 _______________________________________________ systemd-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/systemd-devel
