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

[platform] - add unique hardware identifier as info label #10543

Closed
wants to merge 17 commits into from

Conversation

@Memphiz
Copy link
Member

commented Sep 23, 2016

As discussed at DevCon we would benefit from having a possibility to better identifer unique kodi instances.

This is the core work for this. ATM it simply uses the mac address of the first connected network interface and builds the MD5 of it. This value is:

  1. visible in the SystemInfo -> Hardware Section
  2. Logged to the header of the kodi.log
  3. Available as info label with the name "hardwareuuid"

For osx and ios i already implemented overrides that use proper os methods for getting a unique hardware id (which might be better as a mac address).

@FernetMenta @fritsch @Paxxi @popcornmix please have a look and in case there is something better on your platforms - implement an override class in
https://github.com/xbmc/xbmc/tree/master/xbmc/platform/overrides/*yourplatform*/Platform*Name*.cpp with overridden GetUniqueHardwareIdentifier method.

The better we get this right in the first shot - the more accurate the statistics will be!

@MartijnKaijser can you do anything good with this? (from an addon side i mean)

Beside that - we agreed that we will document the purpose of that identifier. We should do this before anything gets into production.

@mention-bot

This comment has been minimized.

Copy link

commented Sep 23, 2016

@Memphiz, thank you for improving Kodi! According to the last 5 commits, we found the potential reviewers: @davilla, @Karlson2k and @tru . Final approval needs to be given by the componenent maintainer.

@Memphiz

This comment has been minimized.

Copy link
Member Author

commented Sep 23, 2016

@chewitt fyi ping

{
static std::string uuid = NoValidUUID;

#if defined(HAS_LINUX_NETWORK) || defined(HAS_WIN32_NETWORK)

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Sep 23, 2016

Member

I would expect this in the overrides for Linux and Windows

This comment has been minimized.

Copy link
@Memphiz

Memphiz Sep 23, 2016

Author Member

but the check is for LINUX_NETWORK ... it is true for other platforms too ... its not a check for TARGET_LINUX and TARGET_WIN32

This comment has been minimized.

Copy link
@Memphiz

Memphiz Sep 23, 2016

Author Member

i will simply remove those ifdefs - if g_application.getNetwork().GetFirstConnectedInterface(); the platform is broken - there is no platform which doesn't implement that anyway ... the ifdefs where a c&p from somewhere else - i don't see why those are required at all.

@@ -47,3 +53,22 @@ void CPlatform::Init()
// nothing for now
}

std::string CPlatform::GetUniqueHardwareIdentifier()
{
static std::string uuid = NoValidUUID;

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Sep 23, 2016

Member

please make uuid a member

This comment has been minimized.

Copy link
@Memphiz

Memphiz Sep 23, 2016

Author Member

added a small refactor commit .. see last commit (adding m_uuid and InitUniqueHardwareIdentifier which is called in Init)

@popcornmix

This comment has been minimized.

Copy link
Member

commented Sep 23, 2016

@Memphiz I've added support for reading hardware serial number from /proc/cpuinfo.
This works on Pi and many linux platforms (e.g. Odroid). The serial is a 64-bit hex number (although Pi only uses lower 32-bits). My x86 Ubuntu machine doesn't support this.
Currently it is #ifdef TARGET_RASPBERRY_PI, but I think it may make sense to always try this before mac address check on linux platforms.
See: popcornmix@8292542

@Memphiz

This comment has been minimized.

Copy link
Member Author

commented Sep 23, 2016

@popcornmix thanks - sounds good in general - @FernetMenta how do we fit this in the overrides design. We can add an override rbpi which only works for rbpi - there is no way to have something for all linux systems with our principle - @popcornmix we are evaluating a new design pattern which gets rid of platform ifdefs - once we are clear about it i will take your code and add it in the new way and ping you so that you get an idea how it is meant to be done :)

@Memphiz

This comment has been minimized.

Copy link
Member Author

commented Sep 23, 2016

@popcornmix see 41a6486 - notice it only adds a cpp and a h file - the buildsystem (cmake and makefiles) will pick this up automagically because it is in the overrides folder and has a match on CORE_SYSTEM_NAME

Though this concept doesn't allow yet to have something common for linux for example (so its no 1:1 replacement for TARGET_LINUX for example)

You get the idea ...

@Memphiz Memphiz force-pushed the Memphiz:hw_identifier branch from 41a6486 to eaa9552 Sep 23, 2016

@Memphiz

This comment has been minimized.

Copy link
Member Author

commented Sep 23, 2016

@popcornmix sorry - had to force push - eaa9552 is the commit for rbpi

@Hedda

This comment has been minimized.

