Hi Greg,
Thanks for testing.
On Thu, Oct 25, 2018 at 08:28:56PM -0400, Greg Troxel wrote:
> ok, but when this is merged a comment is in order, so that people
> starting to read the code will get that.
Agreed.
> I built the branch and am currently using cd-paranoia with it to rip a
> disc, and I'm getting files with the same hash as before.
Great news!
> cdda-player doesn't work, but it doesn't on master either, and it seems
> roughly the same. It gets errors, lists the tracks, and indicates that
> it's playing but keyboard input does nothing, and ^C exits. (I
> understand that I almost certainly have no audio path, but I expect the
> curses ui to sort of work.)
This was the behaviour I experienced on master, but the fixes on my
branch made it work again. Are you sure you were on my branch?
> Reading the diff, there are some changes whose comments don't explain
> the change and why it's necessary (vs sort of restating what I absorb
> from the diff). Specifically these
>
> 8207ace8914c418dea9d626d7839bec0f7202fab
This is because, on OpenBSD it's possible to have multiple versions of
autoconf and automake installed. The top-level wrapper scripts --
/usr/local/bin/auto{conf,make} read these environment variables to
choose which one to run.
> 946ebc924ddeb7b26badfdcc7f29f0f5373ff027
This prevents consumers of libcdio from being terminated upon an error.
The correct thing to do is to continue execution returning an error.
(I actually got this fix from NetBSD pkgsrc)
> a7fc90e1ce7a70a9efde4d1ca4711b20c5b2c8e5
Without this the drive would continue to play, instead of stopping.
> And then there's the dropping of using
> sysctl in favor of probing by name - and it's not clear why that should
> be changed on NetBSD even if the sysctl approach doesn't/can't work on
> OpenBSD. (It's also not clear why it shouldn't change, but for a big
> change like that there should be rationale in the commit message.)
When I first picked up libcdio, drive detection was broken on OpenBSD. I
had this custom-written backend to use as a reference:
https://cvsweb.openbsd.org/cgi-bin/cvsweb/ports/audio/libcdio/files/openbsd.c?rev=1.4&content-type=text/x-cvsweb-markup
This is where we got the "open in a loop" idea. I think the sysctl
approach could be made to work on OpenBSD, but the other method seems
much simpler.
Also, look on the libcdio lists a month or two back. We discussed this
change, and came up with the RAW_PART trick in doing so.
> Some of the changes look unrelated to OpenBSD (unbreak cdda-player?,
> Ensure that stdin..., missing set of b_cd, add missing -d)
That's true. Some of the changes just fix stuff that's broken for all
platforms.
> Also, I don't get this:
>
> ef44a97af6430107f09e27a31576c4adfa36ac09
> It does not make sense to me to be compiling the netbsd.c file on
> operating systems where that isn't the plan, or if it is compiled
> because ifdefing the whole file is easier than not including it via
> automake, then a giant ifdef around the file.
Rocky has already responded to this change. It surprised me too, but for
now I'm trying to do the bare minimum to get the driver working on
OpenBSD.
I'll leave that discussion up to the libcdio devs.
> Hope this helps and if you need me to test more or review code changes
> please feel free to ask.
It does. Thanks again for testing.
--
Best Regards
Edd Barrett
http://www.theunixzoo.co.uk
