[linux-dvb] patch - descrambling on stream level

Philip Prindeville philipp_subx at redfish-solutions.com
Thu Oct 20 06:00:50 CEST 2005


Henrik Sjoberg wrote:

>>Philip Prindeville wrote:
>>
>>    
>>
>>>Manu Abraham wrote:
>>>
>>>      
>>>
>>>>Henrik Sjoberg wrote:
>>>>
>>>>        
>>>>
>>>>>Hi,
>>>>>
>>>>>This is a bit embarrassing, but my latest patch contained pmt test
>>>>>code,
>>>>>which caused it not to work. I have prepared another one (third one's
>>>>>a
>>>>>charm?) which I have actually tested against cvs dvb-apps. It works
>>>>>ok for
>>>>>me.
>>>>>
>>>>>
>>>>>
>>>>>          
>>>>>
>>>>Any comments on this patch ? If none i can check it in.
>>>>
>>>>
>>>>Manu
>>>>        
>>>>
>>>
>>>Yeah, minor comments...  Should use space after keywords like "if"
>>>and "for".
>>>
>>>      
>>>
>>Yeah, these should be fixed .. Henrik .. ?
>>    
>>
>
>Sure, no problem. However, a few questions on some of the comments.
>
>  
>
>>>General comment about the code is that there are a lot of "magic
>>>numbers"
>>>that aren't evident to the casual reader:
>>>
>>>+       if(p_descriptor->descriptor_tag != 0x09) {
>>>+               printf("ERROR::Trying to copy a CA descriptor with
>>>incorrect tag [%d]. Bailing out.\n",
>>>+                      p_descriptor->descriptor_tag);
>>>+               return 16;
>>>+       }
>>>
>>>It's not clear what 0x09 signifies here...  This should be either a
>>>"const unsigned"
>>>or else a #define...  Ditto for the 16...
>>>
>>>      
>>>
>>These descriptors are defined in ISO13818 and other relevant documents..
>>Maybe we need to #define the descriptors...., but the descriptors are
>>quite a lot to be defined thus.,
>>    
>>
>
>As Manu sais, there are lots of descriptors to be defined, especially in
>descriptor.c. I _could_ change them all, however, I _could_ use the time
>to improve other areas instead...
>  
>

See my last message.  I suggested that new constants be #defined, and that
old constants could be replaced by #define's when reworking old code...


>>>There are no other printf()'s in this library...  And errors should
>>>usually
>>>go to STDERR anyway...  But in general, a library should return error
>>>status but not print messages.  Let the application decide if it wants
>>>to
>>>generate a message or not.
>>>
>>>      
>>>
>>Hmm... i think that was for debugging, Anyway you are right. But the
>>problem here is if we don't see the error's/messages it would be quite
>>difficult to debug the lib ..
>>    
>>
>
>As this is how the error messages of most of dvb-apps are used, I think we
>should agree on a good solution on this and do a larger improvement of it
>in all libs in dvb-apps.
>  
>

Sure.  Consistency has the lowest surprise-factor...


