pivot_root - wrong check on mount(2)

pivot_root - wrong check on mount(2)

[Davide Giorgio]

[-- Attachment #1.1.1: Type: text/plain, Size: 595 bytes --]

Good morning,

reading the pivot_root man page
(https://man7.org/linux/man-pages/man2/pivot_root.2.html)
there seems to be an error in the example source program
"pivot_root_demo.c".
In particular, there is a wrong check on the return value of mount(2).
https://man7.org/linux/man-pages/man2/mount.2.html#RETURN_VALUE

The error is in this line
if (mount(NULL, "/", NULL, MS_REC | MS_PRIVATE, NULL) == 1)

that should be
if (mount(NULL, "/", NULL, MS_REC | MS_PRIVATE, NULL) == -1)


Thank you for your work, kind regards
-- 
Davide Antonino Giorgio
http://giorgiodavide.it

[-- Attachment #1.1.2: 0x4BAA68CE23187566.asc --]
[-- Type: application/pgp-keys, Size: 2505 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: pivot_root - wrong check on mount(2)

[Michael Kerrisk (man-pages)]

Hello Davide,

On Thu, 26 Nov 2020 at 01:01, Davide Giorgio <davide@giorgiodavide.it> wrote:
>
> Good morning,
>
> reading the pivot_root man page
> (https://man7.org/linux/man-pages/man2/pivot_root.2.html)
> there seems to be an error in the example source program
> "pivot_root_demo.c".
> In particular, there is a wrong check on the return value of mount(2).
> https://man7.org/linux/man-pages/man2/mount.2.html#RETURN_VALUE
>
> The error is in this line
> if (mount(NULL, "/", NULL, MS_REC | MS_PRIVATE, NULL) == 1)
>
> that should be
> if (mount(NULL, "/", NULL, MS_REC | MS_PRIVATE, NULL) == -1)
>
>
> Thank you for your work, kind regards

Thanks! Fixed!

Cheers,

Michael

-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: pivot_root - wrong check on mount(2)

[Alejandro Colomar (mailing lists; readonly)]

On 11/26/20 10:31 AM, Michael Kerrisk (man-pages) wrote:
> Hello Davide,
> 
> On Thu, 26 Nov 2020 at 01:01, Davide Giorgio <davide@giorgiodavide.it> wrote:
>>
>> Good morning,
>>
>> reading the pivot_root man page
>> (https://man7.org/linux/man-pages/man2/pivot_root.2.html)
>> there seems to be an error in the example source program
>> "pivot_root_demo.c".
>> In particular, there is a wrong check on the return value of mount(2).
>> https://man7.org/linux/man-pages/man2/mount.2.html#RETURN_VALUE
>>
>> The error is in this line
>> if (mount(NULL, "/", NULL, MS_REC | MS_PRIVATE, NULL) == 1)
>>
>> that should be
>> if (mount(NULL, "/", NULL, MS_REC | MS_PRIVATE, NULL) == -1)
>>
>>
>> Thank you for your work, kind regards
> 
> Thanks! Fixed!

Hi Michael,

What about fixing this from a different approach:

instead of comparing against -1
for functions that either return either 0 or -1,
we can include those functions in the greater family of
functions that return either 0 or non-zero (error code).
I propose comparing against 0:

- if (mount(NULL, "/", NULL, MS_REC | MS_PRIVATE, NULL) == 1)
+ if (mount(NULL, "/", NULL, MS_REC | MS_PRIVATE, NULL) != 0)

I consider this to be safer, simpler,
and although negligible, also faster.

What are your thoughts?

Thanks,

Alex

> 
> Cheers,
> 
> Michael
> 

Re: pivot_root - wrong check on mount(2)

[Alejandro Colomar (man-pages)]

Hi Michel,

Even more generic:

Most--if not all--functions can be catalogued as one of the following:

   __________________________
   Success      |       Error
   --------------------------
A)  0	        |   non-zero
B)  0           |   negative  (the intersection of A and C)
C)  non-negative|   negative
D)  non-zero    |       NULL
D)  true        |      false

For error checking, I'd use:

A) (ret != 0)  [or simply (ret)]
B) (ret < 0)
C) (ret < 0)
D) (ret == NULL)  [or simply (!ret)]
E) (!ret)

