Description
Jonathan Druart
2015-03-19 15:52:59 UTC
Created attachment 37014 [details] [review] Bug 13876: Fallback to /tmp if logdir is not writable If the logdir directory is not writable by the koha user, the installer should not explode. This patch uses /tmp as a fallback. That means that the history won't be kept. Test plan: 0/ Generate an update (modify updatedatabase.pl and kohaversion) 1/ Make sure the permission are not correct on $logdir 2/ Go on the interface and execute the update entry. 3/ Confirm that the log has been created in /tmp Retry with correct permissions and confirm the log will be created in $logdir. Maybe use File::Temp instead of hardcoding /tmp ? BTW Note that if I would have permissions to write in the original log dir, but SELinux would not allow it, your test here would not work. Created attachment 37022 [details] [review] Bug 13876: Fallback to /tmp if logdir is not writable If the logdir directory is not writable by the koha user, the installer should not explode. This patch uses a temporary directory as a fallback. That means that the history won't be kept. Test plan: 0/ Generate an update (modify updatedatabase.pl and kohaversion) 1/ Make sure the permission are not correct on $logdir 2/ Go on the interface and execute the update entry. 3/ Confirm that the log has been created in a temporaty directory. Retry with correct permissions and confirm the log will be created in $logdir. (In reply to M. de Rooy from comment #2) > Maybe use File::Temp instead of hardcoding /tmp ? Done. (In reply to M. de Rooy from comment #3) > BTW Note that if I would have permissions to write in the original log dir, > but SELinux would not allow it, your test here would not work. I don't know a lot about SELinux, how can you know if a directory is writable? Will sign off on this one with a follow-up. Please wait :) Created attachment 37052 [details] [review] Bug 13876: Fallback to /tmp if logdir is not writable If the logdir directory is not writable by the koha user, the installer should not explode. This patch uses a temporary directory as a fallback. That means that the history won't be kept. Test plan: 0/ Generate an update (modify updatedatabase.pl and kohaversion) 1/ Make sure the permission are not correct on $logdir 2/ Go on the interface and execute the update entry. 3/ Confirm that the log has been created in a temporaty directory. Retry with correct permissions and confirm the log will be created in $logdir. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Adding a follow-up for completely resolving encountered issues. Created attachment 37053 [details] [review] Bug 13876: [QA Follow-up] Resolve updatedatabase logfile issues This patch removes the test on write permissions of the logdir. It uses File::Temp to generate a file in the logdir or otherwise in a system tmp dir. This resolves issues with both write permissions as well as SELinux denials. Note that after generating an empty file, we are appending to it in the system statement. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Katrin or Jonathan: Please check. Thanks. Created attachment 37094 [details] [review] [SIGNED OFF] Bug 13876: Fallback to /tmp if logdir is not writable If the logdir directory is not writable by the koha user, the installer should not explode. This patch uses a temporary directory as a fallback. That means that the history won't be kept. Test plan: 0/ Generate an update (modify updatedatabase.pl and kohaversion) 1/ Make sure the permission are not correct on $logdir 2/ Go on the interface and execute the update entry. 3/ Confirm that the log has been created in a temporaty directory. Retry with correct permissions and confirm the log will be created in $logdir. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Adding a follow-up for completely resolving encountered issues. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Created attachment 37095 [details] [review] [SIGNED OFF] Bug 13876: [QA Follow-up] Resolve updatedatabase logfile issues This patch removes the test on write permissions of the logdir. It uses File::Temp to generate a file in the logdir or otherwise in a system tmp dir. This resolves issues with both write permissions as well as SELinux denials. Note that after generating an empty file, we are appending to it in the system statement. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> This fixes the problem for me, my database updates are working again! As the QA follow-up has more lines than the initial patch ;) I'd like to get another QA view from Jonathan or another QA person on this. What's the point of the suffix? Now the log filename is updatedatabase_2015-03-25T15:06:59_uyj4.log Your patch does not call the tempdir subroutine, you should modify the use File::Temp line: 10 use File::Temp qw( tempdir ); I'm just curious why not use IPC::Run::run? Is the time stamp on the file of when we ran the updates important? It seems like update log files will just build up forever. Is there a point to their accumulation? What questions are we trying to answer with them? I'm wondering if there is an optimization that is being missed. Never mind. I saw the 13793 bug now. Created attachment 37906 [details] [review] Bug 13876: [QA Follow-up] Resolve updatedatabase logfile issues This patch removes the test on write permissions of the logdir. It uses File::Temp to generate a file in the logdir or otherwise in a system tmp dir. This resolves issues with both write permissions as well as SELinux denials. Note that after generating an empty file, we are appending to it in the system statement. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Amended: only use File::Temp in the fallback case. Do not add the four character suffix in the first attempt. (In reply to Jonathan Druart from comment #13) > What's the point of the suffix? > Now the log filename is updatedatabase_2015-03-25T15:06:59_uyj4.log > Your patch does not call the tempdir subroutine, you should modify the use > File::Temp line: > 10 use File::Temp qw( tempdir ); Now do a require File::Temp if needed. Only use the particular four character suffix of File::Temp in the fallback case. Could you retest, Jonathan? Created attachment 37965 [details] [review] [SIGNED OFF] Bug 13876: [QA Follow-up] Resolve updatedatabase logfile issues This patch removes the test on write permissions of the logdir. It uses File::Temp to generate a file in the logdir or otherwise in a system tmp dir. This resolves issues with both write permissions as well as SELinux denials. Note that after generating an empty file, we are appending to it in the system statement. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Amended: only use File::Temp in the fallback case. Do not add the four character suffix in the first attempt. Signed-off-by: Chris Nighswonger <cnighswonger@foundations.edu> Created attachment 37966 [details] [review] [SIGNED-OFF] Bug 13876: Fallback to /tmp if logdir is not writable If the logdir directory is not writable by the koha user, the installer should not explode. This patch uses a temporary directory as a fallback. That means that the history won't be kept. Test plan: 0/ Generate an update (modify updatedatabase.pl and kohaversion) 1/ Make sure the permission are not correct on $logdir 2/ Go on the interface and execute the update entry. 3/ Confirm that the log has been created in a temporaty directory. Retry with correct permissions and confirm the log will be created in $logdir. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Adding a follow-up for completely resolving encountered issues. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Signed-off-by: Chris Nighswonger <cnighswonger@foundations.edu> Created attachment 37999 [details] [review] Bug 13876: [QA Follow-up] Resolve updatedatabase logfile issues This patch removes the test on write permissions of the logdir. It uses File::Temp to generate a file in the logdir or otherwise in a system tmp dir. This resolves issues with both write permissions as well as SELinux denials. Note that after generating an empty file, we are appending to it in the system statement. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Amended: only use File::Temp in the fallback case. Do not add the four character suffix in the first attempt. Signed-off-by: Chris Nighswonger <cnighswonger@foundations.edu> Reordered the patches. Katrin: did you forget to sign the second one? Hm I probably intended to sign off the second, but wanted to have someone else to have another look at it Created attachment 38000 [details] [review] Bug 13876: Fallback to /tmp if logdir is not writable If the logdir directory is not writable by the koha user, the installer should not explode. This patch uses a temporary directory as a fallback. That means that the history won't be kept. Test plan: 0/ Generate an update (modify updatedatabase.pl and kohaversion) 1/ Make sure the permission are not correct on $logdir 2/ Go on the interface and execute the update entry. 3/ Confirm that the log has been created in a temporaty directory. Retry with correct permissions and confirm the log will be created in $logdir. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Adding a follow-up for completely resolving encountered issues. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Signed-off-by: Chris Nighswonger <cnighswonger@foundations.edu> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 38001 [details] [review] Bug 13876: [QA Follow-up] Resolve updatedatabase logfile issues This patch removes the test on write permissions of the logdir. It uses File::Temp to generate a file in the logdir or otherwise in a system tmp dir. This resolves issues with both write permissions as well as SELinux denials. Note that after generating an empty file, we are appending to it in the system statement. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Amended: only use File::Temp in the fallback case. Do not add the four character suffix in the first attempt. Signed-off-by: Chris Nighswonger <cnighswonger@foundations.edu> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 38002 [details] [review] Bug 13876: Add the from => to versions in the filenames Additionally to the datetime, it could be useful to have the version numbers in the log filenames. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 38004 [details] [review] Bug 13876: Add the from => to versions in the filenames Additionally to the datetime, it could be useful to have the version numbers in the log filenames. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 38021 [details] [review] [PASSED QA] Bug 13876: Fallback to /tmp if logdir is not writable If the logdir directory is not writable by the koha user, the installer should not explode. This patch uses a temporary directory as a fallback. That means that the history won't be kept. Test plan: 0/ Generate an update (modify updatedatabase.pl and kohaversion) 1/ Make sure the permission are not correct on $logdir 2/ Go on the interface and execute the update entry. 3/ Confirm that the log has been created in a temporaty directory. Retry with correct permissions and confirm the log will be created in $logdir. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Adding a follow-up for completely resolving encountered issues. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Signed-off-by: Chris Nighswonger <cnighswonger@foundations.edu> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 38022 [details] [review] [PASSED QA] Bug 13876: [QA Follow-up] Resolve updatedatabase logfile issues This patch removes the test on write permissions of the logdir. It uses File::Temp to generate a file in the logdir or otherwise in a system tmp dir. This resolves issues with both write permissions as well as SELinux denials. Note that after generating an empty file, we are appending to it in the system statement. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Amended: only use File::Temp in the fallback case. Do not add the four character suffix in the first attempt. Signed-off-by: Chris Nighswonger <cnighswonger@foundations.edu> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 38023 [details] [review] [PASSED QA] Bug 13876: Add the from => to versions in the filenames Additionally to the datetime, it could be useful to have the version numbers in the log filenames. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Patches pushed to master. Thanks guys! This doesn't apply on 3.18.x please rebase if needed |