[Toybox] [PATCH] blkdiscard (for review)

[Rob Landley]

On 4/14/20 7:39 PM, enh wrote:
> On Mon, Apr 13, 2020 at 8:45 PM Rob Landley <rob at landley.net> wrote:
>>
>> On 4/13/20 8:02 PM, enh wrote:
>>> On Fri, Apr 10, 2020 at 12:33 AM Rob Landley <rob at landley.net> wrote:
>>>>
>>>> On 4/9/20 12:54 AM, Patrick Oppenlander wrote:
>>>>> Hi Rob,
>>>>>
>>>>> I needed blkdiscard for an embedded job so had a go at putting it together.
>>>>
>>>> Applied, let's see...
>>>>
>>>>> Length & offset parsing don't match util-linux where MB, GB are powers
>>>>> of 1000 and M, MiB, G, GiB are powers of 1024.
>>>>>
>>>>> I didn't use xioctl to avoid leaking fd if CFG_TOYBOX_FREE is set. Is
>>>>> there a better solution to this?
>>>>
>>>> CFG_TOYBOX_FREE is... squishy.
>>>>
>>>> In theory there's no point freeing memory and closing files right before a
>>>> program exits, because the process is about to go away and the OS will clean all
>>>> that up. But the android guys like running stuff under valgrand and such to find
>>>> resource leaks (even when they don't necessarily matter).
>>>
>>> TL;DR: always use xioctl() :-)
>>
>> Yeah, agreed here. :)
>>
>> CFG_TOYBOX_FREE is a bit like having config options within a command in that my
>> position on doing that has shifted over the years. I've always prioritized
>> "simple" over "small" in toybox, but I've changed my mind on what "simple" means.
>>
>> "A codepath with less in it" is one type of simple, but "one consistent codepath
>> that always behaves the same way" is another type of simple, and these days I
>> lean towards the second a bit more strongly. I want to know how it behaves. I
>> want that question to have a consistent answer.
>>
>>> i think you're confusing a few different things here. we do have fd
>>> checks, yes. we're also moving as fast as we can to a world where the
>>> hardware faults on memory errors
>>> (https://android-developers.googleblog.com/2020/02/detecting-memory-corruption-bugs-with-hwasan.html
>>> being our latest missive on the subject).
>>>
>>> historically we had to use CFG_TOYBOX_DEBUG when were were having asan
>>> issues with ls, but CFG_TOYBOX_DEBUG broke mount. so i committed a
>>> local patch (afair our only patch that isn't upstream):
>>>
>>>   memset(totals, 0, sizeof(totals));
>>>   if (CFG_TOYBOX_ON_ANDROID || CFG_TOYBOX_DEBUG) memset(len, 0, sizeof(len));
>>
>> That's not in mount, that's in ls?
> 
> yes. like i said, CFG_TOYBOX_DEBUG was our *workaround for ls* but it
> *broke mount* instead.

Sorry, I'd compared the git logs of AOSP and local for toys/*/mount.c and they
were identical.

> the thread about this was entitled "DEBUG +
> NORECURSE "mount" crash when not root" but i don't think it ever got
> fixed.

Hmmm...

http://lists.landley.net/pipermail/toybox-landley.net/2016-January/007918.html

> if i run "mount" as non-root, i get a crash here:
> 
>     } else if (CFG_TOYBOX_DEBUG && uid && which != toy_list)
>       error_msg("Not installed suid root");

I fixed that one two weeks later, see commit ca311f1a41a5.

Rob