Hi Mingye,
Thanks a lot for providing your input on this proposal.
On 2021.03.20 09:13, Mingye Wang wrote:
Hi Pete,
(I am not a ntfs-3g maintainer, but) Your project is quite
interesting! A few comments from just some rando on the list follows.
* Patches 0049-0051, with 0050 being the one that adds generic static
assert support to ntfs-3g, have to do with ensuring that MVSC does pack
the layout structures properly.
Regarding the change to compat.h:
* I don't think you need that ifdef for _MSC_VER to get the comparison
to work, since unknown values are assumed to be 0.[1]
The first #ifdef is actually for the following two lines:
#define __inline__ __inline
#define __attribute__(X)
We also happen to have a #if (_MSC_VER >= 1900) comparison for the MSVC
_Static_assert definition, but these are two separate sections, so I
don't think we can remove that first #ifdef.
Also, as surprising at it may sound (and this has been the bane of
Windows developers for years), the default MSVC compiler is *not* C99
compliant (though Microsoft finally added an option for C11 compliance
in recent versions of Visual Studio). So one has to be careful not to
rely on anything C99 when using default MSVC...
* _Static_assert is relatively new to C, so you should probably also
add an #elif for __STDC_VERSION__ < 201112L.
'static_assert()' was formalized as a formal built in macro in C11, but
the _Static_assert() built in macro we use was introduced sooner, as
compiler extensions.
Per [1], we run a configure test to find out if the compiler has support
for _Static_assert, so we don't need an extra version check.
And we do have a version check for the MSVC compiler, so that
_Static_assert() resolves to a noop for the pre VS2015 version.
[1]
https://github.com/pbatard/ntfs-3g/commit/2e0865bca1881ce916f89bea2f3567f7e4a842ac#diff-49473dca262eeab3b4a43002adb08b4db31020d190caaad1594b47f1d5daa810
[1]: https://stackoverflow.com/a/5085425
* Patch 0064... though of course this means that we need to convey that the
this library is under a different license (which the patch does).
Despite the additional license, considering that not having this patch
makes EDK2 compilation on Windows impossible for X64 and IA32, I would
very much like to see it integrated.
Including BSD-2 code does not actually put the whole library "under a
different license" (which would require every contributor to agree);
Yes, I expressed myself poorly here, and I am well aware that adding
BSD-2 code in a GPL project does not change the license of the project.
Just to be clear, the only library I am talking about above is the
CompilerIntrinsicsLib which we are adding with the patch. And this
licensing discussion has to do with indicating that the code we are
adding to this GPLv2 project was originally found under a BSD-2 license.
it's just whatever the part that prints licenses needs to also print
the BSD license disclaimer. If the UEFI driver has an about or version
string, you might need to worry about it a bit.
It's a driver, so there's no about info or about dialog to speak of.
And whereas it does have a version number, the version is implicitly
constrained by the maximum string size that the UEFI Shell 'drivers'
command (which lists all the drivers along with their version) does
allow, which is not much.
Thus, once you display the driver type, version and source project,
along with a short commit hash, there isn't that much space left to
display licenses. If were to display licensing data, it would just be
truncated, and, considering that we are very explicit in the README and
also with a formal copy of the BSD license in the CompilerIntrinsicsLib
directory, formally displaying all of the licenses in the version string
does seem like an overkill to me.
For reference, here is some example output of the current driver version
(along with a couple others) when seen through the 'drivers' command:
V VERSION E G G #D #C DRIVER NAME IMAGE NAME
== ======== = = = == == =================================== ==========
(...)
A7 00000010 B - - 1 1 QEMU Video Driver QemuVideoDxe
A8 00000010 ? - - - - Virtio GPU Driver VirtioGpuDxe
A9 00000010 D - - 1 - NTFS Driver 1.0 (ntfs-3g 0e160412) ntfs_x64.efi
Personally I find it a bit odd that you need to copy code from EDK2
libc when compiling stuff for EDK2, but a quick search indicates that
it's by design due to the presence of some fancy shell-related things.
Indeed. That's the curse of nostdlib (and no-builtin, which EDK2 applies
for good measure) that one must use when building next to bare-metal
executables. A lot of standard calls are thrown out of the window, and
require re-implementation...
* Patch 0065 adds gnu-efi as a submodule. gnu-efi [2] is a much leaner
alternative to EDK2, that can make the process of building UEFI
executables a lot less daunting for people who don't already have
experience with tianocore/EDK2.
If you do have clang on the Linux environment you are testing with, it
might also be a good idea to see how clang does with gnu-efi and
./configure (with appropriate CFLAGS and LDFLAGS). Clang can be a bit
easier to set up since there's no need for a cross-compile compiler to
be enabled.
Yes, I have some plans to test with Clang eventually, but my main
targets for this project remain gcc and MSVC.
Oh, and it should be possible to make gcc/gnu-efi work on Linux, for
those who don't want to use EDK2 (in which case it might be desirable
for ntfs-3g to formally add gnu-efi as a submodule, since, again, it is
very lightweight), but integrating this in the current autotools scripts
will require some effort, which is why I limited gnu-efi support to
Visual Studio where it was a lot simpler to add.
PS: Until now I didn't know that EDK2, like gnu-efi, also relied on
turning ELF output into PE on Linux. The more you know...
PPS: On that note, I should probably try cloning and building with the
EDK2 CLANGPDB toolchain too.
Sounds good. Don't hesitate to let me know if there are other points
that strike you as odd, or if you find issues.
Regards,
/Pete
_______________________________________________
ntfs-3g-devel mailing list
ntfs-3g-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ntfs-3g-devel
