[LiSA-Devel] Comments on 4069be7

[Radu Rendec]

Hi everyone,

First of all, thanks Andreea for the patch that you committed in the
userspace tree! I have a few comments though, and I'm posting them to
the list because I think they may be useful for the other developers as
well.

To make it clear from the beginning, I haven't compiled/tested your
patch - it's just what I noticed while looking at the patch.

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.

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.

3. Using short_if_name() directly with strcpy() leaks the memory
allocated by asprintf() inside short_if_name().

4. Using strcpy() (with short_if_name()) is dangerous. I know that right
now the maximum length that can be returned by short_if_name() is
MAXSIZ_IFNAMSH (which is used for allocation as well). But consider that
somebody may change the short_if_name() function in the future and add a
new path that returns longer names. The person who makes the change will
probably be unaware of your use case in exec.c and cause your code to
write outside the allocated space.

5. The non-ethernet path inside short_if_name() must use "net %s"
instead of "net%s". Without the space separator it would generate a name
that cannot be parsed back by the CLI parser - assuming that it may be
used in the future for other purposes than displaying the vlan-assigned
interfaces, such as generating the configuration.

I have an idea that solves some of the problems described above and also
makes the code a little bit more efficient. Instead of allocating new
storage space for all the interface names, you can only store an array
of pointers (as they are returned by the short_if_names()) and run
qsort() on this array.

The advantages of this approach:
      * you don't allocate (a large) space on the stack for the "names"
        variable
      * you don't need to copy the interface names from the memory space
        that is allocated by short_if_name()
      * when qsort() swaps elements, it only needs to swap the amount of
        memory required by a pointer instead of MAXSIZ_IFNAMSH

I'm looking forward to hearing your feedback on all these topics.

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.

Keep up the great work!

Radu