[linux-dvb] patch - descrambling on stream level

Manu Abraham abraham.manu at gmail.com
Wed Oct 19 23:45:23 CEST 2005


Henrik Sjoberg wrote:

>>>
>>>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...
>
>  
>
Since this one is not a functional change, but a time consuming cosmetic 
change, i think this can probably wait.

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

A macro for prints would be easier, which can be enabled/disabled, maybe 
an optional parameter also would be better to debug also, in case to 
identify what needs to be verbosed.

This macro can be then used to replace the printf's.

Something similar ..

#define print(format, arg...) do {
    if (debug)
        fprintf(stderr, "format", ##arg)
} while(0)

>>>
>>>
>>>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.
>  
>
I would suggest to have a functional change applied, then cleanups/other 
optimizations, rather than one single patch checked in. Would be easier 
for everybody.

>I would too ;) Also in descriptor.c. I wanted to change as few things as
>possible when I had no chance of testing it.
>
>  
>
What i would say is, cosmetic changes should be a different patch rather 
than a functional patch.
But sometimes that cannot be avoided, but generally we should go that 
way i think.

Manu




More information about the linux-dvb mailing list