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

LIB-43: Use first CD drive letter on Windows as default device #20

Merged
merged 5 commits into from Aug 11, 2013

Conversation

@phw
Copy link
Member

phw commented Apr 23, 2013

Currently if the default device is used on Windows it tries to access the hardcoded drive "D:". The patch uses the first CD drive instead.

The code is completely stolen from Picard (https://github.com/musicbrainz/picard/blob/master/picard/util/cdrom.py#L33), I just translated it back to C.

EDIT by JonnyJD:
tracked with http://tickets.musicbrainz.org/browse/LIB-43
EDIT by JonnyJD:
updated title to match ticket title

@JonnyJD

This comment has been minimized.

Copy link
Contributor

JonnyJD commented Apr 23, 2013

I kind of hoped this is already what cdaudio does on Windows. Now looking at the code, this just a placeholder for D: in the code..

Similar to Mac, some code to accept drive numbers (1,2,...) would be nice.
It is not as much of an issue on Windows, since drive letters are fixed, whether a disc is in the drive or not.
That is different for Mac. Mac disc device names are fairly unstable.
On Mac this does seem to be difficult (or I just didn't find anything), unless we use a drutil system call.
See: http://tickets.musicbrainz.org/browse/LIB-28

As an additional note:
We should never have accepted the empty string as a placeholder for the DEFAULT_DEVICE, but now we probably shouldn't change that.
On Linux we just take what we get as a device. If it was given as NULL, DEFAULT_DEVICE is used, otherwise it better be a working device.

@JonnyJD

This comment has been minimized.

Copy link
Contributor

JonnyJD commented Apr 23, 2013

Wait, isn't that what happens on Anything using Windows XP or higher, using cdaudio?

Please also have a look at #5. This is basically a change in the pipeline and it involves removing Windows 9x completely.
So it might be no issue at all that the "cdaudio" thing only works in Windows XP.

Not sure about the drive number thing. Like I said, drive letters are fairly stable on Windows.

@phw

This comment has been minimized.

Copy link
Member Author

phw commented Apr 23, 2013

Wait, isn't that what happens on Anything using Windows XP or higher, using cdaudio?

Not sure if I understand you, but yes, as I understand it the code I replaced is used on XP or higher (I tested on Win7). I have not looked in all details at issue #5, but it seems to unify the code nicely and get's rid of the hardcoded drive letter. So if that would be merged my patch here is obsolete. Are there any open issues with the code of #5?

Not sure about the drive number thing. Like I said, drive letters are fairly stable on Windows.

I see no need to change anything here. Calling it "D" or "4" doesn't make much difference, but "4" would be pretty unusual on Windows.

That is quite a long line. In particular it is longer than 80 characters. Possibly using a define isn't the best option.

I prefer a define over a function call here. Is it possible to split the line with \ here? But it isn't that long anyway and it's only a single line, doesn't harm the readability much.

@phw

This comment has been minimized.

Copy link
Member Author

phw commented Apr 23, 2013

Wait, isn't that what happens on Anything using Windows XP or higher, using cdaudio?

Not sure if I understand you, but yes, as I understand it the code I replaced is used on XP or higher (I tested on Win7). I have not looked in all details at issue #5, but it seems to unify the code nicely and get's rid of the hardcoded drive letter. So if that would be merged my patch here is obsolete. Are there any open issues with the code of #5?

Sorry, I was too fast here. I had missed your comment on #5, I'll comment there. And there is still the hardcoded "D:" which could be replaced by my code above.

@phw phw mentioned this pull request Apr 23, 2013
@JonnyJD

This comment has been minimized.

Copy link
Contributor

JonnyJD commented Apr 23, 2013

Yes, the data-tracks branch still includes D: as default_device. However, was that done the wrong way?

In the master branch D: is only used when GetVersion() < 0x80000000. Otherwise cdaudio is used as a default.

So what does GetVersion() < 0x80000000 do? I would think it marks some old version, but return mb_disc_unportable_nt suggests that is the Windows NT branch (which is also used in Windows XP/7 etc).
In that case cdaudio is something that only worked in Windows 9x.

@JonnyJD

This comment has been minimized.

Copy link
Contributor

JonnyJD commented Apr 23, 2013

I found what that GetVersion() line is supposed to do:
http://msdn.microsoft.com/en-us/library/windows/desktop/ms724439%28v=vs.85%29.aspx
(make sure you enable some extra JS scripts on that page to enable the user comments)
A user comment quoted an older MSDNA (the one I couldn't find):

From older MSDN:
To distinguish between operating system platforms,
use the high order bit and the low order byte,
as shown in the following table:

Windows NT
    High order bit: 0
    Low order byte (major version number): 3 or 4

Windows 95 and Windows 98
    High order bit: 1
    Low order byte (major version number): 4

Win32s with Windows 3.1
    High order bit: 1
    Low order byte (major version number): 3

For Windows NT and Win32s, the remaining bits in the high order word specify the build number.
For Windows 95 and Windows 98, the remaining bits of the high order word are reserved.

So GetVersion() < 0x80000000 just checks if the highest bit is 0, which means it is NT-based.

And that in turn means cdaudio as default device is only directly supported on deprecated Windows (9x etc.).

@JonnyJD

This comment has been minimized.

Copy link
Contributor

JonnyJD commented Apr 23, 2013

The whole "default device generation thing" should probably go to mb_disc_get_default_device_unportable(). Possibly combined with changing or removing MB_DEFAULT_DEVICE.

If you have time for that. I can also do that later. Your code is also useful if not.
That part of the code is already a bit of a mess before your patch, so you don't necessarily have to clean that up.

The problam with not having discid_get_default_device() as a real constant (not dependent on the actual machine) is that it might not work on integration tests.
I am well aware of that and that is already the case with the Darwin/Mac implementation for quite some time.
In python-discid I only test if DEFAULT_DEVICE is not None.
So it can be the empty string (but should not be NULL in the end).

@phw

This comment has been minimized.

Copy link
Member Author

phw commented Apr 24, 2013

I would definitely add this code to mb_disc_get_default_device_unportable() in case we add it to the code in #5. You can't really rely on any value here anyway due to the different platforms. Making sure it returns an empty string at least makes sense, though, as that's how the generic implementation handles it.

What is your plan to move forward? I'd suggest not to merge this here but to integrate it into #5 and continue the discussion there.

@JonnyJD

This comment has been minimized.

Copy link
Contributor

JonnyJD commented Apr 24, 2013

Yes, doing the merge of win32 and win32_new in #5 before actually changing things like this would be the best way to go.
Splitting #5 up in two branches works, but that only makes merging the TOC changes for Unix easy. That one commit for Windows (27888a1) includes the TOC changes, so it shouldn't be rebased in a branch without the toc changes.
I do would like to have the TOC changes in a separate branch for a while to make some tests.
Maybe I can see if I can revert the toc changes for the "windows_rename" branch. Not sure if rebasing and reverting is the way to go here. Maybe just doing a new commit for the win32/win32_new merge is a bit more clean and we only keep the other commit for reference for the toc branch.

Not sure when that can happen. Don't wait for this, this week. I do intend to make it for the next release though, since your change in itself is fine and doesn't need as much testing as raw ISRCs or the toc changes. (the toc change possibly won't be in the next release)

@JonnyJD

This comment has been minimized.

Copy link
Contributor

JonnyJD commented Apr 24, 2013

I thought I could still rely on DEFAULT_DEVICE being True (as in, not None/NULL, not 0, not ""), but like I said, that broke with Mac. Of course you can't check the actual value generically.
Maybe returning "D:" as a default if no disc device is found at all (possibly on a CI machine) is an option.

Same goes, for Mac and "1", but for that to work I need an implementation using numbers.
"/dev/rdisk2" is another option for a default when no drive is found at all.

@JonnyJD

This comment has been minimized.

Copy link
Contributor

JonnyJD commented Jun 29, 2013

The build failure in jenkins is probably only because "make dist" didn't work back at that time. Merging master or rebasing would probably fix this.
This branch needs to incorporate changes from master anyways.

@phw

This comment has been minimized.

Copy link
Member Author

phw commented Jun 30, 2013

I have updated the branch based on the current master and our recent discussions. get_default_device now returns the first available drive and falls back to D: if no drive was found. This more closely follows the proposed pull request #24 for unix systems.

DWORD mask = GetLogicalDrives();
for (i = 0; i <= 25; i++) {
if (mask >> i & 1) {
sprintf_s(default_device, MAX_DEV_LEN, "%c:", i + 'A');

This comment has been minimized.

Copy link
@JonnyJD

JonnyJD Jun 30, 2013

Contributor

We should probably use an intermediate variable for this and only save the result in the global variable,
since we hand out a pointer to the global variable to the user.

Making the default_device thread-local is an additional option, but that is a bit more difficult since this should work with gcc (mingw, __thread) and MSVC (declspec?).

This comment has been minimized.

Copy link
@JonnyJD

JonnyJD Jun 30, 2013

Contributor

You also can't use sprintf_s here, since it only works with MSVC. Use snprintf, which has defines for MSVC.
The usage of snprintf is fine, even in MSVC, since we can make sure that the buffer is long enough.

This comment has been minimized.

Copy link
@phw

phw Jun 30, 2013

Author Member

Didn't think about non-MS, sorry. Will change that. The threading issue is a good point, that should be reviewed in https://github.com/metabrainz/libdiscid/pull/24/files#L1R128, too.

This comment has been minimized.

Copy link
@JonnyJD

JonnyJD Jun 30, 2013

Contributor

Yes, there is an incoming branch for that PR: JonnyJD#2.

This comment has been minimized.

Copy link
@JonnyJD

JonnyJD Aug 1, 2013

Contributor

Any update on this?
The PRs for the other platforms should be ready: #24 Linux/BSD/Solaris, #37 Mac OS X.

I would like to merge all platforms at the same time and the next release would be 0.6.0 in a couple of weeks (scheduled for end of august).

This comment has been minimized.

Copy link
@phw

phw Aug 5, 2013

Author Member

Sorry, didn't work on it yet :( But I will make this PR ready for merging

char *mb_disc_get_default_device_unportable(void) {
DWORD mask = GetLogicalDrives();
for (i = 0; i <= 25; i++) {

This comment has been minimized.

Copy link
@phw

phw Aug 8, 2013

Author Member

Don't know where the declaration of i got lost, but this definitely won't build. That's a bit embarrassing, time to fix this branch.

@JonnyJD

This comment has been minimized.

Copy link
Contributor

JonnyJD commented Aug 8, 2013

We could easily support optical disc drive numbering on Windows. I just found out \\.\CdRom0 is defined to be the first optical disc drive. (see msdn).

This won't help getting the corresponding drive letter and is just an additional option on how to support taking a number as a drive. When we do support that on Windows, we should also support that on Linux, of course.

Preferred return value for get_default_device is still a drive letter.

Signed-off-by: Philipp Wolfer <ph.wolfer@gmail.com>
@phw

This comment has been minimized.

Copy link
Member Author

phw commented Aug 8, 2013

I have updated the branch. I decided to go the same route as in the Linux code and make default_device thread local, there is a declspec for this.

The thing with drive numbering looks pretty nice. You can easily verify that it works by specifying CdRom0 as a parameter to discid.exe. It does not completely work as expecte, though. I tested on two laptops. One running Win7 with a built-in drive E:, and one running Win8 without any CD drive.

On the first laptop CdRom0 reads from drive E: as expected. But when I plugin an external USB drive F: and try to access it via CdRom1 it fails. Acces via the drive letter works as expected. On the laptop without built-in CD drive I can plugin in the external drive and access it via CdRom0.

I don't know if the behavior on the first laptop is a general thing that it won't enumerate multiple drives or if it is just because it is an external drive. In any case, I think this is too unreliable to support real drive numbering, but I wonder if we should set MD_DEFAULT_DEVICE to "CdRom0" as a fallback (instead of the current "D:").

@phw

This comment has been minimized.

Copy link
Member Author

phw commented Aug 8, 2013

This branch was tested in both VS 2012 and MinGW using GCC 4.7.

@JonnyJD

This comment has been minimized.

Copy link
Contributor

JonnyJD commented Aug 8, 2013

I wouldn't use "CdRom0" in user output. I would rather use "1" and match that to CdRom0 internally (testing if it is a number, checking with strtol(), see #37).

That CdRom1 doesn't work is weird. Not sure if implementing device numbers with GetLogicalDrives is better then. Just increment an index every time a CDROM drive is found and break if index == chosen drive. Using that you can also just return "1" as default.

I couldn't test your new changes with a drive yet, but it compiles fine.

EDIT:
I should note that I wouldn't use "0" as users don't really like to cope with that.

@phw

This comment has been minimized.

Copy link
Member Author

phw commented Aug 9, 2013

Would be easy to support the numbering with the code in this PR. But I would prefer having this and the default device patches for other platforms merged before adding the numbered device implementation. I think this PR and #24 are good to merge. I can''t test #37 but it looks quite solid on a first glance, too.

@JonnyJD

This comment has been minimized.

Copy link
Contributor

JonnyJD commented Aug 9, 2013

Well, at least for Mac, numbering disc drives is part of the current solution. It wouldn't be wrong to implement that in the same branch.
Either way this is a feature that should go into the same release as these PRs go into.

Or do you need commits from other branches? Should we open a new branch to combine these changes?


for (i = 0; i <= 25; i++) {
if (mask >> i & 1) {
snprintf(default_device, MAX_DEV_LEN, "%c:", i + 'A');

This comment has been minimized.

Copy link
@JonnyJD

JonnyJD Aug 9, 2013

Contributor

I would like a variable local to the function to be used for this and only strcpy to default_device when the final device is found.

This comment has been minimized.

Copy link
@JonnyJD

JonnyJD Aug 9, 2013

Contributor

Yes, for most systems this is thread local, meaning there is no other thread accessing default_device so for most cases this can't be problematic.

However, it is still not good "style" to write temporary values to a global.

…et_default_device_unportable.
@phw

This comment has been minimized.

Copy link
Member Author

phw commented Aug 11, 2013

I've updated the branch according to your last comment. Yes, I think it's time to start merging those branches into one and then open new pull request for additional features we want to get in. I think this is all stable enough to be merged together, and it makes it easier to start working on new stuff if there is a single development branch and we don't get stacked pull requests.

Whether you add this to a release branch for 0.6 or whatever version you plan to get this in or just a feature branch for all the default_device changes is up to you.

JonnyJD added a commit that referenced this pull request Aug 11, 2013
see pull request #20 (LIB-43)

I also removed some trailing spaces.
@JonnyJD JonnyJD merged commit 7ecabf3 into metabrainz:master Aug 11, 2013
1 check passed
1 check passed
default Merged build finished.
Details
@JonnyJD

This comment has been minimized.

Copy link
Contributor

JonnyJD commented Aug 11, 2013

This isn't what you said in your comment, but I read it as "Drive Numbering on Windows shouldn't be implemented in this branch/PR"?

I tested this again on a Windows machine and merged this to master.

Drive numbering is a different feature and now tracked in http://tickets.musicbrainz.org/browse/LIB-55 (for Windows).

@phw

This comment has been minimized.

Copy link
Member Author

phw commented Aug 11, 2013

Thanks. Yes, I think you got me right. I just wanted to keep a separate issue separate, so we can have this and still decide we don't want drive numbering or we want it in some other way or whatever:)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.