On libdvdcss

[Håkan Hjort]

Mon Dec 03 2001, Eugenio Jarosiewicz wrote:
> 
> I have a few err, comments about the patch ;-)
> 
> 1)
> 
> -    /* dvdcpi.dataLength[0] = 0x00; */ /* dataLength[0] is already  memset to 0 */
> +    /* dvdcpi.dataLength[0] = 0x00; */
> 
> Please don't remove comments that aren't in your code. It wasn't that
> ugly was it?  I left it there so I'd remember why I commented that
> code out. As Douglas Coupland wrote "People are amnesia machines" (I
> think it was him; case in point!-). 
> 
It was ugly as it was more than 80 chars wide, but in fact I meant for
it to be "dvdcpi.dataLength[0] = 0x00;" (i.e. not commented out).
This really is no place for such micro optimizations.

> 2)
> 
> +#error "Are you sure? shouldn't dataLength be written at all?"
> 
> Um, no way would I take a diff that had this.  If you were trying to
> prevent data corruption or a crash in a kernel maybe, but this dosn't
> apply here.  I wholly appreciate your effort, but I'd appreciate a fix
> more instead of a break in the compile!
> 
This isn't critical code I give you that.  Doing it like this will
make the next MacOSX guy that tries to compile it look at it though.
It's not a release of the code that looks like this, it's not even in
cvs (and I doubt it ever will be).

> In this specific situation, a) to answer your question: no I am not
> sure (I don't particularly enjoy reading specs, so I might have
> misunderstood something), and also b) I also didn't understand your
> grammar.  ( English grammar is a PITA.  Mental note to self, learn
> Esperanto. ;-) 
> 
> Did you mean that 
> 
>      /* dvdcpi.dataLength[1] = 0x06; */
> 
> should or should not happen?
> 
Sorry about that shouldn't write patches late at night..  I meant, are
you sure that no value (other than zero) should be written to
dataLength[0] and [1]?
In all other cases you write the same lengths as the other user space
SCSI implementations.

> 3)
> 
> -    memcpy( p_key, dvddki.discKeyStructures, sizeof(dvddki.discKeyStructures) );
> +    memcpy( p_key, dvddki.discKeyStructures, 2048 );
> 
> Ugh, no no fork you kindly no.
> 
> If you check on a OSX, sizeof(dvddki.discKeyStructures) is in fact
> the correct size of 2048 .  One of the first things you learn in
> programming is to not litter code with meaningless constants and at a
> minimum use #defines.  It's a shame that the Linux & BSD parts of
> libdvdcss ioctl code have these magic constants all around.
> 
But so what? It might not be... p_key is just exactly 2048 bytes no
matter what size dvddki.discKeyStructures is.
Having different sizeof(..) for all the targets and in some cases
adding dummy structure typedefs just to be able to do sizeof() is not
nice, IMHO.  I can add three defines (of 2048, 10 and 5) for these if
you wish.

> 4)
> 
> _
> +
> 
> I didn't change the indentation in the wintel/bsd/linux/solaris
> sections so people can use their editor, so please don't change it in
> the OSX Darwin parts;-)  If it is giving other people troubles then
> I'll gladly accept the change.  In general, I try to follow the
> indentation convention of the project/file I'm working on.  I didn't
> actually check back against the original to see if it was somewhere I
> had deviated from the convention, if I did then thanks for the typo
> fix and I'll STHU.
> 
Why is this an issue? so I forgot to use -w to diff.  I try following
whatever style things are written in too.  I don't see how this could
be something keeping you from using a particular editor.  I had to do
this merge with some hand applying of a three way diff.

> 5) Why did the location of ioctl_ReadTitleKey() (and maybe
> others) change? It makes the diff so much bigger!
> 
That was the only one that moved.  It moved because that was where I
had written it in my original patch.  The only implemented targets
that existed where for linux and MacOSX, while I had code for *bsd,
beos, solaris, 2*windows.  No all that many extra lines.
I'll redo the patch if people find it to hard to review like this.

> I didn't check the whole patch, only some relevant parts of it.
> 
> Parts of the patch did look good, like initiliazing structs to zero
> and changes for the right sizes in other places that needed it (with
> "magic" constants, unfortunately ; note that Darwin and in some other 
> places the sizeof()'s were fine).
> 
See my points above.

> About the only thing that I'd consider taking right (for the Darwin 
> parts, I do not speak for all of vlc) is if someone explains whether
> or not this should happen (or if the code around it needs to change).
> 
> +    /* dvdcpi.dataLength[0] = 0x00; */
> 
I think that there should be an explicit set to 0.  That or just
remove the line all together.  The thing that I was more interested
in where the fact that the set of dataLength[1] was commented out 
for some (3) cases.

If you could take a look at the things in the comment I added for
the Read/ReportTitleKey code for MacOSX please.  I tried to point
out in my earlier email that some thing looks fishy with that.  It 
doesn't use the i_pos argument, though the title key depend on it.

> Alas, I don't know about the whole *agid issue.  When I read it it
> seemed right to me ;-)
> 
When you read the original code, or when you read the new code 
or when you read the standard?
 
-- 
Håkan Hjort
d95hjort at dtek.chalmers.se