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. -- Why is it that all of the instruments seeking intelligent life in the universe are pointed away from Earth? _______________________________________________ systemd-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/systemd-devel
