[PATCH] D55014: Add a new interceptor for getvfsstat(2) from NetBSD

[Kamil Rytarowski via Phabricator via llvm-commits]

krytarowski marked an inline comment as done.
krytarowski added inline comments.


================
Comment at: test/sanitizer_common/TestCases/NetBSD/getvfsstat.cc:18
+  if ((rv = getvfsstat(NULL, 0, ST_WAIT)) == -1)
+    err(EXIT_FAILURE, "getvfsstat");
+
----------------
krytarowski wrote:
> krytarowski wrote:
> > vitalybuka wrote:
> > > krytarowski wrote:
> > > > vitalybuka wrote:
> > > > > you can simplify the test with simple asserts here
> > > > Do you mean: `assert(rv != -1)` ? err(3) is already simple enough so I will keep it.
> > > I see couple other tiny reasons, which are not strong enough to block the patch, but maybe you'll still consider them.
> > > 
> > > The only important lines here are 17 and 25. The rest is just necessary distruction from the test,  so smaller is better.
> > > 
> > > Also I'd prefer assert report with line number, which can be used for quick jump to location than current printout of "getvfsstat".
> > > 
> > > "if (!buf)" is totally unneeded as it crash anyway, and we don't to that in other tests.
> > > 
> > > If you don't CHECK them maybe: printf("Filesystem %d %s %s %s\n", i, buf[i].f_fstypename,   buf[i].f_mntonname, buf[i].f_mntfromname);
> > > 
> > > and consistency: compiler_rt tests never use err() in cases like this, mostly asserts
> > > 
> > `err(3`) is a BSD style. I will switch it to assert and land soon.
> I still have 2-3 APIs to submit to review and I will start addressing reported comments from review.
I've replaced err(3) with assert(3). I will keep the current style of printf(3) and I will assert `buf` to be != `NULL`. It will certainly crash in the case of error.. but I feel that it's a little bit better programming style to check allocation for success.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55014/new/

https://reviews.llvm.org/D55014