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

[xbmc] Start work on getting rid of our emulated GetDiskFreeSpaceEx method #11741

Merged
merged 3 commits into from Feb 27, 2017

Conversation

@Paxxi
Copy link
Member

commented Feb 24, 2017

Decided to unify some of our low level file handling
methods on the std::filesystem idea. As it's not standardized
yet and implementations are experimental this is a simple
version that suits our current needs. The idea is that once platforms
start offering non-experimental versions of std::filesytem we can
easily move over to using them.

Description

Motivation and Context

How Has This Been Tested?

runtime tested on windows

Screenshots (if appropriate):

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the Code guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the CONTRIBUTING document
  • I have added tests to cover my change
  • All new and existing tests passed
Decided to unify some of our low level file handling
methods on the std::filesystem idea. As it's not standardized
yet and implementations are experimental this is a simple
version that suits our current needs. The idea is that once platforms
start offering non-experimental versions of std::filesytem we can
easily move over to using them.
@Paxxi

This comment has been minimized.

Copy link
Member Author

commented Feb 24, 2017

jenkins build this please

1 similar comment
@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Feb 25, 2017

jenkins build this please

@Paxxi

This comment has been minimized.

Copy link
Member Author

commented Feb 25, 2017

@Montellese thoughts?
maybe @wsnipex for linux and @koying for android and @Memphiz for osx

auto result = statfs(CSpecialProtocol::TranslatePath(path).c_str(), &fsInfo);
#else
struct statfs64 fsInfo;
auto result = statfs64(CSpecialProtocol::TranslatePath(path).c_str(), &fsInfo);

This comment has been minimized.

Copy link
@wsnipex

wsnipex Feb 25, 2017

Member

quote from the linux standards base: https://refspecs.linuxbase.org/LSB_5.0.0/LSB-Core-generic/LSB-Core-generic/baselib-statfs64.html

Note: Application developers should use the statvfs64() function to obtain general file system information. Applications should only use the statfs64() function if they must determine the file system type, which need not be provided by statvfs64().

This comment has been minimized.

Copy link
@Paxxi

Paxxi Feb 25, 2017

Author Member

I just copied the code from current implementation, will fix

ULTotalTmp.QuadPart+= ULTotal.QuadPart;
ULTotalFreeTmp.QuadPart+= ULTotalFree.QuadPart;
}
total = space("/", ec);

This comment has been minimized.

Copy link
@koying

koying Feb 25, 2017

Contributor

This actually makes no sense on unix-derived platforms ;)
Not your fault, obviously

Edit:
To be clearer, finding out the available disk space on the root fs only gets you a very partial picture, as unix often has partitions mounted here and there, for which you'll get no info by only checking rootfs

This comment has been minimized.

Copy link
@Paxxi

Paxxi Feb 25, 2017

Author Member

Yeah it should probably fetch the mounts and iterate over them as on windows?

This comment has been minimized.

Copy link
@koying

koying Feb 25, 2017

Contributor

Yeah, but even that doesn't make much sense to get "Free Disk Space"...
What is Windows doing when not passed an actual path?

This comment has been minimized.

Copy link
@Paxxi

Paxxi Feb 25, 2017

Author Member

Windows only cares about a disk or mount so we pass it c: d: and so on.
Possible to mount in directories like Linux but afaik I've never seen it used on windows so we don't support it.

Suggestions on what would make sense?

This comment has been minimized.

Copy link
@Paxxi

Paxxi Feb 25, 2017

Author Member

Right, looking at IStorageProvider the GetDiskUsage method might be suitable to use but maybe add an overloaded version that returns the numbers and not formatted strings

This comment has been minimized.

Copy link
@koying

koying Feb 25, 2017

Contributor

Suggestions on what would make sense

No, really...
Seems only used from

if (g_sysinfo.GetDiskSpace("", total, totalFree, totalUsed, percentFree, percentused))
and the whole concept of "GetHddSpaceInfo" seems bogus to me ;)

This comment has been minimized.

Copy link
@koying

koying Feb 25, 2017

Contributor

The thing is untouched since years. I bet it still dates from single disk / single partition xbox ;)

This comment has been minimized.

Copy link
@Paxxi

Paxxi Feb 25, 2017

Author Member

and that method is called from CGUIInfoManager so it's likely that some random skin out there uses it, I'm all for dropping it but maybe @phil65 or @ronie has something to say about it.

This comment has been minimized.

Copy link
@ronie

ronie Feb 25, 2017

Member

correct, there are a number of skins that use the System.FreeSpace / System.TotalSpace infolabels.

they are either shown in some home screen widget or in the filemanager window.

@koying

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2017

