GNOME Bugzilla – Bug 633309
Guard against setting the library folder to / or ~
Last modified: 2011-03-22 15:33:55 UTC
Created attachment 173367 [details] Dump running under Ubuntu 10.04 It's pretty simple bug: I run Banshee, it opens and the crashes.
Thank you for your bug report. This seems to be caused by the FileSystemWatcher extension, which apparently has trouble watching your Music or Videos folder. Could you run the following two commands in a terminal and post the result here ? gconftool --get /apps/banshee-1/sources/_music_library_source_-_library/library-location gconftool --get /apps/banshee-1/sources/_video_library_source_-_video_library/library-location These should print out the current values configured for your Music and Videos folders.
Here are the results of the commands: myaccount@myaccount-laptop:~$ gconftool --get /apps/banshee-1/sources/_music_library_source_-_library/library-location / ouaejk@ouaejk-laptop:~$ gconftool --get /apps/banshee-1/sources/_video_library_source_-_video_library/library-location /home/myaccount/Videos
Please disregard the above comment...here are the correct results: myaccount@myaccount-laptop:~$ gconftool --get /apps/banshee-1/sources/_music_library_source_-_library/library-location / myaccount@myaccount-laptop:~$ gconftool --get /apps/banshee-1/sources/_video_library_source_-_video_library/library-location /home/myaccount/Videos
Hmm, the location of your music library is set to / ? That's probably the cause of your problem. Could edit the value of this parameter and set it to a "normal" value, like "/home/myaccount/Music" ? You can use the "gconf-editor" tool to navigate to the parameter and edit it, or run the following command : gconftool --type string --set apps/banshee-1/sources/_music_library_source_-_library/library-location "/home/myaccount/Music"
That took care of it! Thanks!
We really need to guard against this.
(In reply to comment #6) > We really need to guard against this. Changing the summary to reflect this. Also marking as gnome-love, should be a simple enough task for a novice contributor.
Created attachment 180445 [details] [review] check questionable directories and prevent starting the library watcher for them Proposed patch: check for questionable directories and for them, instead of starting the library watcher, throw a warning. Fixes Banshee crash when library path is set to / or ~
Review of attachment 180445 [details] [review]: Thanks, Frank. ::: src/Extensions/Banshee.LibraryWatcher/Banshee.LibraryWatcher/SourceWatcher.cs @@ +80,1 @@ throw new Exception ("Will not create LibraryWatcher for the entire home directory"); isn't this check ^ already doing the home-directory check? @@ +82,3 @@ + if (path.Equals (Environment.SystemDirectory) + || path.Equals ("//") This probably isn't valid on windows -- maybe use System.IO.Path.DirectorySeparator ? @@ +88,3 @@ + library.Name, + path, + Environment.GetFolderPath (Environment.SpecialFolder.MyMusic)); This warning is a little misleading w/ the SpecialFolder.MyMusic bit -- since the user can set whatever directory they want, plus we can watch the video folder etc. I think just remove that part of the warning.
Created attachment 180545 [details] [review] check for root directory and prevent starting the library watcher for it Updated after Gabriel's review. Home directory was already checked, now determining the root path in a OS independent manner and throwing an exception the way the existing code is doing it.
Review of attachment 180545 [details] [review]: ::: src/Extensions/Banshee.LibraryWatcher/Banshee.LibraryWatcher/SourceWatcher.cs @@ +81,3 @@ } + if (path.Equals (Environment.SystemDirectory) This is "" for me -- is this check necessary in your testing/on your setup? Are you using Linux? @@ +82,3 @@ + if (path.Equals (Environment.SystemDirectory) + || path.Equals (Path.GetPathRoot (Environment.CurrentDirectory) + Path.DirectorySeparatorChar)) this value "//" for me -- I don't think you need the + Path.DSC part @@ +83,3 @@ + if (path.Equals (Environment.SystemDirectory) + || path.Equals (Path.GetPathRoot (Environment.CurrentDirectory) + Path.DirectorySeparatorChar)) + { put braces for if's on the same line as the condition, and put the condition all on one line if it's < 120 chars
Created attachment 180631 [details] [review] check for root directory and prevent starting the library watcher for it Removed the SystemDirectory check, I am on Linux and it was "" for me as well. For some Reason the value of the root directory comes out at "//" and it does work. Without Path.DSC it does not, coming to the value of "/". I put my Music Library to "/" via gconf for testing. I assume, there is some point in the code where the pathes are assumed "naked" and the Path.DSC is added. Haven't verified that, but I had to put Path.DSC in order to get the patch working. Also conformed to the coding guidelines now. Thanks for the great support Gabriel, just getting started with this.
Comment on attachment 180631 [details] [review] check for root directory and prevent starting the library watcher for it Committed, thanks Frank. I also pushed a patch checking for null/empty path, and for non-existent path.