Re: [PATCH 05/11] xfsprogs: missing and dummy calls for OS X support

Christoph Hellwig
2015-08-17T19:32:25Z

On Mon, Aug 17, 2015 at 06:23:20PM +0200, Jan Tulak wrote:
> Add and update various API, macros and types where is some change
> in OS X or xfsprogs. Most changes are in darwin.h.
>
> Add dummy implementations where native support is nonexistent
> and the tools are not expected to work anyway, so all tools can be
> at least compiled.
>
> Signed-off-by: Jan Tulak <jtulak@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
> fsr/xfs_fsr.c | 8 +++
> include/darwin.h | 153 +++++++++++++++++++++++++++++++++++++++++++++++++------
> 2 files changed, 145 insertions(+), 16 deletions(-)
>
> diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
> index 7c1d776..b4ff136 100644
> --- a/fsr/xfs_fsr.c
> +++ b/fsr/xfs_fsr.c
> @@ -43,6 +43,14 @@
> #define _PATH_FSRLAST "/var/tmp/.fsrlast_xfs"
> #define _PATH_PROC_MOUNTS "/proc/mounts"
>
> +#ifdef USE_DUMMY_XATTR
> + /* OS X has fsetxattr with different number of arguments.
> + * Because it won't work anyway (no fstab/mtab and so on),
> + * hijack the call to a dummy function so it can at least
> + * compile.
> + */
> +# define fsetxattr(a,b,c,d,e) dummy_fsetxattr(a,b,c,d,e)
> +#endif

I don't think this is the right way to do it. I'd rather:

a) check for fsetxattr using autoconf, and
b) stub out the whole code calling it.
c) make this a patch on it's own

>
> char *progname;
>
> diff --git a/include/darwin.h b/include/darwin.h
> index 8b5a661..775dfc8 100644
> --- a/include/darwin.h
> +++ b/include/darwin.h
> @@ -76,45 +76,37 @@ static __inline__ void platform_getoptreset(void)
>
> static __inline__ int platform_uuid_compare(uuid_t *uu1, uuid_t *uu2)
> {
> - return uuid_compare(uu1, uu2, NULL);
> + return uuid_compare((const unsigned char *) uu1, (const unsigned char*) uu2);

Do you have any idea why MacOS defines a uuid_t, but wants unsigned
char arguments to uuid_compare?

Or given that the others work with pointer dereferences why doesn't:

return uuid_compare(*uu1, *uu2);

work?

Also please make the uuid changes a patch on it's own.

> -#define ENOATTR 989 /* Attribute not found */
> #define EFSCORRUPTED 990 /* Filesystem is corrupted */
> #define EFSBADCRC 991 /* Bad CRC detected */
> #define constpp char * const *

> +#define XATTR_SIZE_MAX 65536 /* size of an extended attribute value (64k) */
> +#define XATTR_LIST_MAX 65536 /* size of extended attribute namelist (64k) */

Eww, looks like we depend on these Linux values in the on disk
defintion. I think we need to add new XFS_XATTR_SIZE_MAX and
XFS_XATTR_LIST_MAX defintions to xfs_format.h and use them where
we currently use these.

> +/*
> + * Dummy POSIX timer replacement
> + */
> +#define CLOCK_REALTIME 1
> +typedef uint64_t timer_t;
> +typedef double timer_c;
> +typedef clock_id_t clockid_t;
> +struct itimerspec
> + {
> + struct timespec it_interval;
> + struct timespec it_value;
> + };
> +
> +static inline int timer_create (clockid_t __clock_id,
> + struct sigevent *__restrict __evp,
> + timer_t *__restrict __timerid)
> +{
> + return 0;
> +}
> +
> +static inline int timer_settime (timer_t __timerid, int __flags,
> + const struct itimerspec *__restrict __value,
> + struct itimerspec *__restrict __ovalue)
> +{
> + return 0;
> +}
> +
> +static inline int timer_delete (timer_t __timerid)
> +{
> + return 0;
> +}
> +
> +static inline int timer_gettime (timer_t __timerid, struct itimerspec *__value)
> +{
> + return 0;
> +}

I don't think these stubs will work. It might be worth to figure
out what people use as replacement for realtime clocks on MacOS
by searching various programming resoures on the internet.

> +static inline int nftw64(const char *path, int (*fn)(const char *, const struct stat *ptr, int flag, struct FTW *), int depth,
> + int flags)
> +{
> + return nftw(path, fn, depth, flags);
> +}
> +
> +#define MREMAP_FIXED 1
> +#define MREMAP_MAYMOVE 2
> +static inline void *mremap(void *old_address, size_t old_size,
> + size_t new_size, int flags, ... /* void *new_address */)
> +{
> + return NULL;
> +}
> +
> +/* FSR */

Please just build the mremap file in xfs_io conditional.

> +typedef int __fsblkcnt_t;
> +typedef int __fsfilcnt_t;
> +typedef long long int __fsblkcnt64_t;
> +typedef long long int __fsfilcnt64_t;
> +
> +struct statvfs64
> +{
> + unsigned long int f_bsize;
> + unsigned long int f_frsize;
> + __fsblkcnt64_t f_blocks;
> + __fsblkcnt64_t f_bfree;
> + __fsblkcnt64_t f_bavail;
> + __fsfilcnt64_t f_files;
> + __fsfilcnt64_t f_ffree;
> + __fsfilcnt64_t f_favail;
> + unsigned long int f_fsid;
> + int __f_unused;
> + unsigned long int f_flag;
> + unsigned long int f_namemax;
> + int __f_spare[6];
> +};

Does MacOS support statvfs? or statfs? We should use the proper API
here instead of a dummy. Or maybe just not build fsr on OSX for now,
as that would solve the fsetxattr issue as well.

> +struct mntent
> +{

Another fsr issue, right? Seems like it would need similar getmntinfo
based code. As a stopgap I'd suggest we stop building fsr on OSX for
now.