[LiSA-Devel] Comments on 4069be7

[Radu Rendec]

On Wed, 2013-03-06 at 09:54 +0200, Andreea-Cristina Hodea wrote:
> On Tue, Mar 5, 2013 at 9:51 PM, Radu Rendec <radu.rendec at mindbit.ro> wrote:
> > 1. Why did you wrap strcmp() in your custom cmpstr() function? I'm
> > guessing that it's related to argument type casting and const
> > qualifiers, but maybe we can find a more elegant solution.
> 
> You got it right, it's about the format that qsort requires for the
> compare function. I've made a little more research and it's a common
> practice when it comes for sorting array of strings in C. For now I
> did not find another way to do it if I want to sort with qsort, but
> I'll keep thinking on this.

Since you're not doing anything to the arguments (except for casting)
and to the return value, I guess you can do something like this:

typedef int(*qsort_compare)(const void *, const void *);

And then cast the strcmp symbol to this type before passing it to
qsort() - something like this:

qsort( ... , (qsort_compare)strcmp)

I haven't tested this, though :)

> > 2. Allocating names[vlif_no][MAXSIZ_IFNAMSH] is a little bit awkward.
> > The main issue that I see is that vlif_no is not constant. I'm not sure
> > if this is actually allowed by the C99 standard (it probably is, since
> > you're not getting a compiler error), but I would avoid it.
> 
> Declaring an array of non constant size comes as a feature from C99
> which is called VLA(variable length arrays). I would keep it because
> it makes the code clearer. Though, if you are aware of problems with
> this approach, please reply and I'll change it to malloc - check
> malloc succeeded - free.

It's probably because I've been doing a lot of embedded C during the
past 2 years, so I freak out when it comes to allocating large amounts
of memory on the stack.

This is actually my only concern, and your approach is probably valid on
linux. I would check, though, how much stack space we have on other
architectures, such as ARM and MIPS - since we plan on integrating LiSA
into low-end devices such as SOHO routers.

> > Last but not least, please don't take all these comments as criticism
> > and, more important, don't get discouraged. I only mean to point out
> > what can be done better and *guide* you through the process.
> 
> I don't and thank you for taking the time to explain all the issues.
> I've "upgraded" the patch and as soon as I receive a reply to point 2,
> I'll push it.

Ok, good to know. Then we are on the same page :)

One more thing: when replying to list messages, please use "reply all"
so the reply be posted back to the list - unless of course you really
mean the reply to be private for the original sender.

Radu