[linux-dvb] patch - descrambling on stream level

[Henrik Sjoberg]

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

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

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

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

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

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

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

Regards,
Henrik