Copy link
Contributor

commented Sep 23, 2016

@Memphiz Tip; You can also use Date and Time to make an even more unique identifier.

That is, in case you have several Kodi installations on the same hardware.

https://en.wikipedia.org/wiki/Universally_unique_identifier#Version_1_.28date-time_and_MAC_address.29

This is what a universally unique identifier (UUID) did in Version 1 (original).

@Memphiz

This comment has been minimized.

Copy link
Member Author

commented Sep 23, 2016

@Hedda that uuid would change on each start of the software - we want a unique identifier which is as stable as possible - similar to an IMEI on phones - we want to gather some accurate usage counts of unique devices.

@Hedda

This comment has been minimized.

Copy link
Contributor

commented Sep 23, 2016

@Memphiz Ah, ok I see. Sorry I misunerstod your intended purpose.

@chewitt

This comment has been minimized.

Copy link
Member

commented Sep 23, 2016

LGTM 👍

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Sep 23, 2016

@Memphiz

This comment has been minimized.

Copy link
Member Author

commented Sep 23, 2016

@FernetMenta yes i di that in my last commit - but that means its only used on rbpi and not general on linux ... i take it - we have to duplicate that code for linux then (and fall back to the base implementation there)?

Also why is in the processinfo a check for target_raspberry_pi? Is this really needed?

@Memphiz

This comment has been minimized.

Copy link
Member Author

commented Sep 23, 2016

this is a tradeoff ... instead of having something like if defined(TARGET_LINUX) || defined(TARGET_FOO) we have to duplicate the code into overrides/linux and overrides/foo to achieve the same. Do we want that sort of code duplication just for the sake of removing ifdefs?

@chewitt

This comment has been minimized.

Copy link
Member

commented Sep 23, 2016

FWIW.. on Linux nothing apart from Pi has a useable serial identifier and the only devices we've seen where MAC address is an issue is a statistically irrelevant minority of older Amlogic devices where u-boot had a hardcoded MAC address. In our own UUID script we try MAC and if for some reason it's not available we generate our own random number and store it so it survives a reboot (but not reinstall).

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Sep 23, 2016

Also why is in the processinfo a check for target_raspberry_pi? Is this really needed?

It should not be required.

Do we want that sort of code duplication just for the sake of removing ifdefs?

I don't see this kind of code duplication as an issue. In particular not in this stage. Later, should we notice that bigger portions of code are duplicated, we can share this amongst the platforms.

@Memphiz

This comment has been minimized.

Copy link
Member Author

commented Sep 23, 2016

@chewitt well as said - odroid has and maybe other embedded platforms - i'll add the serial check to linux aswell and fallback to macaddr if not found - better save then sorry :)

@Memphiz

This comment has been minimized.

Copy link
Member Author

commented Sep 23, 2016

@FernetMenta ok - i'll add overrides for win32, linux and android then ...

@Paxxi
Copy link
Member

left a comment

Will have a look at win32 and see what we can use

