Manu Abraham 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 .. ?
>
>> 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.,
Perhaps newly introduced constants should be #defined, and we can
go back and replace existing ones as part of s/w maintenance.
>
>> 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.
>
>
>
> ???.
Have a look at fmemopen().
You can have an output stream that points to a buffer, but isn't flushable.
>
>> 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.. ?
Yes, or fine a less convoluted way to build the output message...
>> 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
I skimmed through that, and the basic arguments are:
(1) it's not easy to grep -- my answer to that is get real tools,
like cscope, rather than twisting your programming habits to
conform to inadequate tools.
(2) the contents of a structure change more often that the type
of structure a variable refers to -- not always true. If you are
cutting & pasting similar routines that deal with structures that
are similar but vary only in a couple of additional elements,
then this isn't true... which actually happens a lot when you're
doing protocol programming in middleware (and which I spent
the last 5 years doing, so I saw it happen a lot...)
(3) that if the type of structure changes, then you're more likely
to leave members of the structure uninitialized -- this argument
was so twisted it left my head hurting... this is more of an argument
for using an abstract function for allocating and initializing structures
than having client functions call malloc() directly, which thereby
gives you a single place to add/remove new member handling in the
initialization.
Most of the discussion is about the evils of using temporary structures
when doing literal construction for an RHS, and the inherent evils of
the memcpy() that ensues...
Not really pertinent.
-Philip
>
> You can see a lot of comments on the same.
>
>
> Manu
>
