[linux-dvb] patch - descrambling on stream level

[Manu Abraham]

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

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

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

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

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

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

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