[michael-o]

ENODATA is an XSI/SVID extension, not part of the POSIX base specification, so <errno.h> is not required to define it. FreeBSD and OpenBSD do not define it, which currently breaks the build of nanoarrow_ipc on those platforms. (NetBSD already defines ENODATA itself and is unaffected.)

The existing fallback only kicked in for old Windows/MinGW toolchains (R 3.6). Widen the guard to apply on any platform that does not define ENODATA, so FreeBSD, OpenBSD, and other such systems also get the fallback definition.

The value 120 matches the existing Windows/MinGW fallback and does not collide with any errno value defined by FreeBSD (ELAST == 97 as of FreeBSD 16-CURRENT) or OpenBSD (ELAST == 95).

[michael-o]

All tests pass locally:

100% tests passed, 0 tests failed out of 269

Total Test time (real) =   1.80 sec

The following tests did not run:
         51 - SharedBufferTest.SharedBufferThreadSafeClone (Skipped)
         57 - SharedArrayTest.SharedArrayThreadSafeBuffer (Skipped)
        152 - ArrayCloneSharedTest.ThreadSafeClone (Skipped)
kona@deblndw013x4j:/usr/home/kona/repositories/arrow-nanoarrow/build (main =)

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.59%. Comparing base (f11d517) to head (04b538f).
⚠️ Report is 63 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #906      +/-   ##
==========================================
- Coverage   79.96%   78.59%   -1.38%     
==========================================
  Files         105      106       +1     
  Lines       15461    16128     +667     
  Branches     1738     1858     +120     
==========================================
+ Hits        12364    12675     +311     
- Misses       1998     2286     +288     
- Partials     1099     1167      +68     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[wjwithagen]

'mmmm,

Not sure if this is the correct fix for FreeBSD.....

But I have my findings from porting Ceph to FreeBSD where Functions calls that under FreeBSD return ENOATTR and Ceph actually tests for ENODATA. These are mostly functions handeling (ex)atributes....
So hence my suggestion with FreeBSD to

*** /usr/include/errno.h        Mon Oct  3 02:05:43 2016
--- /usr/srcs/head/src/sys/sys/errno.h  Sun Aug 21 18:25:05 2016
***************
*** 164,170 ****
  #define       ECANCELED       85              /* Operation canceled */
  #define       EILSEQ          86              /* Illegal byte sequence */
  #define       ENOATTR         87              /* Attribute not found */
- #define       ENODATA         87              /* Attribute not found */

  #define       EDOOFUS         88              /* Programming error */
  #endif /* _POSIX_SOURCE */
--- 164,169 ----

If you fix it in a different way:

• compilation will work
• program will run
• but attribute errors will not be noticed
• wierd errors will need serious debugging

So you atleast need to look at the function that it return got tested agains ENODATA to make sure that tests actually will work as expected.... And I'm pretty sure that just picking a random value will not add to really fiing the problem in Apache tests.

[michael-o]

My problem with ENOATTR that it is logically tied to xattr and would be an abuse. Does it make it better? I am not a huge fan of the downstream patch either. I am open for improvements.

[paleolimbot]

Thank you!

I agree that setting this to a random value is not ideal. Probably the bigger problem is that we're using ENODATA as a sentinel for a special case in the process of reading IPC files (the "end of stream"). There might be a function that surfaces this to users (we could change that and document it), but mostly this is internal.

We could either rewrite the code to avoid using a sentinal errno value at all, or use a different errno define like ENOATTR. It is probably better to use an actual define for anything we return to a user.

In any case, this change is an improvement (also happy to merge something like if !defined(ENODATA) -> ENOATTR as long as it's documented for any function where it actually returns to a user).

[wjwithagen]

My problem with ENOATTR that it is logically tied to xattr and would be an abuse. Does it make it better? I am not a huge fan of the downstream patch either. I am open for improvements.

I totally agree that in essence ENODATA is tied to attributes...
And the Linux manpages for xattr() functions give ENODATA errors when the attributes are not there.
They seems to have made those there aliases.

But just random assigning a leftover number doesn't help either.
I do not know enough of your full set of code, but I found that in Ceph the guys returned ENODATA on some points where ENOATTR would have been more apropriate.
Currently I'm sort of starting to wonder how to fix this mess.

This is another definition that is available:
'''
.//devel/dwarves/files/patch-lib_bpf_src_str__error.c:+#define ENODATA 8817
'''

Which is something I've come arcross in some other libraries. (Which ones I cannot recall at this moment)

So it comes back to why do you want to define it?
And are you totally free to just pick a value for this define, completely outside the scope of the *xattr() functions.
Otherwise I would really refrain from adding even more complexity to this tarpit.

But looking at the objective off this project, where you build a set of functions to be used by others
I'd keep with the mess Linux created and have ENODATA == ENOATTR.
And not add jet again another value to ENODATA.

[paleolimbot]

I don't mind which errno value we use here...I had to choose one of them to use as a sentinel and I didn't know at the time that ENODATA was problematic. The easiest short term solution is to pick a new sentinel that is not ENODATA; a more involved long term solution is to rewrite that portion of the IPC decoder to not use a sentinel value at all.

[michael-o]

I don't mind which errno value we use here...I had to choose one of them to use as a sentinel and I didn't know at the time that ENODATA was problematic. The easiest short term solution is to pick a new sentinel that is not ENODATA; a more involved long term solution is to rewrite that portion of the IPC decoder to not use a sentinel value at all.

Why not have a define of your own since it seems to be internal only? E.g. NA_ENODATA (NA for nanoarrow)? Borrowed from curl, they have CURL_EOK and many others.

[paleolimbot]

These are all great ideas...I opened #909 to track making the situation more robust, but I'll merge in the meantime since this is an improvement over the current failure mode on BSD (and because that particular value, defined internally, does not leak to users in the vast majority of current usage).

[michael-o]

@paleolimbot Thank you for the quick turnaround and do it the ASF way!