Review Request 110187: Don't communicate with mysql by env vars and autogenerated files

Matěj Laitl matej at laitl.cz
Sun Apr 28 10:35:04 UTC 2013 


> On April 25, 2013, 3:43 p.m., Edward Hades Toroshchin wrote:
> > Thanks for the fix. However, this is the approach, we've used originally, and it didn't prove very reliable. I think we even went back and forth more than once.
> > 
> > So you should really have a look in the history of the mysql collection, to see if and how this is different from the way it's been done before (and why it isn't done that way anymore).
> > 
> > > This fixes an issue where amarok is writing the database to "C:\Program Files (x86)\Amarok\data\amarok".
> > > This issue prevents Amarok from running correctly, because regarding to the rights of the useraccount the directory can be not writeable.
> > 
> > It's not only that, it could also lead to problems if several users tried to use Amarok simultaneously.
> > 
> > I suggest you check, where does this path come from. Why is storageLocation empty?
> 
> Patrick von Reth wrote:
>     Storage location is not empty but either mysql isn't using MYSQL_HOME or unable to read the file.
>     Why should it be a problem if several user are running amarok, what would be the difference to the my.cnf approach.
>
> 
> Edward Hades Toroshchin wrote:
>     > Why should it be a problem if several user are running amarok, what would be the difference to the my.cnf approach.
>     
>     No, I was talking about the my.cnf approach, since the my.cnf file would be in the same place for different users.
> 
> Matěj Laitl wrote:
>     > However, this is the approach, we've used originally, and it didn't prove very reliable. I think we even went back and forth more than once. So you should really have a look in the history of the mysql collection, to see if and how this is different from the way it's been done before (and why it isn't done that way anymore).
>     
>     This was my initial comment too, please see my IRC discussion with Patrick that led to this patch.
>     
>     TL;DR: the last transition to my.cnf was in Jeff's commit that simply stated "use MYSQL_HOME and generate my.cnf on the fly instead of hardcoding. Will be necessary later." - however we were unable to find later commits that would made use of my.cnf necessary. (but we haven't been looking real hard)
>     
>     When I ignore the past (which we should not), passing arguments directly looks more reliable than my.cnf + env vars.
>     
>     -------------------------------------------
>     [Wednesday 24 of April 2013] [15:27:35] <TheOneRing> strohel: is there a reason why amarok is using the my.cnf instead of passing the arguments directly?
>     [Wednesday 24 of April 2013] [15:33:14] <strohel> TheOneRing: I thought there were come problems with it.
>     [Wednesday 24 of April 2013] [15:33:21] <strohel> TheOneRing: lemme dig it
>     [Wednesday 24 of April 2013] [15:39:01] <strohel> TheOneRing: commit 7abdb3421: Jeff Mitchell <mitchell at kde.org> Mon Jul 19 2010 Work around longstanding mysqle-loads-innodb issues with changing to loose-innodb=0; but more importantly, use MYSQL_HOME and generate my.cnf on the fly instead of hardcoding. Will be necessary later.
>     [Wednesday 24 of April 2013] [15:39:02] <kgbot> 4Jeff Mitchell master * [14http://quickgit.kde.org/?p=amarok.git&a=commit&h=7abdb34] Work around longstanding mysqle-loads-innodb issues with changing to loose-innodb=0; but more importantly, use MYSQL_HOME and generate my.cnf on the fly instead of hardcoding. Will be necessary later.
>     [Wednesday 24 of April 2013] [15:39:20] <strohel> Wow, kgbot++
>     [Wednesday 24 of April 2013] [15:39:35] <strohel> TheOneRing: But I don't see the "will be necessary later"
>     [Wednesday 24 of April 2013] [15:40:23] <strohel> TheOneRing: If all the options are passable as arguments, feel free to change it to pass the args directly (but run through reviewboard pls.)
>     [Wednesday 24 of April 2013] [15:40:27] <TheOneRing> so we could pass the stuff as arguments as far as I understand the documentation
>     [Wednesday 24 of April 2013] [15:40:37] <TheOneRing> kk
>     [Wednesday 24 of April 2013] [15:41:30] <strohel> TheOneRing: you can use revert of the mentioned patch as basis, but I'd suggest a cleaner approach. (i.e. not "char *" variables, just QStrings and their toLocal8bit().data() when you need char *)
>     [Wednesday 24 of April 2013] [15:41:54] <TheOneRing> yes
>     [Wednesday 24 of April 2013] [15:42:03] <TheOneRing> local 8bit or latin1?
>     [Wednesday 24 of April 2013] [15:42:59] <strohel> TheOneRing: I think the local8bit is more correct - mysql should assume the that the parameters are specified in currently selected locale's charset - and that's exactly what toLocal8bit does.
>     [Wednesday 24 of April 2013] [15:43:36] <TheOneRing> kk
>     [Wednesday 24 of April 2013] [15:43:44] <strohel> TheOneRing: (at least on Linux; do check with home directory containing accented and special characters on Win, I'll check on Linux once you submit the patch)
>     [Wednesday 24 of April 2013] [15:45:35] <strohel> TheOneRing: do mysql methods take ownerwhip of the passed char * arguments? If so, you'd have to wrap them with qstrdup().
>     [Wednesday 24 of April 2013] [15:46:18] <TheOneRing> . mysql_library_init() makes a copy of the arguments so it is safe to destroy argv or groups after the call.
>     [Wednesday 24 of April 2013] [15:46:28] <strohel> TheOneRing: good
>     [Wednesday 24 of April 2013] [15:47:20] <strohel> TheOneRing: also note that QString::toLocal8bit() creates temporary QByteArray - it's data() pointer is only valid for lifetime of the QByteArray.
>     [Wednesday 24 of April 2013] [15:47:45] <strohel> TheOneRing: i.e. "char *something = QString( "Hi" ).toLocal8Bit().data()" is wrong
>     [Wednesday 24 of April 2013] [15:51:53] <TheOneRing> kk
>     [Wednesday 24 of April 2013] [15:52:57] <TheOneRing> strohel: could you have a look on "mysqld --verbose --help > mysql-help.txt" I can find every setting defined in the my.cnf but the loose-innodb
>     [Wednesday 24 of April 2013] [15:53:22] <TheOneRing> but a lot of settings regarding innodb
>     [Wednesday 24 of April 2013] [15:53:34] <strohel> TheOneRing: hmm, that may actually be aproblem.
>     [Wednesday 24 of April 2013] [15:54:26] <strohel> TheOneRing: perhaps you can find another option that disables loading od the innodb engine?
>     [Wednesday 24 of April 2013] [15:54:26] <kgbot> 4Mat?j Laitl master * [14http://quickgit.kde.org/?p=amarok.git&a=commit&h=0f59c66] IpodCopyTracksJob: even better error messages
>     [Wednesday 24 of April 2013] [15:55:19] <strohel> TheOneRing: --ignore-builtin-innodb ?
>     [Wednesday 24 of April 2013] [15:55:24] <TheOneRing> http://dev.mysql.com/doc/refman/5.6/en/innodb-turning-off.html
>     [Wednesday 24 of April 2013] [15:57:52] <TheOneRing> ok it looks like amarok offers the possebility to use a handcrafted my.cnf ...
>     [Wednesday 24 of April 2013] [15:58:17] <strohel> TheOneRing: does it?
>     [Wednesday 24 of April 2013] [15:59:48] <strohel> TheOneRing: I've found that 7abdb3421 in effect just changed --loose-skip-innodb to loose-innodb -- si it shouldn't be a problem as there seem to be other ways to disable it.
>     [Wednesday 24 of April 2013] [15:59:49] <kgbot> 4Jeff Mitchell master * [14http://quickgit.kde.org/?p=amarok.git&a=commit&h=7abdb34] Work around longstanding mysqle-loads-innodb issues with changing to loose-innodb=0; but more importantly, use MYSQL_HOME and generate my.cnf on the fly instead of hardcoding. Will be necessary later.
>     [Wednesday 24 of April 2013] [16:00:18] <TheOneRing> kk
>     [Wednesday 24 of April 2013] [16:00:22] <TheOneRing> http://quickgit.kde.org/?p=amarok.git&a=blob&h=0233498fdeb18ab51e709e9a78384fc37c47cb2a&hb=e033f7c07a476a9641a8c04ce73b57ecb8209df3&f=src%2Fcore-impl%2Fcollections%2Fdb%2Fsql%2Fmysqlecollection%2FMySqlEmbeddedStorage.cpp line 54
>     [Wednesday 24 of April 2013] [16:00:22] <kgbot> 4Patrick von Reth master * [14http://quickgit.kde.org/?p=amarok.git&a=commit&h=e033f7c] updated scripts for 2.7.0 relesae
>     [Wednesday 24 of April 2013] [16:01:39] <strohel> TheOneRing: Hmm, I'd just ditch the hidden option. Please do it - as the patch will do though reviewboard other devs will have a change to object.
>     [Wednesday 24 of April 2013] [16:01:54] <strohel> *chance
>     [Wednesday 24 of April 2013] [16:28:40] <TheOneRing> strohel: not yet ready but at least it is working :D
>     [Wednesday 24 of April 2013] [16:29:06] <strohel> TheOneRing: good, hope it won't break Linux.
>     [Wednesday 24 of April 2013] [16:34:36] <TheOneRing> strohel: does http://paste.kde.org/731414/ look ok?
>     [Wednesday 24 of April 2013] [16:34:59] <TheOneRing> ah
>     [Wednesday 24 of April 2013] [16:35:09] <TheOneRing> I betetr remove the old support completely
>     [Wednesday 24 of April 2013] [16:35:34] <TheOneRing> because I would need to call setenv again in this approach
>     [Wednesday 24 of April 2013] [16:36:01] <strohel> TheOneRing: yes, do
>     [Wednesday 24 of April 2013] [16:36:14] <strohel> TheOneRing: also please mind Amarok coding style - spaces around args
>     [Wednesday 24 of April 2013] [16:36:43] <strohel> TheOneRing: qstrdup() is actually not needed for string literals ("Amarok")
>     [Wednesday 24 of April 2013] [16:37:30] <strohel> TheOneRing: Or: you've said mysql funcs copy the arguments - qstrdub is actually *never* needed then.
>     [Wednesday 24 of April 2013] [16:37:47] <TheOneRing> hm yes but the delete would fail?
>     [Wednesday 24 of April 2013] [16:39:06] <strohel> TheOneRing: oh yes, but the fact is that with proper use of QByteArrays you wouldn't have to call the delete at all.
>     [Wednesday 24 of April 2013] [16:39:49] <strohel> TheOneRing: I'd just construct the args as QList<QByteArray> and then create a char **mysql_argv; array of it in a simple foreach loop.
>     [Wednesday 24 of April 2013] [16:40:41] <strohel> TheOneRing: BTW, why don't you use git? ;)
>     
>     *** Logfile started
>     *** on Wed Apr 24 16:53:02 2013
>     
>     [Wednesday 24 of April 2013] [16:53:02] Topic The channel topic is "commitfilter dead, use projects.kde.org -> Follow this project (log in) | 2.8 & 3.0 Roadmap: http://bit.ly/TO7PbI | Component Bugs: bit.ly/OBZ8c7 | bored? http://tinyurl.com/65jf64 | calendar: http://tinyurl.com/qj63r |important links: http://tinyurl.com/6krsuy |Bugs: http://tinyurl.com/allamarokbugs |Continuous integration: http://build.kde.org".
>     [Wednesday 24 of April 2013] [16:53:02] Topic The topic was set by strohel!~strohel at 193.86.155.71 on 8. 3. 2013 17:57.
>     [Wednesday 24 of April 2013] [16:53:04] Mode Channel modes: no messages from outside, secret
>     [Wednesday 24 of April 2013] [16:53:04] Created This channel was created on 26. 11. 2006 07:42.
>     [Wednesday 24 of April 2013] [16:53:30] <strohel> TheOneRing: sorry, net connection problems. I think it failed to send:
>     [Wednesday 24 of April 2013] [16:53:37] <strohel> heOneRing: I'd just construct the args as QList<QByteArray> and then create a char **mysql_argv; array of it in a simple foreach loop.
>     [Wednesday 24 of April 2013] [16:53:44] <strohel> TheOneRing: BTW, why don't you use git? ;)
>     [Wednesday 24 of April 2013] [16:55:10] <TheOneRing> because I curently only have a usable build of the 2.7.0 tag and dont want to rebuild everything^^
>     [Wednesday 24 of April 2013] [17:04:41] <TheOneRing> strohel: http://paste.kde.org/731438/
>     [Wednesday 24 of April 2013] [17:09:05] <TheOneRing> I will open a review request tomorrow or later this evening
>     [Wednesday 24 of April 2013] [17:24:32] <strohel> TheOneRing: looks good on first glance

Edward, does that resolve your remarks? Would you object merging this?


- Matěj


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/110187/#review31576
-----------------------------------------------------------


On April 25, 2013, 2:36 p.m., Patrick von Reth wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/110187/
> -----------------------------------------------------------
> 
> (Updated April 25, 2013, 2:36 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> -------
> 
> Don't communicate with mysql by env vars and autogenerated files
> 
> Instead of generating the my.cnf every time amarok starts and to pass
> the location of this file by setting an environment variable directly
> pass the settings as arguments to mysql. As this is probably a better
> approach and the only one working on windows.
> 
> This fixes an issue where amarok is writing the database to "C:\Program Files (x86)\Amarok\data\amarok".
> This issue prevents Amarok from running correctly, because regarding to the rights of the useraccount the directory can be not writeable.
> 
> The commands used are taken from the output of "mysqld --verbose --help". 
> 
> 
> Diffs
> -----
> 
>   src/core-impl/collections/db/sql/mysqlecollection/MySqlEmbeddedStorage.cpp 0233498fdeb18ab51e709e9a78384fc37c47cb2a 
> 
> Diff: http://git.reviewboard.kde.org/r/110187/diff/
> 
> 
> Testing
> -------
> 
> Only on windows
> 
> 
> Thanks,
> 
> Patrick von Reth
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20130428/e667935e/attachment-0001.html>

More information about the Amarok-devel mailing list