[win32] Some fixes for usb scan #1187

Merged
merged 10 commits into from Nov 9, 2013

Projects

None yet

5 participants

@Karlson2k
Member

Added error checks.
Used other WINAPI function for PID and VID extraction.

@Karlson2k
Member

The only question:
Is m_strLocation from result really used anywhere except device identification?
I check the code and didn't find any other usage. Did I miss something?

@Karlson2k
Member

This is subset of changes from #1189, but it could be applied independently.

@wsoltys
Member
wsoltys commented Jul 23, 2012

@opdenkamp: mind having a look?

@opdenkamp
Member

will do asap. regarding your optimisations, you are optimising malard's copy of an msdn example, cause that is where the windows usb scan comes from for the most part :)

@opdenkamp opdenkamp was assigned Jul 23, 2012
@Karlson2k
Member

@opdenkamp The idea about memory preallocation was inspired by 'devcon' example from Windows DDK.
So in general - is't just a shuffle of MS examples. :)

@Karlson2k
Member

Some refactoring added

@Karlson2k
Member
@opdenkamp
Member

yes I'll look into it. merge window isn't open yet anyway ;-)

@opdenkamp
Member

to answer your question: m_strLocation is only used to id devices indeed. as long as the value is unique, it'll be fine

@opdenkamp opdenkamp and 1 other commented on an outdated diff Jul 29, 2012
xbmc/peripherals/bus/win32/PeripheralBusUSB.cpp
{
- SetupDiDestroyDeviceInfoList(hDevHandle);
- return bReturn;
+ if (GetLastError() != ERROR_INSUFFICIENT_BUFFER)
+ continue;
+ free(deviceProperty);
+ nBufferSize = required;
+ deviceProperty = (PTSTR)malloc(nBufferSize);
+ if(deviceProperty == NULL)
+ break;
+ if (!SetupDiGetDeviceRegistryProperty(hDevHandle, &devInfoData, SPDRP_HARDWAREID, NULL, (PBYTE) deviceProperty, nBufferSize, &required))
+ continue;
@opdenkamp
opdenkamp Jul 29, 2012 Member

all these breaks and continues are definitely not making it more readable.

@Karlson2k
Karlson2k Jul 29, 2012 Member

I can add some comments.
But I think that "break = exit" and "continue = get next one" looks enough logical.

@opdenkamp
opdenkamp Jul 30, 2012 Member

well, maybe looks logical and maybe I'm oldstyle, but I think it's more readable without them and using if statements. I must confess that I'm sometimes a sinner against this too, but I usually clean it up shortly after adding it :)

it would be a quick change for you, and then this code will match the coding style of the other classes in /peripherals nicely.

@opdenkamp opdenkamp and 1 other commented on an outdated diff Jul 29, 2012
xbmc/peripherals/bus/win32/PeripheralBusUSB.cpp
- return bReturn;
+ CStdString strVendorId(StringUtils::EmptyString);
@opdenkamp
opdenkamp Jul 29, 2012 Member

just declare these where you assign them and drop StringUtils::EmptyString

@Karlson2k
Karlson2k Jul 29, 2012 Member

OK. I'll do it.
It's old part of code, I didn't touched it. :)

@opdenkamp
opdenkamp Jul 30, 2012 Member

sure, might even have written it myself once. but now that you're optimising that, this is just one more :)

@opdenkamp opdenkamp and 1 other commented on an outdated diff Jul 29, 2012
xbmc/peripherals/bus/win32/PeripheralBusUSB.cpp
- return bReturn;
+ CStdString strVendorId(StringUtils::EmptyString);
+ CStdString strProductId(StringUtils::EmptyString);
+
+ // actually deviceProperty is REG_MULTI_SZ, but we need only the first string from it
+ CStdString strTmp(deviceProperty);
@opdenkamp
opdenkamp Jul 29, 2012 Member

please use more meaningful var names than this

@Karlson2k
Karlson2k Jul 29, 2012 Member

This it a really device property from registry. :) Coming from 'SetupDiGetDeviceRegistryProperty'

@opdenkamp
opdenkamp Jul 30, 2012 Member

I was talking about strTmp

@opdenkamp opdenkamp and 1 other commented on an outdated diff Jul 29, 2012
xbmc/peripherals/bus/win32/PeripheralBusUSB.cpp
- return bReturn;
+ CStdString strVendorId(StringUtils::EmptyString);
+ CStdString strProductId(StringUtils::EmptyString);
+
+ // actually deviceProperty is REG_MULTI_SZ, but we need only the first string from it
+ CStdString strTmp(deviceProperty);
+ strTmp.MakeLower();
+ int pos;
+ if ((pos=strTmp.Find("vid_")) < 0)
+ continue;
+ strVendorId = strTmp.substr(pos + 4, 4);
+ if ((pos=strTmp.Find("pid_")) < 0)
@opdenkamp
opdenkamp Jul 29, 2012 Member

