Re: pivot_root - wrong check on mount(2)

["Alejandro Colomar (man-pages)" <alx.manpages@gmail.com>]

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
>>