This way you avoid any magic numbers such as '-1' for the error code,
and it's more difficult to introduce bugs.
Also, CPUs usually compare faster against zero, AFAIK.


Cheers,

Alex

On 11/26/20 1:28 PM, Alejandro Colomar (mailing lists; readonly) wrote:
> 
> 
> On 11/26/20 10:31 AM, Michael Kerrisk (man-pages) wrote:
>> Hello Davide,
>>
>> On Thu, 26 Nov 2020 at 01:01, Davide Giorgio <davide@giorgiodavide.it> wrote:
>>>
>>> Good morning,
>>>
>>> reading the pivot_root man page
>>> (https://man7.org/linux/man-pages/man2/pivot_root.2.html)
>>> there seems to be an error in the example source program
>>> "pivot_root_demo.c".
>>> In particular, there is a wrong check on the return value of mount(2).
>>> https://man7.org/linux/man-pages/man2/mount.2.html#RETURN_VALUE
>>>
>>> The error is in this line
>>> if (mount(NULL, "/", NULL, MS_REC | MS_PRIVATE, NULL) == 1)
>>>
>>> that should be
>>> if (mount(NULL, "/", NULL, MS_REC | MS_PRIVATE, NULL) == -1)
>>>
>>>
>>> Thank you for your work, kind regards
>>
>> Thanks! Fixed!
> 
> Hi Michael,
> 
> What about fixing this from a different approach:
> 
> instead of comparing against -1
> for functions that either return either 0 or -1,
> we can include those functions in the greater family of
> functions that return either 0 or non-zero (error code).
> I propose comparing against 0:
> 
> - if (mount(NULL, "/", NULL, MS_REC | MS_PRIVATE, NULL) == 1)
> + if (mount(NULL, "/", NULL, MS_REC | MS_PRIVATE, NULL) != 0)
> 
> I consider this to be safer, simpler,
> and although negligible, also faster.
> 
> What are your thoughts?
> 
> Thanks,
> 
> Alex
> 
>>
>> Cheers,
>>
>> Michael
>>

Re: pivot_root - wrong check on mount(2)

[Michael Kerrisk (man-pages)]

Hi Alex,

On 11/26/20 1:28 PM, Alejandro Colomar (mailing lists; readonly) wrote:
> 
> 
> On 11/26/20 10:31 AM, Michael Kerrisk (man-pages) wrote:
>> Hello Davide,
>>
>> On Thu, 26 Nov 2020 at 01:01, Davide Giorgio <davide@giorgiodavide.it> wrote:
>>>
>>> Good morning,
>>>
>>> reading the pivot_root man page
>>> (https://man7.org/linux/man-pages/man2/pivot_root.2.html)
>>> there seems to be an error in the example source program
>>> "pivot_root_demo.c".
>>> In particular, there is a wrong check on the return value of mount(2).
>>> https://man7.org/linux/man-pages/man2/mount.2.html#RETURN_VALUE
>>>
>>> The error is in this line
>>> if (mount(NULL, "/", NULL, MS_REC | MS_PRIVATE, NULL) == 1)
>>>
>>> that should be
>>> if (mount(NULL, "/", NULL, MS_REC | MS_PRIVATE, NULL) == -1)
>>>
>>>
>>> Thank you for your work, kind regards
>>
>> Thanks! Fixed!
> 
> Hi Michael,
> 
> What about fixing this from a different approach:
> 
> instead of comparing against -1
> for functions that either return either 0 or -1,
> we can include those functions in the greater family of
> functions that return either 0 or non-zero (error code).
> I propose comparing against 0:
> 
> - if (mount(NULL, "/", NULL, MS_REC | MS_PRIVATE, NULL) == 1)
> + if (mount(NULL, "/", NULL, MS_REC | MS_PRIVATE, NULL) != 0)
> 
> I consider this to be safer, simpler,
> and although negligible, also faster.
> 
> What are your thoughts?

History and the standards say -1. (Okay, mount(2) is not in 
POSIX, but the statement is true for syscalls generally.) So, I
prefer to use -1 (and always do so in my own code.)

The check "ret != 0" does not work for system calls that 
return  a nonnegative value on success (e.g., open()).

The check "ret < 0" does not work for system calls that
can legitimately return a value less than zero on success.
(getpriority() is the most notable example, but there
are one or two other cases also.)

The check "ret == -1" is clear, and--in a standards
sense--precise. Though one must still be careful, since, 
for example, getpriority() can return -1 on success.
(See the manual page for info on how to fdeal with this.)

Thanks,

Michael

-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: pivot_root - wrong check on mount(2)

[Alejandro Colomar (man-pages)]

On 11/27/20 9:30 AM, Michael Kerrisk (man-pages) wrote:
> Hi Alex,
> 
> On 11/26/20 1:28 PM, Alejandro Colomar (mailing lists; readonly) wrote:
>>
>>
>> On 11/26/20 10:31 AM, Michael Kerrisk (man-pages) wrote:
>>> Hello Davide,
>>>
>>> On Thu, 26 Nov 2020 at 01:01, Davide Giorgio <davide@giorgiodavide.it> wrote:
>>>>
>>>> Good morning,
>>>>
>>>> reading the pivot_root man page
>>>> (https://man7.org/linux/man-pages/man2/pivot_root.2.html)
>>>> there seems to be an error in the example source program
>>>> "pivot_root_demo.c".
>>>> In particular, there is a wrong check on the return value of mount(2).
>>>> https://man7.org/linux/man-pages/man2/mount.2.html#RETURN_VALUE
>>>>
>>>> The error is in this line
>>>> if (mount(NULL, "/", NULL, MS_REC | MS_PRIVATE, NULL) == 1)
>>>>
>>>> that should be
>>>> if (mount(NULL, "/", NULL, MS_REC | MS_PRIVATE, NULL) == -1)
>>>>
>>>>
>>>> Thank you for your work, kind regards
>>>
>>> Thanks! Fixed!
>>
>> Hi Michael,
>>
>> What about fixing this from a different approach:
>>
>> instead of comparing against -1
>> for functions that either return either 0 or -1,
>> we can include those functions in the greater family of
>> functions that return either 0 or non-zero (error code).
>> I propose comparing against 0:
>>
>> - if (mount(NULL, "/", NULL, MS_REC | MS_PRIVATE, NULL) == 1)
>> + if (mount(NULL, "/", NULL, MS_REC | MS_PRIVATE, NULL) != 0)
>>
>> I consider this to be safer, simpler,
>> and although negligible, also faster.
>>
>> What are your thoughts?
> 
> History and the standards say -1. (Okay, mount(2) is not in 
> POSIX, but the statement is true for syscalls generally.) So, I
> prefer to use -1 (and always do so in my own code.)
> 
> The check "ret != 0" does not work for system calls that 
> return  a nonnegative value on success (e.g., open()).
> 
> The check "ret < 0" does not work for system calls that
> can legitimately return a value less than zero on success.
> (getpriority() is the most notable example, but there
> are one or two other cases also.)
> 
> The check "ret == -1" is clear, and--in a standards
> sense--precise. Though one must still be careful, since, 
> for example, getpriority() can return -1 on success.
> (See the manual page for info on how to fdeal with this.)


Hi Michael,

Hmmm, I understand.

Thanks,

Alex

> 
> Thanks,
> 
> Michael
>