Work at SourceForge, help us to make it a better place! We have an immediate need for a Support Technician in our San Francisco or Denver office.

Close

#61 Corrected funcionality to bu_basename

closed-accepted
Sean Morrison
6
2011-06-06
2011-03-27
No

Function bu_basename() says it'll return things that it doesn't. For instance, the input "////" should return "/" and "a/" should return "a", but in both cases the current code is returning an empty string.

I edited the code to solve these problems. I had to include string.h lib and also allocate a new string since the input string may need to be modified.

I've attached the diffs in files include/bu.h and src/libbu/basename.c

Discussion

1 2 > >> (Page 1 of 2)
  •  
    Attachments
  • Sean Morrison
    Sean Morrison
    2011-03-27

    • assigned_to: nobody --> brlcad
    • status: open --> pending
     
  • Sean Morrison
    Sean Morrison
    2011-03-27

    Guilherme, at a glance, the patch needs a little more work as it doesn't conform to at least one of our HACKING guidelines. It's a minor point, but you should be familiar with the guidelines since it's the first step towards attaining commit privs. The main issue is calling malloc(), so you can start there but I would suggest getting familiarized with everything in the guidelines.

    If you want to take this patch to the next level, a regression test file would be useful -- one that calls bu_basename() and basename() for a variety of inputs testing common use and edge cases, to make sure they both return expected results. Not necessary, but definitely something that would be useful.

     
  • I've updated the patch. I read the guidelines and I changed the code so that it conforms to it. This includes calling bu_strlcpy and bu_malloc instead of the standard ones. Also I corrected spacing around operators.

    Following the recomendation, I added a uni-test to this function, namely basenametester.c at /src/libbu (I also included it the Makefile.am). It performs tests against unix basename (I assumed that noinst binaries shouldn't be cross plataform).

     
    • status: pending --> open
     
  • updated patch

     
    Attachments
  • Sean Morrison
    Sean Morrison
    2011-03-29

    • status: open --> pending
     
  • Sean Morrison
    Sean Morrison
    2011-03-29

    A quick review and the update looks much better. Curious that you used the basename binary with popen() instead of the basename(3) function in basenametester. Any particular reason?

    That said, there is still one remaining issue. Now that bu_basename() is allocating memory on the heap, the return value should no longer be const and callers need to make sure that memory is released. Otherwise, applying the patch would inject memory leaks everywhere it's called.

     
  • updated patch - version 3

     
    Attachments
    • status: pending --> open
     
1 2 > >> (Page 1 of 2)