Bug 6688 - Unnecessary diagnostic issued from Slim::Formats::RemoteStream::open
: Unnecessary diagnostic issued from Slim::Formats::RemoteStream::open
Status: CLOSED FIXED
Product: Logitech Media Server
Classification: Unclassified
Component: MusicIP
: 7.0
: PC Linux (other)
: P2 trivial (vote)
: ---
Assigned To: Michael Herger
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-01-20 20:18 UTC by quiet.dragon
Modified: 2009-09-08 09:12 UTC (History)
3 users (show)

See Also:
Category: ---


Attachments
Proposed patch (1.73 KB, patch)
2008-01-20 20:18 UTC, quiet.dragon
Details | Diff
silence warning during startup when MusicIP isn't enabled (1.41 KB, patch)
2008-01-22 00:31 UTC, Michael Herger
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description quiet.dragon 2008-01-20 20:18:03 UTC
When MusicMagic is not installed, the following somewhat cryptic diagnostic is issued:

[08-01-20 18:21:45.8037] Slim::Formats::RemoteStream::open (98) Can't open socket to [localhost:10002]: 111: Connection refused

The corresponding call stack is:

        Slim::Formats::RemoteStream::open('Slim::Player::Protocols::HTTP', 'HASH(0xd106368)') called at /store/slimserver/squeezecenter-7.0-16473-noCPAN/slimserver/Slim/Player/Protocols/HTTP.pm line 38
        Slim::Player::Protocols::HTTP::new('Slim::Player::Protocols::HTTP', 'HASH(0xd106368)') called at /store/slimserver/squeezecenter-7.0-16473-noCPAN/slimserver/Slim/Plugin/MusicMagic/Plugin.pm line 131
        Slim::Plugin::MusicMagic::Plugin::initPlugin('Slim::Plugin::MusicMagic::Plugin') called at /store/slimserver/squeezecenter-7.0-16473-noCPAN/slimserver/Slim/Utils/PluginManager.pm line 481
        eval {...} called at /store/slimserver/squeezecenter-7.0-16473-noCPAN/slimserver/Slim/Utils/PluginManager.pm line 481
        Slim::Utils::PluginManager::enablePlugins('Slim::Utils::PluginManager') called at /store/slimserver/squeezecenter-7.0-16473-noCPAN/slimserver/Slim/Utils/PluginManager.pm line 123
        Slim::Utils::PluginManager::init('Slim::Utils::PluginManager') called at /store/slimserver/squeezecenter-7.0-16473-noCPAN/slimserver/slimserver.pl line 407
        main::init() called at /store/slimserver/squeezecenter-7.0-16473-noCPAN/slimserver/slimserver.pl line 453
        main::main() called at /store/slimserver/squeezecenter-7.0-16473-noCPAN/slimserver/slimserver.pl line 1005

The MusicMagic plugin is attempting:

        my $http = Slim::Player::Protocols::HTTP->new({
                'url'     => "http://$MMSHost:$MMSport/api/version",
                'create'  => 0,
                'timeout' => 5,
        });

        if (!$http) {

                $initialized = 0;

                $log->warn("Can't connect to port $MMSport - MusicMagic disabled
.");

Thus the RemoteStream::open() diagnostic is actually a informational only, but it's severity is
not indicated. Seeing the diagnostic alone suggests that something quite severe has occurred.

I would suggest something like:

        my $http = Slim::Player::Protocols::HTTP->new({
                'url'     => "http://$MMSHost:$MMSport/api/version",
                'create'  => 0,
                'timeout' => 5,
                'quiet'   => 1,
        });

Then RemoteStream::open() can simply return a status code instead of logging an error.

See attachment.
Comment 1 quiet.dragon 2008-01-20 20:18:30 UTC
Created attachment 2694 [details]
Proposed patch
Comment 2 KDF 2008-01-20 22:23:47 UTC
Please don't preset targets. May or may not be too late for 7.0

I'm not sure a new arg is the way we'd want to go.  It's also not recent style to use 'unless', but I'll cc andy as this falls mostly into the remotestream code.
Comment 3 Michael Herger 2008-01-21 05:11:51 UTC
I'd rather make it $log->info() instead of warn() to silence the warning.
Comment 4 Blackketter Dean 2008-01-21 09:42:46 UTC
What do you think Earl?
Comment 5 quiet.dragon 2008-01-21 09:56:20 UTC
(In reply to comment #4)
> What do you think Earl?
> 

That sounds like a reasonable compromise.
Comment 6 Michael Herger 2008-01-21 13:53:47 UTC
Let's fix this correctly: MusicIP is delaying SC startup and producing error messages in the log if it's not found during startup. Let's therefore

- disable the plugin, if it's not selected during the setup wizard, or

- move most of the initialization to late_init() or something, which is only called if the plugin is installed and enabled

will try to come up with a patch for approval by tomorrow
Comment 7 Michael Herger 2008-01-22 00:31:49 UTC
Created attachment 2704 [details]
silence warning during startup when MusicIP isn't enabled

Kevin - may I ask you to comment on this patch? It's basically cutting short initPlugin() if the plugin is enabled, but the integration is not (which is default behaviour after a clean install).

Additionally I'm changing the warn() to info() and suppressing the error message in S::Formats::RemoteStream for this particular case.

Dean - approved?
Comment 8 Andy Grundman 2008-01-22 06:53:25 UTC
Is the change to RemoteStream needed now that it won't try unless enabled?  It is a useful error if MIP is enabled and it can't connect.
Comment 9 Michael Herger 2008-01-22 07:03:35 UTC
No, it's not needed.
Comment 10 KDF 2008-01-22 07:52:20 UTC
I'll try to give it a test later this morning.  On initial inspection, probably not worth having the log->warn say that musicmagic is disabled, since it won't get there if it is disabled. Since were in the area, perhaps change it to MusicIP and maybe the wording could be "MusicIP connection failed."
Comment 11 KDF 2008-01-22 10:37:02 UTC
testing is ok.  If musicip IS enabled and the connection fails, the RemoteStream error will show, but it's explained by the error from MusicIP shortly after.  We may, however, want to ensure that if the remotestream error is shown, then the musicip error will also.  This means they need the same debug level, or at least debug levels that both match the default.  

too much thinking about this would probably come up with an unreasonably large number of cases to consider for this.  It's just thoughts, nothing critical.  Patch looks good otherwise.
Comment 12 Michael Herger 2008-01-23 01:00:23 UTC
Change 16640 - don't test the http connection to the MIP service if the integration has been disabled. Align log level with Slim::Formats::RemoteStream

Please re-open if this should cause any new issues. Thanks!
Comment 13 Chris Owens 2008-03-07 09:04:34 UTC
This bug is being closed since it was resolved for a version which is now released!  Please download the new version of SqueezeCenter (formerly SlimServer) at http://www.slimdevices.com/su_downloads.html

If you are still seeing this bug, please re-open it and we will consider it for a future release.