basu icon indicating copy to clipboard operation
basu copied to clipboard

Namespace internal struct ucred

Open tuxillo opened this issue 4 years ago • 4 comments
trafficstars

It looks to me as if struct ucred is for internal basu usage (I might be wrong):

https://github.com/emersion/basu/blob/8cd22051ef8e238e3682825cc00bfd5198450076/src/basic/socket-util.h#L27

If that's the case, can it be renamed to struct basu_ucred ?

Thanks!

tuxillo avatar Feb 14 '21 17:02 tuxillo

I assume you're experiencing issues as a result? If so, please describe these. Otherwise this just seems like a style change suggestion.

A PR that renames the internal struct ucred to struct basu_cred would be accepted, which implies writing conversion functions to/from struct ucred and struct xucred and changing all use to struct basu_cred

kennylevinsen avatar Feb 14 '21 18:02 kennylevinsen

This isn't a public header anyways. No need to use prefixes if we just use it internally.

emersion avatar Feb 14 '21 18:02 emersion

In FreeBSD, struct ucred is under _KERNEL or WANT_UCRED so there is no collision. In DragonFly BSD that is not the case, hence there is a redefinition error. Other BSDs might have the same issue but I have not checked.

Even tho the structure is internal, it can potentially collide with definitions from sys/ucred.h as far as I can tell.

tuxillo avatar Feb 14 '21 18:02 tuxillo

Options:

  1. Gate struct ucred better - if DragonFly BSD already has it, and it is compatible, there is no need to define it.
  2. Make FreeBSD expose struct ucred - We still need to map to/from xucred, as that's what the ioctls use, but there's no need to define it then.
  3. Make our own internal credential structure and map on all OS's regardless of native structure.

The current struct ucred definition was made to allow drop-in support for FreeBSD in the UNIX peer credentials code paths, as the structure was used throughout the codebase. I don't think there's that many uses left though, so it wouldn't be that big a patch to change.

kennylevinsen avatar Feb 14 '21 18:02 kennylevinsen