{
public:
/**\brief C'tor */
CPlatformRbpi();

This comment has been minimized.

Copy link
@Paxxi

Paxxi Sep 23, 2016

Member

Use = default; instead of having empty definitions

This comment has been minimized.

Copy link
@Memphiz

Memphiz Sep 23, 2016

Author Member

@Paxxi thx for the =default trick - changed ... (and learned something new)

@stefansaraev

This comment has been minimized.

Copy link
Contributor

commented Sep 23, 2016

In our own UUID script we try MAC and if for some reason it's not available we generate our own random number and store it so it survives a reboot (but not reinstall)

@chewitt perhaps a bit irrelevant. but you should not track users by mac address. you should use systemd's machineid instead.

@Memphiz

on linux, if /etc/machine-id exists, use that.

on android: https://developer.android.com/reference/android/provider/Settings.Secure.html#ANDROID_ID

on windiws: every windows machine has unique id that can be fetched via wmi, not sure if that's easy to do from kodi.

Memphiz added 2 commits Sep 25, 2016
[PlatformRbpi] - added PlatformRbpi override and implemented InitUniq…
…ueHardwareIdentifier (by using the serialnumber from /proc/cpuinfo)
[PlatformWindows] - added PlatformWindows override and implemented In…
…itUniqueHardwareIdentifier (by using registry key HKEY_LOCAL_MACHINE\Software\Microsoft\Cryptography\MachineGuid)

@Memphiz Memphiz force-pushed the Memphiz:hw_identifier branch from 1f4c645 to 969af88 Sep 25, 2016

@Memphiz

This comment has been minimized.

Copy link
Member Author

commented Sep 25, 2016

@MartijnKaijser added a description to the language change (i don't want this to be translated at all - to prevent stupid ideas of how to translate this ;) ). My time is over for now next up is my move into the flag - could you somehow ensure that platform devs test this / sign it off and then take care of the merge in case we go forward to next beta?

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Sep 25, 2016

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2016

On RPi, I see the HardwareUUID being logged correctly (http://sprunge.us/aYGO) but it's not appearing in the System Info > Hardware section:

s1

I think there's not enough labels. If I apply this patch on top of this PR, then I see:

s2

@Hedda

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2016

Tip; UUID is in most cases displayed in a format with inserted hyphen (dash) characters:

e.g. 600e2d98-c0bf-4d6b-be53-88de15a01346 or {600E2D98-C0BF-4D6B-BE53-88DE15A01346}

https://en.wikipedia.org/wiki/Universally_unique_identifier

The uses of hyphens (dashes) makes it a little more human-readable when displayed.

Also, using all lowercase as standard on all platforms is also more common than using all uppercase

https://henbo.wordpress.com/2007/12/02/the-mystery-of-upper-case-and-lower-case-guid-values/

@Memphiz

This comment has been minimized.

Copy link
Member Author

commented Sep 26, 2016

@Hedda i know you mean good. But i would like to avoid a change on all platform files again for the sake of what is imo cosmetics. I only show the uuid in the GUI for transparency reasons. It is not ment to be human readable. If you want to change this you can do a PR against my repo no problem.

@wsnipex

This comment has been minimized.

Copy link
Member

commented Sep 26, 2016

quick test on ubuntu, shows the UUID in system info and log.

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Sep 26, 2016

Windows shows
17:11:26 T:12744 NOTICE: HardwareUUID: E8F4044081A6C078F55F926F0F2CD05E

jenkins build this please

@Paxxi
Paxxi approved these changes Sep 26, 2016
Copy link
Member

left a comment

Windows bits looks fine

@popcornmix

This comment has been minimized.

Copy link
Member

commented Sep 26, 2016

If the md5sum of the mac address is used as a way of anonymising the data I wonder if it would be better moved up one layer so it also applies to platform specific implementations too.
Technically the Pi's MAC can be determined from the serial number, so I wonder if that should really be md5summed.
Or the md5sum sum could be added to the Pi specific implementation.

@wsnipex
Copy link
Member

left a comment

I know its annoying, but if we want to merge this for v17, we really also need autotools fixed up

if (fgets(line, sizeof line, f) != nullptr)
{
m_uuid = line;
#if defined(_DEBUG)

This comment has been minimized.

Copy link
@wsnipex

wsnipex Sep 26, 2016

Member

don't we have log levels for a reason?

@Memphiz

This comment has been minimized.

Copy link
Member Author

commented Sep 26, 2016

@popcornmix could you clarify your comment please? I don't understand it :) - if it is about using the mac address instead of the serialnumber - i wanted to use mac only in worst case scenarios where everything else failed. Because i don't trust the network interfaces being up when needed (well as of murphy we will init the uuid in exactly that moment where the if is down ...) - thats why i did the specific implementations in the first place - to have something that works from the first second and can't be easily changed (which is at least true for ios/osx/android and maybe windows ...)

@Memphiz

This comment has been minimized.

Copy link
Member Author

commented Sep 26, 2016

logging of uuid removed for privacy reasons (logs from forum users would allow to bind usernames to uuids ...)

MilhouseVH referenced this pull request in phil65/skin.estuary Sep 26, 2016
@Memphiz

This comment has been minimized.

Copy link
Member Author

commented Sep 28, 2016

Jenkins build this please

@ronie

This comment has been minimized.

Copy link
Member

commented Sep 29, 2016

may i suggest to start a public discussion on the forum before merging this?

@kib

This comment has been minimized.

Copy link
Member

commented Oct 23, 2016

For forum readers visiting github:
It is not silent on this subject, but we are having an internal discussion about it currently before involving the rest of the community. This code is not merged and likely will not be merged until both a team consensus on scope and size and a public involvement on the forum has taken place. That being said, this PR is about the technical implementation of such a UUID if we want to start using it for the purpose of correct statistics.

Team members can find my lengthy post on intended usage and on what this should absolutely not do as discussed during Devcon here in the team discussion thread.

Further comments on the PR should be technical in dealing with how to properly do a UUID on different platforms.

@tamland tamland referenced this pull request Oct 24, 2016
@Rechi

This comment has been minimized.

Copy link
Member

commented Nov 6, 2017

@Memphiz this needs a rebase

@Memphiz

This comment has been minimized.

Copy link
Member Author

commented Nov 6, 2017

Not Interested in working on this any further - sorry ...

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.