Re: [Libcdio-devel] CD_MSF_FORMAT vs LBA on NetBSD

[Greg Troxel]

Edd Barrett <address@hidden> writes:

> Thanks for testing.

Glad to help - I don't expect to be really digging in, but try to pop in
and help portability here and there.

[deleting lots of this when there's no reason for me to add anything]

> On Thu, Oct 25, 2018 at 08:28:56PM -0400, Greg Troxel wrote:

>> 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?

So, I really did have your branch checked out, which I built and
installed to /usr/local, but I just realized that I had the 2.0.0
release in /usr/pkg, and thus I'm not really sure what I ran.  So I'm
redoing tests right now.  Followup message with new results shortly.

>> 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

Thanks for explaining on the list.  I should have been clearer - what I
meant is that I think the commit messages should be adjusted so that
someone reading the history, without the list history, can know the
rationale.  That's of course my opinion only, and I have no idea about
this project's norms, so if that's not how libcdio does things you
should ignore this comment.

>>   8207ace8914c418dea9d626d7839bec0f7202fab
>
> This is because, on OpenBSD it's possible to have multiple versions of
> autoconf and automake installed.

(I assume this is OpenBSD ports, and these aren't in the base system.)

FWIW, on pkgsrc there is autoconf 2.69 and automake 1.15.1.  (There is
also autoconf 2.13 for some old software that needs it.)

> The top-level wrapper scripts -- /usr/local/bin/auto{conf,make} read
> these environment variables to choose which one to run.

Do they really need this, vs choosing a modern version if one isn't set?
I would think this would lead to needing to adjust vast numbers of
upstream repositories.  Do the wrappers autoinstall the requested
verison?  Or is this going to need updating everytime the default
changes?

I have no idea if these variables being set would have an effect, so I
would wrap setting them in "if [ `uname` = OpenBSD ];" and stick in a
comment.  But that is highly likely not important.

>>   946ebc924ddeb7b26badfdcc7f29f0f5373ff027
>
> This prevents consumers of libcdio from being terminated upon an error.
> The correct thing to do is to continue execution returning an error.

Sounds good.  I didn't mean to say I thought the change was wrong, just
good to explain.  Totally valid point that libraries should not exit.

> (I actually got this fix from NetBSD pkgsrc)

When I updated the patches applied ok so I didn't read them :-)

>> 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.

I guess the tradeoff is it being simpler vs failing to find cd devices
that exist but have odd names.  Which sounds ok

> 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.

I'm really not trying to say the change isn't right.  I just meant that
I think the commit message should make the case, for future readers.

>> 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.

ok - that wasn't obvious to me given the commit messages and branch name.

>> 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.

Reading Rocky's message, I see the point of having essentially a virtual
function table and classes.  If that's easier for now, that makes
sense.  In the glorious future something that omits files from both
compilation and being in the vtable seems good, but indeed it takes
someone who thinks that's what they want to spend time on.