[systemd-devel] Failure to remount / ro (Was: Re: [PATCH 2/4] Adding unmount functions to be used in shutdown)

[Gustavo Sverzut Barbieri]

On Sun, Oct 24, 2010 at 2:17 PM, Michael Biebl <mbiebl at gmail.com> wrote:
> 2010/10/24 Gustavo Sverzut Barbieri <barbieri at profusion.mobi>:
>> On Sun, Oct 24, 2010 at 2:01 PM, Michael Biebl <mbiebl at gmail.com> wrote:
>>> 2010/10/24 Gustavo Sverzut Barbieri <barbieri at profusion.mobi>:
>>>> On Sun, Oct 24, 2010 at 12:41 PM, Michael Biebl <mbiebl at gmail.com> 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  <fidencio at profusion.mobi>:
>>>>>> +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: barbieri at gmail.com
Skype: gsbarbieri
Mobile: +55 (19) 9225-2202