clean up optionsNameThumbnail()

[guijan]

Sorry but this function has been bugging me ever since #220. I knew it could be better but I failed back then.

I've tested it with these commands:

src/scrot --thumb 50 test.png
src/scrot --thumb 50 test
src/scrot --thumb 50 te.st.png

As you can see, all the right files were created:

$ git status
On branch briefer-namethumb
Your branch is up to date with 'origin/briefer-namethumb'.

Untracked files:
  (use "git add <file>..." to include in what will be committed)
        te.st-thumb.png
        te.st.png
        test
        test-thumb
        test-thumb.png
        test.png

nothing added to commit but untracked files present (use "git add" to track)

[guijan]

Force-push because I had an extra ; of all things.

[N-R-K]

I don't really like the existing function either, but the improvement here is very marginal. It's still directly messing with pointer arithmetic which are error prone.

I honestly had no clue that a screenshot tool would have so much string manipulation going on! If I were to write scrot from scratch then I definitely would've opted for using sized-strings, with "higher level" string manipulation routines.

I also wouldn't have created a massive web of allocated pointers, each of which needs individual managing, which is error prone (evident by the amount of leaks we've discovered lately). Instead the strings would go into an arena so that they can be managed in groups.

But in any-case, we're not writing scrot from scratch so we're far past that point :)

</rant>

---

But if this still bothers you, then you can use the stream functions from #250, they're guarded by assertions so mistakes are less likely to go unnoticed. Something like this (pseudo-code):

Stream ret = {0};
streamReserve(newNameLen);

streamMem(name, ..);
streamMem(thumbSuffix, sizeof(thumbSuffix) - 1);
if (ext) streamMem(ext, extLen);
streamChar('\0');
return ret.buf;

Also, I'd probably use memrchr for the extension, it's portable enough and is being considered for next POSIX.

[guijan]

This version has a slightly better algorithm (less strlcat()'ing), generates smaller assembly on x64 with Clang, and reads like a clearer C version of an English explanation of what the function is supposed to do:

• Copy the string up to the extension (if any) exclusive
• Append "-thumb"
• Append the extension if we had one

It does, however, do a lot of working around the poor design of C's string.h. If you want, I can change it to this version which doesn't:

char *optionsNameThumbnail(const char *name)
{
    char *ext = strrchr(name, '.');
    int basenameLen = ext ? ext-name : INT_MAX;
    char *ret;
    if (asprintf(&ret, "%.*s-thumb%s", basenameLen, name, ext ? ext : "") == -1)
        err(1, "asprintf");
    return ret;
}

Although I'm not sure how widely adopted asprintf() is. The "Stream" functions might be a good idea too, we can wait for #250.

I considered memrchr(), but OS X doesn't have it, that would introduce a dependency on my libobsd library in the OS X version which @daltomi was against in the past.

Memory management in scrot isn't complicated, there's just a lot of bad code that needs fixing, but all we need to do is look for the last use of a variable and free() it. We don't have a complicated task we don't delegate to libraries to do, just unnecessarily complicated code. Hand-holding wouldn't make malloc() and free() simpler.

[N-R-K]

I considered memrchr(), but OS X doesn't have it,

Unfortunate, most other BSDs have it. Drop that idea then.

[N-R-K]

One more thing I realized (just mentioning it so I don't forget about it later, doesn't need to be fixed in this PR) is that this function probably won't work reliably for cases like ./- (#241) or basically any other pathname that has . or .. it in.

[guijan]

That's similar to #228.

I'd like to make the code chdir() to the output directory (acquired using dirname()) eventually which would also fix #226. Then it could error if the basename() of the output file is ".".

This would open the path for some mitigations that restrict the filesystem view like chroot, unveil, and landlock, but I'm not sure of how feasible those are because the output directory could contain imPrintf()'s special strings.

[guijan]

Should be ready for review.

[N-R-K]

Didn't test (yet) but looks OK.

[N-R-K]

Thanks.