please put the two finds in 1 if statement, invert them, and drop the two continues. makes it a lot more readable

@Karlson2k
Karlson2k Jul 29, 2012 Member

OK, no problem.

But we'll need one more variable in case of combining in one 'if', so readability will be preferred over efficiency.

@opdenkamp
opdenkamp Jul 30, 2012 Member

well, this code is executed once every 5 seconds when no device changes are reported by the os, and it's not blocking anything. and even if a device change comes in, I don't think the user will notice the delay of assigning one more int when inserting a new device :)

@opdenkamp opdenkamp and 1 other commented on an outdated diff Jul 29, 2012
xbmc/peripherals/bus/win32/PeripheralBusUSB.cpp
+ if ((pos=strTmp.Find("pid_")) < 0)
+ continue;
+ strProductId = strTmp.substr(pos + 4, 4);
+
+ PeripheralScanResult prevDevice;
+ if (!results.GetDeviceOnLocation(deviceProperty, &prevDevice))
+ {
+ PeripheralScanResult result;
+ result.m_strLocation = deviceProperty;
+ result.m_iVendorId = PeripheralTypeTranslator::HexStringToInt(strVendorId.c_str());
+ result.m_iProductId = PeripheralTypeTranslator::HexStringToInt(strProductId.c_str());
+
+ static const CStdString strHidClass("HIDClass");
+ // Assume that buffer is more then enough. If not - just skip type detection.
+ if (SetupDiGetDeviceRegistryProperty(hDevHandle, &devInfoData, SPDRP_CLASS, NULL, (PBYTE) deviceProperty, nBufferSize, &required) &&
+ strHidClass.compare(deviceProperty)==0 )
@opdenkamp
opdenkamp Jul 29, 2012 Member

can't you just use strcmp for this and avoid the CStdString?

@Karlson2k
Karlson2k Jul 29, 2012 Member

It was in-line with other code.
Thanks, I'll convert it, it will be definitely faster.

@Karlson2k
Member

It's mostly a bugfixes, no a featureadding, so no need to wait for merge window.

@opdenkamp
Member

which bug is it fixing exactly? not that I'm shocked to hear that there were bugs in an msdn example, but this looks like a cleanup round to me, not something that needs to be in before the next merge round.

@Karlson2k Karlson2k and 1 other commented on an outdated diff Jul 30, 2012
xbmc/peripherals/bus/win32/PeripheralBusUSB.cpp
- results.m_results.push_back(result);
- }
- }
+ PeripheralScanResult result;
+ result.m_strLocation = deviceProperty;
+ result.m_iVendorId = PeripheralTypeTranslator::HexStringToInt(strVendorId.c_str());
+ result.m_iProductId = PeripheralTypeTranslator::HexStringToInt(strProductId.c_str());
+
+ // Assume that buffer is more then enough. If not - just skip type detection.
+ if (SetupDiGetDeviceRegistryProperty(hDevHandle, &devInfoData, SPDRP_CLASS, NULL, (PBYTE) deviceProperty, nBufferSize, &required) &&
+ strcmp("HIDClass", deviceProperty)==0 )
+ result.m_type = PERIPHERAL_HID;
+ else
+ result.m_type = defaultType;
+
+ if (!results.ContainsResult(result))
@Karlson2k
Karlson2k Jul 30, 2012 Member

I was copied from old code.

Do we really need this double check? It was checked already in 'if (!results.GetDeviceOnLocation(deviceProperty, &prevDevice))'

@opdenkamp
opdenkamp Jul 30, 2012 Member

yes please leave it there for now. I need to change the method to add results when I got time so you don't need that extra call.

@Karlson2k
Karlson2k Jul 30, 2012 Member

So may be it's better to remove first check: 'if (!results.GetDeviceOnLocation(deviceProperty, &prevDevice))' ?

@Karlson2k
Member

@opdenkamp Code is updated.
About bugs. The old code missing a lot of checks: memory allocation, string really found.
Anyway, optimizations are added between merge rounds.
But I'd like to avoid this discussion any deeply as it does not make sense as merge window is coming. :)

@Karlson2k
Member

@opdenkamp More corrections?

@opdenkamp
Member

what do you mean?

@Karlson2k
Member

@opdenkamp Is code ok now? :)

@opdenkamp
Member

I'll have a look when I have time to test this on windows.

@Karlson2k
Member

Nice!
Thanks.

@Karlson2k
Member

@opdenkamp Code is updated a little.
Ready to test. ;)

@opdenkamp
Member