Wouldn't hooking into our StorageProvider a valid alternative?

@Paxxi

This comment has been minimized.

Copy link
Member Author

commented Feb 25, 2017

Our storage provider is a bit higher in the hierarchy for what I want to achieve with the filesystem methods, intending to implement stat and possibly the directory iterators so we have a common low level abstraction and can reduce some ifdefs.

Using storage manager for the sysinfo might be a good idea, will have a look at it and see.

if (result != 0)
{
ec.assign(result, std::system_category());
sp.available = static_cast<uintmax_t>(-1);

This comment has been minimized.

Copy link
@thebrid

thebrid Feb 25, 2017

Contributor

Consider using std::numeric_limits<uintmax_t>::max()

This comment has been minimized.

Copy link
@Paxxi

Paxxi Feb 25, 2017

Author Member

standards draft uses static_cast http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0218r0.html#space_info

Doesn't make any real difference but avoids an include :)

This comment has been minimized.

Copy link
@thebrid

thebrid Feb 25, 2017

Contributor

👍

sp.free = static_cast<uintmax_t>(-1);
return sp;
}
sp.available = static_cast<uintmax_t>(fsInfo.f_bavail * fsInfo.f_bsize);

This comment has been minimized.

Copy link
@wsnipex

wsnipex Feb 25, 2017

Member

note: while it's probably negligible for our purpose, but this is incorrect when kodi is run as root, because file systems reserve a certain percentage of the disk space for root(usually 5%-10%)
I'd still keep it as is, because distros should really stop running kodi as root

This comment has been minimized.

Copy link
@Paxxi

Paxxi Feb 25, 2017

Author Member

Reading https://www.ibm.com/support/knowledgecenter/ssw_i5_54/apis/statvf64.htm it seems like a strange design that it specifically mentions non-privileged users. I'm thinking it should report the correct amount depending on user and quotas but maybe that's left to another api?

The draft standards mentions it should report the f_bavail * f_bsize so we should be good in that regard http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0218r0.html#space_info

This comment has been minimized.

Copy link
@stefansaraev

stefansaraev Feb 25, 2017

Contributor

first of all, not all filesystems reserve root-only blocks. and second, you dont have to care about that. no matter if kodi is run as root or not. those blocks are not to be reported in a different way depending on current uid. even fs specific tools like "df" dont care, you should not. too.

EDIT: just mentioning this because it's not really kodi's (nor any other userspace application) job to deal with reserved fs blocks, and because I have seen enough invalid reasons not to run kodi as root ;)

This comment has been minimized.

Copy link
@Paxxi

Paxxi Feb 25, 2017

Author Member

Thank you for the explanation, at least we're all in agreement on this line of code. Let the battle over root vs nonroot continue in all its glory 😊


#if defined(TARGET_LINUX)
#include <sys/statvfs.h>
#elif defined(TARGET_DARWIN) && defined(TARGET_FREEBSD)

This comment has been minimized.

Copy link
@Memphiz

Memphiz Feb 26, 2017

Member

Should this be an "or" instead?

This comment has been minimized.

Copy link
@Paxxi

Paxxi Feb 26, 2017

Author Member

It certainly should!

@Memphiz

This comment has been minimized.

Copy link
Member

commented Feb 26, 2017

Looks straight forward to me

Paxxi added 2 commits Feb 25, 2017
It's wrong on all systems in different ways so let's
keep it fairly simple and return only for a specified mount
or default to / or C:
@Paxxi Paxxi force-pushed the Paxxi:fs1 branch from 35d0ce6 to 4aee73a Feb 26, 2017
@Paxxi

This comment has been minimized.

Copy link
Member Author

commented Feb 26, 2017

Fixed the invalid test for TARGET_BSD / TARGET_DARWIN

Dropped the counting of "total" space on win32 as it's a useless metric, added some comments but kept the GetDiskspaceInfo method for backward compat with skins that rely on it.

jenkins build this please

@Paxxi

This comment has been minimized.

Copy link
Member Author

commented Feb 27, 2017

jenkins build this please

@Paxxi

This comment has been minimized.

Copy link
Member Author

commented Feb 27, 2017

build error unrelated.

@Paxxi Paxxi merged commit 85ec975 into xbmc:master Feb 27, 2017
2 of 3 checks passed
2 of 3 checks passed
jenkins4kodi You are a failure. Fix the code and try again......
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Paxxi Paxxi deleted the Paxxi:fs1 branch Feb 27, 2017
@MartijnKaijser MartijnKaijser modified the milestone: L 18.0-alpha1 Feb 28, 2017
@Rechi Rechi added the v18 Leia label Mar 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.