On libdvdcss

[Eugenio Jarosiewicz]

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


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!

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?


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.

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.


5) Why did the location of ioctl_ReadTitleKey() (and maybe
others) change? It makes the diff so much bigger!

...

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

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; */


Alas, I don't know about the whole *agid issue.  When I read it it
seemed right to me ;-)

Once again, we appreciate the work, I think we'd be happy to take it
once we sort these things out.

-ej


On Mon, 3 Dec 2001, [iso-8859-1] Håkan Hjort wrote:

> Hey,
> 
> This is the first part of patches to merge back things that we (Ogle)
> changed in libdvdcss 0.0.3.
> 
> This part only touches ioctl.[ch].
> 
> Implement ReportTitleKey for all supported targets.  There is a #error 
> marker in the WIN32 (NT/Win2000/Whistler) part though since I didn't 
> have much to go on there an can't test it.
> 
> A fist attempt to implement ioctl_ReportRPC to in the future allow
> us to query the drive for region settings.
> 
> InvalidateAGID doesn't take a AGID to be invalidated according to the
> standard (INF-8090 / SFF8090r99).
> 
> Other small changes include many fixes of wrong sizes to memcpy. 
> Several 8 that should have been 5 and a few 12 that should have been 10.
> Also clear all structures before using them.
> 
> 
> As for MacOSX / Darwin support there are several suspect parts in that 
> code. 
> dvdcpi.dataLength[0/1] never being initialized at three places and
> the title key request code doesn't put the sector / offset/ lba any
> where in struct. Since there can be several title keys on one Disc
> this is doesn't seem right.
> 
> If some one (Eugenio?) could double check this so that it's correct
> that would be great. 
> (I haven't changed this in any major way, so it's no worse that it was).
> 
> 

-- 
Eugenio Jarosiewicz	  ej0 at cise.ufl.edu	http://www.cise.ufl.edu/~ej0
			* Funny how that works *