[linux-dvb] patch - descrambling on stream level

[Philip Prindeville]

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

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

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.

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.

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

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?

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.

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:

    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.

In general, I'd avoid unnecessary typecasts, because they hide the
instances when you're actually changing the pointer type in the noise.

-Philip