* pivot_root - wrong check on mount(2) @ 2020-11-26 0:01 Davide Giorgio 2020-11-26 9:31 ` Michael Kerrisk (man-pages) 0 siblings, 1 reply; 6+ messages in thread From: Davide Giorgio @ 2020-11-26 0:01 UTC (permalink / raw) To: mtk.manpages; +Cc: linux-man [-- 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 --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: pivot_root - wrong check on mount(2) 2020-11-26 0:01 pivot_root - wrong check on mount(2) Davide Giorgio @ 2020-11-26 9:31 ` Michael Kerrisk (man-pages) 2020-11-26 12:28 ` Alejandro Colomar (mailing lists; readonly) 0 siblings, 1 reply; 6+ messages in thread From: Michael Kerrisk (man-pages) @ 2020-11-26 9:31 UTC (permalink / raw) To: Davide Giorgio; +Cc: linux-man 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/ ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: pivot_root - wrong check on mount(2) 2020-11-26 9:31 ` Michael Kerrisk (man-pages) @ 2020-11-26 12:28 ` Alejandro Colomar (mailing lists; readonly) 2020-11-26 13:40 ` Alejandro Colomar (man-pages) 2020-11-27 8:30 ` Michael Kerrisk (man-pages) 0 siblings, 2 replies; 6+ messages in thread From: Alejandro Colomar (mailing lists; readonly) @ 2020-11-26 12:28 UTC (permalink / raw) To: mtk.manpages, Davide Giorgio; +Cc: linux-man 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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: pivot_root - wrong check on mount(2) 2020-11-26 12:28 ` Alejandro Colomar (mailing lists; readonly) @ 2020-11-26 13:40 ` Alejandro Colomar (man-pages) 2020-11-27 8:30 ` Michael Kerrisk (man-pages) 1 sibling, 0 replies; 6+ messages in thread From: Alejandro Colomar (man-pages) @ 2020-11-26 13:40 UTC (permalink / raw) To: mtk.manpages, Davide Giorgio; +Cc: linux-man 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 >> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: pivot_root - wrong check on mount(2) 2020-11-26 12:28 ` Alejandro Colomar (mailing lists; readonly) 2020-11-26 13:40 ` Alejandro Colomar (man-pages) @ 2020-11-27 8:30 ` Michael Kerrisk (man-pages) 2020-11-27 8:45 ` Alejandro Colomar (man-pages) 1 sibling, 1 reply; 6+ messages in thread From: Michael Kerrisk (man-pages) @ 2020-11-27 8:30 UTC (permalink / raw) To: Alejandro Colomar (mailing lists, readonly), Davide Giorgio Cc: mtk.manpages, linux-man 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/ ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: pivot_root - wrong check on mount(2) 2020-11-27 8:30 ` Michael Kerrisk (man-pages) @ 2020-11-27 8:45 ` Alejandro Colomar (man-pages) 0 siblings, 0 replies; 6+ messages in thread From: Alejandro Colomar (man-pages) @ 2020-11-27 8:45 UTC (permalink / raw) To: Michael Kerrisk (man-pages), Davide Giorgio; +Cc: linux-man 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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-11-27 8:45 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-11-26 0:01 pivot_root - wrong check on mount(2) Davide Giorgio 2020-11-26 9:31 ` Michael Kerrisk (man-pages) 2020-11-26 12:28 ` Alejandro Colomar (mailing lists; readonly) 2020-11-26 13:40 ` Alejandro Colomar (man-pages) 2020-11-27 8:30 ` Michael Kerrisk (man-pages) 2020-11-27 8:45 ` Alejandro Colomar (man-pages)
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).