Re: cvs: php4 /ext/session mod_mm.c

Sascha Schumann wrote:

I thought the reporter want valid SID defined always, since I wanted valid SID defined always. Document does not memtion when SID is defined, IIRC. Document should be updated.

    It has been documented since I first wrote the session
    documentation.

    "Alternatively, you can use the constant SID which is defined,
    if the client did not send the appropriate cookie. SID is
    either of the form session_name=session_id or is an empty
    string.

I'll check and update doc if needed.

You are forcing "user" save handler to return "" string anyway.

    No.  Try to read the code again.  A user handler should
    simply return a non-string.

It works sometimes, but it segfaults sometimes. (with original code)

1. It does not work simply with current code. (It seems mm works for you. While it does not for me. I guess

    you enable register_globals)

    I don't which you can easily derive from the test script I
    sent to you earlier.

Which test script?

2. There is no way to return fatal error from reading. (This could be serious design flaw, since when read failed

    due to lost connection,etc, write function should never try
    to write.)

    A php function can itself halt the script execution, so there
    is no need to enforce bailing out.

It's not my point. Current code is not obvious that session save handler does not write bogus session data when read is failed with other errors like network and database.

1. Since session module functions may not be called sometimes depends of its stauts, if function cannot work, it should fail.

    This is completely independent of using a bit-vector and is
    easily achieved using php_session_status.

Of couse, we can do that. We just have to type more.

2. Since original code uses PS(mod) and it's member for current status of session module (or save handler), you failed to write code correctly. It's much easier and cleaner to use single status value to represent session status.

    I don't get what you are trying to say there.

AFAIK, session module has 2 bugs due to this design. I suggest to use one single status to indicate current session module's status.

http://news.php.net/article.php?group=php.cvs&article=9712 Does not make sense. Die, if session module cannot be initialized.

Does not make sense web server dies when user set save_path to "host=localhost db=session" for their user save handler.

    Then the user should either set save_handler to "user" or
    not change it all (files is perfectly happy then).

files are happy, since it shares save_path. What about others?

If we would like to have configurable path, we need new value.

    No.

Then static path?

- Use return value to indicate if function worked right or not

    Yes, this makes sense at a couple of places.

- Do not use implicit status using value other than status.

    Parse error..

Do not use implicit status value other than variable that is dedicated for status. Is this better?

- Treat session module as simple finite state machine. (It a lot easier to maintain) - Handle error as soon as possible.

    I.e. when the server starts up.

If we can decide 100% error and should never allow that, yes. But it's not the case for mm start up.

Most of problems that I've found is related to these. BTW, you are better not to say other's patches are bogus. We know there are sevral serious bugs. One can easily argue that your code or coding style is bogus. But, it's not productive at all ;)

I was spending my time to fix sevral serious sessoin module bugs, but it seems we have a lot different opinion for coding style. I'll just keep my own session module bug free for my usage. (I leave session module as is. Fix other bugs) -- Yasuo Ohgaki yohgaki@dd.iij4u.or.jp