291574 - Provide ability to specify location for the params file

Status: NEW

(Bugzilla :: Administration, task)

Description

[Alexis Sukrieh]

In Bugilla/Config.pm, the WritePArams function uses the rename() Perl function
to rename the tempfile.
This causes every symlinks to be overwritten.

We cannot use a symlink for the params file then, and that's a problem for those
who want to take care of that file in a special location (/etc/bugzilla for
instance).

Fix : Don't use rename for saving the params file, do the copy by hand.

Comment 1

[Alexis Sukrieh]

Attachment: patch to make the use of symlinks possible

Copying the params file by hand do not overwrite the symlink.

Comment 2

[Dave Miller [:justdave]]

Comment on attachment 181611 [details] [diff] [review]
patch to make the use of symlinks possible

ewwwwwwwwww

You're asking for race conditions and will crash people visiting the website
between the time the process truncates the file and finishes rewriting it.  No
way.

A better idea is to make it possible to put the params file somewhere else so
you don't need a symlink.

Comment 3

[Alexis Sukrieh]

Attachment: Add a $params_file variable

Ok, then if there's no way to make a WriteParams() function that could safely
write the params file without delteing a symlink, here is a patch to provide a
way to specify an absolute location for the params file.

But I still think that overwriting a symlink is a real drawback of the actual
solution, anyway that's only my opinion...

Comment 4

[Dave Miller [:justdave]]

It was pointed out to me in email (not by anyone currently on this bug) that I
was a bit harsh here.  I probably wasn't quite as "clean" as I should have been
in my comments because I was already on familiar terms with the person I was
aiming the comments at, but the person who pointed it out to me is correct that
it still gives a bad impression to everyone else reading this, so I apologize to
anyone who took it that way for being so abrupt.

I'll also take this opportunity to clarify what I said a little and expand on my
idea for how to better fix this. :)

We use rename() because most operating systems will keep the original file
around on disk after it's "overwritten" until all of the current users of the
file close it (they just remove/replace the directory entry to it to the file
that we're moving overtop of it).  This eliminates any race conditions that
might occur because anyone who already had the file open prior to us touching it
will get the old file, and anyone who opens it new after that point gets the new
one.

If you open the existing file and start writing to it while someone else is
already reading from it, all kinds of crazy things could happen.

So we need to find a way to make rename work.  The easiest way is to set up a
variable in Bugzilla::Config like we do for the location of other files (such as
localconfig and the data directory) that points to where the params file is
stored.  The temporary file doesn't necessarily have to be in the same directory
(although that's certainly the easiest thing to do if you can get away with it),
but it does have to be on the same logical filesystem (some OSes will let
rename() work across filesystems, some don't -- sice we're cross-platform, we
don't want to take chances with it).

Comment 5

[Dave Miller [:justdave]]

Comment on attachment 181623 [details] [diff] [review]
Add a $params_file variable

oops, I midaired with you while you were posting this.	But yes, this is much
more what I had in mind.  Now to play with it :)

Comment 6

[Vlad Dascalu]

Comment on attachment 181623 [details] [diff] [review]
Add a $params_file variable

While probably this is what you want for Debian, the patch will break most
Windows installations' ability to save the param file, because /etc/bugzilla is
not a valid directory on most of them.

In the interest of merging Debian with Mozilla's CVS Bugzilla, I'd upload a
patch in which our $params_file = "$datadir/params". I'd be happy to review
that one :-)

After that, we might change the default location in another bug.

Comment 7

[Alexis Sukrieh]

(In reply to comment #6)
> (From update of attachment 181623 [details] [diff] [review] [edit])
> While probably this is what you want for Debian, the patch will break most
> Windows installations' ability to save the param file, because /etc/bugzilla is
> not a valid directory on most of them.

Indeed, that's not a problem to change that default location :)

> In the interest of merging Debian with Mozilla's CVS Bugzilla, I'd upload a
> patch in which our $params_file = "$datadir/params". I'd be happy to review
> that one :-)
> 
> After that, we might change the default location in another bug.

Indeed.

BTW, I realized others issues with the rename() function, it cannot write under
/etc... I don't understand why, as the permissions are correct. If I use the
previous patch (copying by hand) it works...

Strange, moreover, rename didn't return a false error code, then doeditparams
thought everything was find. I found that the /etc/bugzilla/params file was not
changed and that the temp file did not move...

Comment 8

[Vlad Dascalu]

Attachment: sukria's patch with $datadir/param instead of /etc/bugzilla/param

r=vladd on the patch with $datadir/param instead of /etc/bugzilla/param

Comment 9

[Dave Miller [:justdave]]

Comment on attachment 181638 [details] [diff] [review]
sukria's patch with $datadir/param instead of /etc/bugzilla/param

based on Alexis' comment 7 here, it sounds like we have some additional issues
to iron out with this yet.  Like possibly specifying a temporary file location
to use as the scratchpad before the rename overtop of the original one.  As
this is targetted at 2.22 and we haven't branched yet, we have some time to
play with it still before it can land anyway.

Comment 10

[Frédéric Buclin]

the trunk is now frozen to prepare Bugzilla 2.22. Retargetting to 2.24.

Comment 11

[Frédéric Buclin]

This bug is retargetted to Bugzilla 3.2 for one of the following reasons:

- it has no assignee (except the default one)
- we don't expect someone to fix it in the next two weeks (i.e. before we freeze the trunk to prepare Bugzilla 3.0 RC1)
- it's not a blocker

If you are working on this bug and you think you will be able to submit a patch in the next two weeks, retarget this bug to 3.0.

If this bug is something you would like to see implemented in 3.0 but you are not a developer or you don't think you will be able to fix this bug yourself in the next two weeks, please *do not* retarget this bug.

If you think this bug should absolutely be fixed before we release 3.0, either ask on IRC or use the "blocking3.0 flag".

Comment 12

[Serge van den Boom]

Neither of the patches is a complete solution. You want the temporary file to be on the same filesystem as the params file itself, because otherwise "rename" might fail.

Note that for the same reason using symlinks is not a good idea here.

Comment 13

[Max Kanat-Alexander]

Bugzilla 3.2 is now frozen. Only enhancements blocking 3.2 or specifically approved for 3.2 may be checked in to the 3.2 branch. If you would like to nominate your enhancement for Bugzilla 3.2, set the "blocking3.2" flag to "?". Then, either the target milestone will be changed back, or the blocking3.2 flag will be granted, if we will accept this enhancement for Bugzilla 3.2.

This particular bug has not been touched in over eight months, and thus is being retargeted to "---" instead of "Bugzilla 4.0". If you believe this is a mistake, feel free to retarget it to Bugzilla 4.0.