cleaned-up macOS port (not fsck-related) #14
Conversation
| #elif __BYTE_ORDER == LITTLE_ENDIAN | ||
| #define __LITTLE_ENDIAN_BITFIELD | ||
| #endif | ||
| #endif /* __APPLE__ */ |
There was a problem hiding this comment.
Please do not invent new way for determining endiantity. We already use autoconf which has standard macros for it.
There was a problem hiding this comment.
It's how it works without reshuffling too much the chain of ifdef'ed code.
You have to consider that I'm not actually determining there endianness, only perusing how it is pre-defined in the macOS-specific way (i.e., as in machine/endian.h). Indeed the right side of the defines (no double under-score) comes from the Darwin header.
There was a problem hiding this comment.
Autoconf has macro AC_C_BIGENDIAN to check for endianity, see:
https://www.gnu.org/software/autoconf/manual/autoconf.html
There is absolutely no reason for adding new special ifdef for Apple systems as autoconf can handle it.
There was a problem hiding this comment.
Tried, but it does not work. The ifdef'ed HAVE_SYS_ISA_DEFS_H part is a bit non-standard and makes the following (ifdef APPLE) necessary. Again, consider that it is not determining endianness, only doing what you do under HAVE_SYS_ISA_DEFS_H (e.g., define the __*) which are not available anywhere on a Darwin system otherwise.
There was a problem hiding this comment.
But why to use HAVE_SYS_ISA_DEFS_H? WORDS_BIGENDIAN should be defined only on big endian systems.
| * borrow the "Operation not supported" error from the network folks to | ||
| * accomplish this. Maybe someday we will get a more targeted error code, | ||
| * but this will do for now... */ | ||
| #define EDRIVE_CANT_DO_THIS EOPNOTSUPP |
There was a problem hiding this comment.
Where is this EDRIVE_CANT_DO_THIS used? I do not see place where is needed this nor any other macros. So please do not include and bundle dead code.
There was a problem hiding this comment.
As per other comments of mine, the alternative is to prune (Linux-verbatim) code to the minimum, of course it could make it life more difficult to reintroduce defines/etc later down the road, if needed by new (core) code in udftools.
Anyway, albeit we're talking about three/four self-contained files almost verbatim from /usr/include in Linux, if you feel like this is absolutely needed, please let me know, and I'll take care of the pruning.
| #include <linux/fs.h> | ||
| #include <linux/fd.h> | ||
| #elif defined(__APPLE__) | ||
| #include <sys/disk.h> |
There was a problem hiding this comment.
Please add comment for which function (or macro?) is this sys/disk.h include file needed. I really do not know.
There was a problem hiding this comment.
Sure thanks, I'll add a comment straight away. It's the Darwin equivalent of fs.h (and also tackles fd.h).
| #include <linux/cdrom.h> | ||
| #else | ||
| #include "darwin/cdrom.h" | ||
| #endif |
There was a problem hiding this comment.
Are you sure that your Darwin system implement pktcdvd interface from Linux kernel?
There was a problem hiding this comment.
Not sure about the actual working of everything in this PR: with these patches it all compiles, and runs (tried out all binaries with help/version switches). Of course, (possibly missing) functionality is to be tested (I'm willing to do it going forward, of course).
| @@ -229,7 +229,7 @@ extern size_t decode_string(struct udf_disc *, const dstring *, char *, size_t, | |||
| extern size_t encode_string(struct udf_disc *, dstring *, const char *, size_t); | |||
|
|
|||
| /* misc.c */ | |||
| extern const char *appname; | |||
| const char *appname; | |||
There was a problem hiding this comment.
This is wrong change as variable appname is already defined in misc.c. Therefore it can be only declared, not defined.
There was a problem hiding this comment.
This is the kind of error I get in folder mkudffs, if I don't drop that extern:
/bin/sh ../libtool --tag=CC --mode=link gcc -g -O2 -o mkudffs main.o mkudffs.o defaults.o file.o options.o ../libudffs/libudffs.la
libtool: link: gcc -g -O2 -o mkudffs main.o mkudffs.o defaults.o file.o options.o ../libudffs/.libs/libudffs.a
Undefined symbols for architecture x86_64:
"_appname", referenced from:
_main in main.o
_split_space in mkudffs.o
_setup_anchor in mkudffs.o
_setup_partition in mkudffs.o
_setup_space in mkudffs.o
_setup_vat in mkudffs.o
_setup_vds in mkudffs.o
There was a problem hiding this comment.
It is defined in misc.c which is linked into libudffs.a. If linker does not see appname in libudffs.a when building mkudffs binary, you would need to debug build process and find out what is happening there.
I do not know Apple nor Darwin systems, so I cannot help with this part.
|
Thanks for the comments, each of which I replied to already. About your general remarks: most of the code you deem as (partly) dead code, is actually just bundled code, e.g., cdrom.h is copied/adapted from Linux system headers, not available under Darwin otherwise. The rationale, of course, is that the three/four additional (new) files are meant to be substitutes for basic data types/structures/constants which are otherwise Linux-only (but not bound to the kernel): indeed the same pattern I explored has been used, piece-wise, in other porting projects by third parties, so I guess it makes sense. About pktsetup: I guess you are right, but as discussed in the corresponding comment, it's a (first) step, at least to make the binaries all compile, and be launched in, e.g., demo mode at least, i.e., for automated testing like Travis. |
|
Those currently used linux-only headers are there for communication with Linux kernel. They cannot be copied from Linux system to non-Linux and hoping that something would work. Their functionality needs to be replaced by target system and I'm sure target system would provide own headers file (with different API). All this needs to be tested that it is working. I'm not going to merge any broken or nonfunctional code. Before merging it needs to be tested that port is working correctly. |
|
The ifded'ed headers, e.g. sys/disk.h, are equivalent to (differently-named) Linux headers, indeed providing the same interface. The additional ones, e.g., darwin/cdrom.h, have no straight equivalent available, on one hand, and once slightly adapted as I did, provide no dependencies on the Linux kernel, mostly constants; on the other, these lead to a start-to-finish compilation and binaries which can be started, so at least not (fully) broken, but (quite possibly) partially working. I guess this can be an initial step to have the port. Of course, once certain binaries/features are tested (sooner rather than later), and found broken and/or incomplete, those can be excluded from compilation under macOS (or certain switches disabled, respectively) until fixed, but in the meantime the build can be obtained, and automated (e.g., my Homebrew formula and the convenience that it brings). Let me know. PS: it should be clearly stated in the documentation, anyway, if/which parts of (any) port are incomplete/broken. To add some perspective of mine, until recently (when you started being the new -- active -- maintainer) udftools was almost abandonware, so (partly) broken. |
Thanks to your latest advice:
#13 (comment)
I've cleaned up the PR: no more CDDL-encumbered code, just enough (otherwise missing) headers, slightly adapted from Linux ones, to let the compiler stop complaining. By the way, with this groundwork out of the way, it is a very straightforward port to FreeBSD, as well.
This is a quite different PR with respect to the fsck work in: argorain#1
I've tried to keep things as separate (and as simple) as possible, because I think it is valuable to have the macOS port merged even before the fsck is fully upstreamed.
Let me know your opinion.