>>>More comments...
>>>
>>>+static void try_move_ca_descriptors(struct service_info *p_si)
>>>+{
>>>+       int i;
>>>+       int j;
>>>+       int k;
>>>+       int l;
>>>+       struct streams *p_stream1;
>>>+       struct streams *p_stream2;
>>>+       struct descriptor *p_desc1;
>>>+       struct descriptor *p_desc2;
>>>+       int movable = 1;
>>>+       int ca_descriptors = 0;
>>>+       int num_ca1;
>>>+       int num_ca2;
>>>+       int found_match;
>>>+       int found;
>>>
>>>Use commas and group variables of identical types.
>>>
>>>      
>>>
>>Yeah ..
>>    
>>
>
>I'm on it.
>
>  
>
>>>Do we need:
>>>
>>>+       //int j, k, l, i2, j2;
>>>
>>>In:
>>>
>>>       printf("%s: Setting PMT Command\n", __FUNCTION__);
>>>-       for (i = 0; i < p_en50221_pmt_object->program_desc_count; i++) {
>>>-               if
>>>(p_en50221_pmt_object->p_en50221_prog_desc[i].descriptor_tag
>>>== 0x09) {
>>>-                       printf("%s: CA descriptor found @ PROGRAM
>>>Level, Setting CA PMT command=[%02x]\n", __FUNCTION__, pmt_command);
>>>+       if (p_en50221_pmt_object->program_desc_count > 0) {
>>>+               printf("%s: CA descriptor(s) found @ PROGRAM Level,
>>>Setting CA PMT command=[%02x]\n",
>>>+                      __FUNCTION__, pmt_command);
>>>                       p_en50221_pmt_object->ca_pmt_cmd_id =
>>>pmt_command;
>>>                       object_length += 8;
>>>               }
>>>-       }
>>>
>>>
>>>If you're changing the level of nesting, then the indent should change
>>>as well...
>>>
>>>      
>>>
>>True...
>>    
>>
>
>Dito.
>
>  
>
>>>In:
>>>
>>>+       sprintf(message, "%s: %s={", __FUNCTION__, type);
>>>+
>>>+       sprintf(temp, "Length=%d", ptr[pos++]);
>>>+       strcat(message, temp);
>>>+       list_management = ptr[pos++];
>>>+       sprintf(temp, ":CA_PMT_ListManagement=%d", list_management);
>>>+       strcat(message, temp);
>>>+       sprintf(temp, ":ProgramNumber=0x%02x%02x=%d", ptr[pos + 0],
>>>+               ptr[pos + 1], (ptr[pos + 0] << 8) + ptr[pos + 1]);
>>>+       strcat(message, temp);
>>>+       pos += 2;
>>>+       sprintf(temp, ":VersionNumber=%d", (ptr[pos++] >> 1) & 0x1f);
>>>+       strcat(message, temp);
>>>+       program_info_length = ((ptr[pos + 0] << 8) & 0x0f) + ptr[pos +
>>>1];
>>>+       pos += 2;
>>>+       sprintf(temp, ":Program={");
>>>+       strcat(message, temp);
>>>
>>>
>>>I would set up a FILE * to point to the buffer, and therefore guard
>>>against overflowing the size of the buffer.
>>>      
>>>
>> ???.
>>
>>    
>>
>>>Looking a little further,
>>>though, we see:
>>>
>>>+       sprintf(temp, "}\n");
>>>+       strcat(message, temp);
>>>+
>>>+       printf(message);
>>>+
>>>+       return 0;
>>>
>>>So I'm stumped...  Why all of this trouble to build a buffer and then
>>>printf(), when it could be printf'd as you go?
>>>
>>>      
>>>
>>That message is then later on copied out, to create another message.
>>The printf's are temporarily for debugging at the moment.
>>    
>>
>
>The reason is actually that I call parse_ca_descriptor while I construct
>the string. That function in turn does printouts, which would destroy my
>message if I would have printf:d it.
>  
>

I'll go back and stare at that some more...

Hmmm...  It seems confusing that the function parse_ca_descriptor()
is both doing work (parsing data) and generating output... unless that
output is purely for debugging...  in which case, why is it going to
stdout???

I would have debug_parse_message () do work first, then generate
messages afterwards...  or else format them into a buffer using
fmemopen()...


>>>Also, printf(message) is dangerous...  What if "message" contains
>>>"%s"?  Should be using fputs(message, STDOUT) instead...  And
>>>that would avoid using:
>>>
>>>+       char message[2048];
>>>
>>>which is a lot of stack space.
>>>      
>>>
>>You mean allocate in the heap.. ?
>>
>>    
>>
>>>Not sure why:
>>>
>>>uint16_t debug_parse_message(struct ca_msg *p_ca_msg, uint16_t length)
>>>
>>>needs a return value, if it's an invariant "0".
>>>      
>>>
>
>It doesn't, I will remove it.
>
>  
>
>>>Regarding changes like:
>>>
>>>+++ lib/libdvbsi/channels.c     18 Oct 2005 16:44:42 -0000
>>>@@ -102,7 +102,6 @@
>>>static int parse_param(char *val, const param *p_list, int list_size)
>>>{
>>>       int i;
>>>-
>>>       for (i = 0; i < list_size; i++) {
>>>               if (strcasecmp(p_list[i].name, val) == 0)
>>>                       return p_list[i].value;
>>>
>>>Please avoid whitespace only changes.  They make merging multiple
>>>branches hellish.
>>>      
>>>
>
>Strange, I though diff -ub would ignore that. I'll keep it in mind though.
>  
>

It will ignore it.  However, "patch" won't...  Patch doesn't understand -b
(the way that diff does, anyway).

>>>Also, many people like a blank line between declarations and statements.
>>>
>>>Looking at:
>>>
>>>+       if (p_descriptor->extended_event.length_of_items)
>>>+               free(p_descriptor->extended_event.p_items);
>>>+       if (p_descriptor->extended_event.text_length)
>>>+               free(p_descriptor->extended_event.p_text_char);
>>>
>>>
>>>Don't bother.  free(NULL) is perfectly fine.  I would, on the
>>>other hand, always NULL out a pointer that I've freed, unless
>>>it is about to go out of scope.  I.e.:
>>>
>>>       free(p_description->extended_event.p_text_char);
>>>       p_description->extended_event.p_text_char = NULL;
>>>
>>>and thus avoid memory leaks or double-frees.  Same elsewhere.
>>>      
>>>
>
>This is mainly in descriptor.c, which is not a rewrite, but a
>restructuring.  However, I could put in some work here too.
>The entire memory handling would actually benefit from a review.
>  
>

Sign me up...  I might even have some patches to send in...


>>>In:
>>>
>>>+       p_descriptor->short_event.iso_639_language_code =
>>>+               (((buf[pos] << 8) | buf[pos + 1]) << 8) | buf[pos + 2];
>>>
>>>
>>>Not sure I get this...  Shouldn't "pos" be shifted more bits?
>>>Otherwise,
>>>"pos" and "pos + 1" will be combined...  Oh, got it.  Didn't match
>>>parens.
>>>Well, I'd still write:
>>>
>>>    (buf[pos] << 16) | (buf[pos + 1] << 8) | buf[pos + 2];
>>>
>>>instead...
>>>      
>>>
>
>I would too ;) Also in descriptor.c. I wanted to change as few things as
>possible when I had no chance of testing it.
>  
>

Sound strategy.  Patches should either be (a) new functionality, (b) bug 
fixes,
or (c) code clean-up and restructuring...  but only one of the above at 
a time.
Otherwise, you risk introducing new bugs that are harder to track down.


>  
>
>>>Regarding:
>>>
>>>+       struct descriptor *p_en50221_streams_desc =
>>>+               (struct descriptor *) malloc(sizeof (struct
>>>descriptor) * p_streams->streams_desc_count);
>>>
>>>malloc() returns a "void *", which is an untyped pointer.  It doesn't
>>>need
>>>to be cast.  Also:
>>>
>>>      
>>>
>>Right.
>>
>>    
>>
>>>   p = malloc(sizeof(*p));
>>>
>>>is a lot more clear than:
>>>
>>>   p = malloc(sizeof(struct descriptor))
>>>
>>>since you don't have to flip back and confirm that "p" is of type
>>>"struct descriptor".  Same applies whether you are allocating a
>>>single element or an array of them.
>>>      
>>>
>>
>>This is not quite right. On LKML, this has been proven not advisable.
>>You can see a lengthy discussion of this on LKML with a thread like this
>>
>>http://marc.theaimsgroup.com/?t=112703827100001&r=1&w=2
>>
>>You can see a lot of comments on the same.
>>
>>
>>Manu
>>
>>    
>>
>
>I'll give the code a few hours this weekend and try to do an overall
>improvement regarding things mentioned above.
>
>Thanks for the comments Philip.
>  
>

No problem.  I'll study the code a bit, and if I can get a little more
competent on it, I'll look at suggesting some fixes.  I wish that there
were a dedicated forum to discussing code development (as opposed
to using it as well)... might be easier to have focused discussions.

-Philip

>Regards,
>Henrik
>
>
>  
>




More information about the linux-dvb mailing list