Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:117587
MIME-Version: 1.0
References: <CAPyj-LCbBfNgD6dfxTsn+SF3Vo2t-kiCStJcGRDqcqA=UXExoQ@mail.gmail.com>
In-Reply-To: <CAPyj-LCbBfNgD6dfxTsn+SF3Vo2t-kiCStJcGRDqcqA=UXExoQ@mail.gmail.com>
Date: Sun, 24 Apr 2022 16:25:04 +0200
Message-ID: <CAF+90c9fg_8r=Fab46_qrMDX2uG_mi9=FRJJ6Fmj8Y9MSc1org@mail.gmail.com>
To: Ilija Tovilo <tovilo.ilija@gmail.com>
Cc: PHP internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="000000000000c0e32f05dd673aca"
Subject: Re: [PHP-DEV] Change the values of the LOCK_* constants
From: nikita.ppv@gmail.com (Nikita Popov)

--000000000000c0e32f05dd673aca
Content-Type: text/plain; charset="UTF-8"

On Sun, Apr 24, 2022 at 12:14 PM Ilija Tovilo <tovilo.ilija@gmail.com>
wrote:

> Hi everyone
>
> The issue was raised that PHPs LOCK_* constants don't match the Unix
> LOCK_* constants.
> https://github.com/php/php-src/pull/8429
>
> // Unix
> #define LOCK_SH 1
> #define LOCK_EX 2
> #define LOCK_NB 4
> #define LOCK_UN 8
>
> // PHP
> #define PHP_LOCK_SH 1
> #define PHP_LOCK_EX 2
> #define PHP_LOCK_UN 3
> #define PHP_LOCK_NB 4
>
> Essentially, in PHPs binary representation UN doesn't get its own bit,
> but is instead represented as 0b11. I'm guessing the reasoning was
> that SH, EX and UN must not be combined, while they can all be
> combined with NB. This avoids additional error handling when multiple
> of those bits were to be set.
>
> However, this has a downside of making checking of bits harder and
> different from how you would do it in other languages.
> https://3v4l.org/41ebV
>
> We could update the PHP constants to match the Unix values of those
> constants. Unfortunately, there seems to be a not insignificant number
> of usages of flock with hard-coded integer values.
>
>
> https://sourcegraph.com/search?q=context:global+file:%5C.php%24+count:100000+flock%5C%28%5C%24%5Ba-zA-Z0-9_%5D%2B%2C+%5B0-9%5D%2B%5C%29&patternType=regexp
>
> (The regex engine of sourcegraph is flaky, but the majority of results
> are correct)
>
> The process of replacing these hard-coded values could be partially
> automated with a few caveats.
>
> 1. The value must be direct ($flags = 1; flock($file, $flags); would not
> work)
> 2. The migration script would assume that flock is a global and not
> local function
>
> Overall, I'm not completely sure this change is worth it since flock
> flags are just passed and not read.
>
> Let me know what you think.
>
> Ilija
>

I think the current state of things here makes perfect sense. I might help
to think of it as a structure of the form:

struct {
    unsigned lock_type : 2;
    unsigned non_blocking : 1;
}

The first member of that structure is not a bitmask -- the three options
are mutually exclusive, and doing something like LOCK_SH | LOCK_UN is
semantically meaningless.

Consulting https://man7.org/linux/man-pages/man2/flock.2.html, nothing on
the flock() man page suggests that LOCK_SH, LOCK_EX and LOCK_UN can be used
as bitflags -- it so happens that they can in C, but this is not an API
guarantee. The kernel code for these flags handles things properly by first
removing the LOCK_NB flag and then doing equality comparisons against the
lock type -- not flag checks. Incidentally these get mapped to F_RDLCK,
F_WRLCK and F_UNLCK internally, which just so happen to have the same
values as LOCK_SH, LOCK_EX and LOCK_UN in PHP ;)

Is there some kind of evidence that people are actually trying to use these
as bitflags, and you're trying to solve a real problem here? Or is the only
problem being solved that somebody is celebrating their own ignorance and
incompetence over at r/lolphp again?

Regards,
Nikita

--000000000000c0e32f05dd673aca--