still haven't booted into windows. will probably be working on windows tomorrow.

@Karlson2k
Member

@opdenkamp Still waiting. ;)

@MartijnKaijser
Member

@Karlson2k
Learn to have patience.

@Karlson2k
Member

@MartijnKaijser Learn to distinguish between impatience and reminder.

@opdenkamp
Member

no, you need to have patience. sometimes people have other things to do than reviewing PRs

@Karlson2k
Member

Really, I fully understand that here we have no obligations or something like this and all have a lot of things to do, not only open source coding.
And I have a lot of patience.
But I'd like to remind about thing promised to be 'tomorrow' after 3 days, without any offense.
And I don't want to "learn" anything here, except good coding.
Thanks for understanding.

@opdenkamp
Member

I didn't promise anything. I said "probably"

@Karlson2k
Member

OK.
Any chance that code will be reviewed within this merge window?

@MartijnKaijser
Member

Maybe

@Karlson2k
Member

@opdenkamp Rewritten from the scratch.
Now carefully follow you coding style. :)

@opdenkamp
Member

thanks, but this is still a mix of cosmetics and multiple fixes/changes. please stick each fix in a separate commit.

also, your quote from msdn about parsing SetupDiGetDeviceInterfaceDetail is just a snippet, out of it's context. the result can be passed just fine, but the remark is about parsing the result

@Karlson2k
Member

OK, I'll separate all fixes, changes and cosmetics.

About SetupDiGetDeviceInterfaceDetail. There is no garantee that DevicePath will contain device ID. It is just the string that can be used with other functions for specify device in system. Yes, it contains ID now, but we can't be sure that it will contain ID in all possible cases and this behavor will not be changed with future Windows version or service pack. The only function that returns hardware ID in all cases is SetupDiGetDeviceRegistryProperty with parameter SPDRP_HARDWAREID .
And yes, result can be passed, but you shold not parse (analyze, decompose, split) result.

@opdenkamp
Member

right, we are parsing the info that is returned, wasn't reading the original code properly. still, until someone can show me a case in which this doesn't work, there's no need to change it. add a comment if you like. please note that we don't care about the location or anything (as long as that value is unique, which it is) and only care about the vid and pid, which are present in every version of windows i've tested, from xp to win7 to embedded.

so please just provide one or more commits with fixes for bugs you encounter yourself, add a commit with cosmetics if you like, and leave it at that. we're spending far too much time on this, which i explained, will not fix any issue that has been reported by users as far as i can tell.

@Karlson2k
Member

@opdenkamp One change - one commit.
BTW about real problems - PR #1189 is still waiting

@elupus
Member
elupus commented Aug 4, 2013

Is this still interesting?

@opdenkamp
Member

Yeah possibly, although noone has actually had an issue with the current code :)

Op 4 aug. 2013 om 20:43 heeft Joakim Plate notifications@github.com het volgende geschreven:

Is this still interesting?


Reply to this email directly or view it on GitHub.

@opdenkamp
Member

gave it a quick review. just some minor cosmetics and optimisations, and a rebase is needed. could you take another look at the cosmetics please @Karlson2k cause i think i might have missed some extra spaces when i just looked at it ;-)

@Karlson2k
Member

@opdenkamp Nice! I'll update it soon.

@Karlson2k
Member

@opdenkamp Is it OK if I replace CStdString with std::string?

@opdenkamp
Member

sure

@MartijnKaijser
Member

@Karlson2k
needs a rebase and replace CStdString with std::string

@opdenkamp
Member

no shit spammer :)

@Karlson2k
Member

@opdenkamp Rebased, CStdString removed.
Last three commits are up to you.

@Karlson2k
Member

@opdenkamp Done. :)

@Karlson2k
Member

jenkins build this please

@opdenkamp opdenkamp merged commit a24d048 into xbmc:master Nov 9, 2013

1 check passed

default Merged build #505 succeeded in 1 hr 19 min
Details
@Karlson2k Karlson2k deleted the Karlson2k:Win32_Fix_UsbScan branch Nov 23, 2013
@tru tru added a commit to plexinc/plex-home-theater-public that referenced this pull request May 19, 2014
@tru tru Fix channel preferences dialog.
Fixes #1187

We needed to be a bit more careful about copying memory before passing
it to rapidxml. This uses a scoped array to do that which guarantees
that we can’t override the other memory.
2d74406
@tru tru added a commit to RasPlex/plex-home-theatre that referenced this pull request Aug 21, 2014
@tru @LongChair tru + LongChair Fix channel preferences dialog.
Fixes #1187

We needed to be a bit more careful about copying memory before passing
it to rapidxml. This uses a scoped array to do that which guarantees
that we can’t override the other memory.
2b5fd75
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment