Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for SMB shares after suspend #2895

Merged
merged 2 commits into from Aug 7, 2013
Merged

Conversation

verybadsoldier
Copy link
Contributor

It basically is supposed to do this:

-deinitialize libsmbclient before going to sleep (Linux only)
-start and stop all addon services before and after suspend (otherwise addons are able to reinitialize SMB right before actually going to sleep and break it)

More details here:
http://forum.xbmc.org/showthread.php?tid=166430

@Memphiz
Copy link
Member

Memphiz commented Jun 19, 2013

jenkins test this please

@bulkzooi
Copy link

"Good to merge" or "won't break build"? But I like this PR. Hopefully NFS isn't forgotten.

@Memphiz
Copy link
Member

Memphiz commented Jun 20, 2013

It didn't built yet - i have still problems with Jenkins - so basically this is a false status - still to be ignored - and if it was for real it would just indicate that it compiled on all platforms...

CAEFactory::Suspend();
}

void CPowerManager::WaitForNet()

This comment was marked as spam.

@Memphiz
Copy link
Member

Memphiz commented Jun 20, 2013

delay test spam - jenkins build this please

@Memphiz
Copy link
Member

Memphiz commented Jun 20, 2013

@team

For clarification "Good to merge" doesn't mean this PR is approved and cleared for merging. Its something the githubapi sets for us and we are not able to change that message atm. So feel informed about that

@verybadsoldier
Copy link
Contributor Author

Well I cannot offer complete failsafe solutions to the issues mentioned above regarding wait-for-net. Please help me out by providing ideas how you think the problems should be solved in a failsafe way.

I would proprose that I remove the wait-for-net functionality from this PR and just stick with the SMB-Deinit stuff. Those are two totally independent features anyway.

PS.
I did not forget NFS its just that I am not using NFS myself and I dont have a NFS server around. Once this stuff is done I might take a look at it though.

@t4-ravenbird
Copy link
Contributor

I think removing the wait-for-net (or splitting into 2 separate commits) will be a good idea, yes.

I am also not a team-member but I raised the issues because I feel the wait-for-net part overlaps with the recently added wake-on-access feature ;
If wake-on-access is enabled then there IS already a wait-for-net performed and it does the waiting on behalf on any calling-thread and wait for the appropriate nic.
If wake-on-access is disabled, however, this wait is bypassed. An easy solution to would be to allow the wake-on-access function to wait for network even if wol-transmission is not enabled. That is in effect the same as in your wait-for-net and without the mentioned pitfalls.
I suspect however, that this must go in first ; #2770 It adds a function to determine if a host is on lan and would be needed to know when to perform a wait.

@verybadsoldier
Copy link
Contributor Author

Oh fine, if the wait-for-net stuff is already there then its even better. I removed it from this PR so this is now just about deinitializing libsmbclient before going to suspend.

@xhaggi
Copy link
Member

xhaggi commented Jun 25, 2013

a nice sideeffect of stopping/starting the addons, the pvr addon now imports the epg data after suspend. without this the epg-data gets out of syn after a few days and you have to manually reset the epg data or restart xbmc.

@xhaggi
Copy link
Member

xhaggi commented Jul 5, 2013

@Memphiz would it make more sense if we split the start/stop addon stuff into a separated pull, so we can merge this in before?

@Memphiz
Copy link
Member

Memphiz commented Jul 5, 2013

yeah sounds good - those are 2 unrelated things imo...

@verybadsoldier
Copy link
Contributor Author

The SMB-fix wont work properly without stopping the addons because a thread in a addon might re-init SMB just after it got deinited but before the system actually goes to suspend. I had this case when running the Library Watchdog addon.

@xhaggi
Copy link
Member

xhaggi commented Jul 5, 2013

@verybadsoldier could you please split this in logical commits one for the addon stuff and a second for the smb.

@verybadsoldier
Copy link
Contributor Author

Sure, no problem. The splitted PR is here: #2946

@Memphiz
Copy link
Member

Memphiz commented Jul 7, 2013

Could you cherry-pick that one ontop of yours?

Memphiz@ddcfb39

If you have any chance for testing it with afp, nfs or sftp - this would be very welcome (i have no clean suspend on my rigs...). Else i would let that go in untested and wait for complaints on from users...

@verybadsoldier
Copy link
Contributor Author

Ok, done. Sorry, it seems I missed your comment before.

But I cant test the other protocols either. NFS was tested by some users already because I applied the NFS-patch to some cusom OpenELEC builds already.

@wsnipex
Copy link
Member

wsnipex commented Jul 21, 2013

jenkins test this please

pretty pleeeease :)

@wsnipex
Copy link
Member

wsnipex commented Jul 21, 2013

narf, sorry for the spam, wrong trigger.

jenkins build this please

@Memphiz
Copy link
Member

Memphiz commented Jul 21, 2013

Wrong trigger... Build vs. Test

@MartijnKaijser
Copy link
Member

@Memphiz
go for merge?

@Memphiz
Copy link
Member

Memphiz commented Aug 7, 2013

@verybadsoldier - is this the currently wanted implementation of this? I am a Bit confused with the forum thread where a different (or the Same?) Patch is discussed in conjunction with openelec... If it is the same i am Fine with merging this due to the positive feedback from oe users...

@verybadsoldier
Copy link
Contributor Author

Yes its basically the same in OE. Different in the OE patch is:
-it also incorporates this PR #2946
-it does not have the SFTP and AFP part
-it has the "wait-for-net"-feature which delays the XBMC wake up (GUI thread) until the first NIC is up. It was said that a different/better mechanism is in XBMC already. This is not related to network shares but it is meant to fix access to MySQL straight after wakeup.

Memphiz added a commit that referenced this pull request Aug 7, 2013
Fix for SMB shares after suspend
@Memphiz Memphiz merged commit 19a968e into xbmc:master Aug 7, 2013
{
CLog::Log(LOGDEBUG,"CApplication::CloseNetworkShares: Closing all network shares");

#if defined(HAS_FILESYSTEM_SMB) && !defined(_WIN32)

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants