[Toybox] [PATCH] blkdiscard (for review)

[enh]

On Tue, Apr 14, 2020 at 10:54 PM Rob Landley <rob at landley.net> wrote:
>
>
>
> 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.

yeah, and it was the _lack_ of a change to point you to that was why i
couldn't send a clearer email in the first place :-)

> > 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.

ah, cool. i guess i never connected the dots. (i've moved desk and
building several times since then, so if i did have a post-it
reminding me to do anything, it's long gone!)

i'm assuming i'm better off without CFG_TOYBOX_DEBUG anyway, though,
so i'll leave it (and _FREE) off.

> Rob