Re: [PATCH] erofs-utils: Provide identical functionality without libuuid

[Gao Xiang <hsiangkao@linux.alibaba.com>]

Hi Norbert,

On 2023/7/2 18:19, Norbert Lange wrote:
> The motivation is to have standalone (statically linked) erofs binaries
> for simple initrd images, that are nevertheless able to (re)create
> erofs images with a given UUID.
> 
> For this reason a few of libuuid functions have implementations added
> directly in erofs-utils.
> A header liberofs_uuid.h provides the new functions, which are
> always available. A further sideeffect is that code can be simplified
> which calls into this functionality.
> 
> The uuid_unparse function replacement is always a private
> implementation and split into its own file, this further restricts
> the (optional) dependency on libuuid only to the erofs-mkfs tool.
> 
> Signed-off-by: Norbert Lange <nolange79@gmail.com>

Yeah, overall it looks good to me, some minor nits as below:

(Also currently UUID makes the image nonreproducable, I wonder if we
  could use some image hash to calculate the whole UUID instead...)

> ---
>   dump/Makefile.am    |   2 +-
>   dump/main.c         |   8 +---
>   fsck/Makefile.am    |   2 +-
>   lib/Makefile.am     |   4 +-
>   lib/liberofs_uuid.h |   9 ++++
>   lib/uuid.c          | 106 ++++++++++++++++++++++++++++++++++++++++++++
>   lib/uuid_unparse.c  |  21 +++++++++
>   mkfs/Makefile.am    |   6 +--
>   mkfs/main.c         |  21 ++-------
>   9 files changed, 149 insertions(+), 30 deletions(-)
>   create mode 100644 lib/liberofs_uuid.h
>   create mode 100644 lib/uuid.c
>   create mode 100644 lib/uuid_unparse.c
> 
> diff --git a/dump/Makefile.am b/dump/Makefile.am
> index c2bef6d..90227a5 100644
> --- a/dump/Makefile.am
> +++ b/dump/Makefile.am
> @@ -7,4 +7,4 @@ AM_CPPFLAGS = ${libuuid_CFLAGS}
>   dump_erofs_SOURCES = main.c
>   dump_erofs_CFLAGS = -Wall -I$(top_srcdir)/include
>   dump_erofs_LDADD = $(top_builddir)/lib/liberofs.la ${libselinux_LIBS} \
> -	${libuuid_LIBS} ${liblz4_LIBS} ${liblzma_LIBS}
> +	${liblz4_LIBS} ${liblzma_LIBS}
> diff --git a/dump/main.c b/dump/main.c
> index bc4e028..40af09f 100644
> --- a/dump/main.c
> +++ b/dump/main.c
> @@ -17,10 +17,8 @@
>   #include "erofs/compress.h"
>   #include "erofs/fragments.h"
>   #include "../lib/liberofs_private.h"
> +#include "../lib/liberofs_uuid.h"
>   
> -#ifdef HAVE_LIBUUID
> -#include <uuid.h>
> -#endif
>   
>   struct erofsdump_cfg {
>   	unsigned int totalshow;
> @@ -620,9 +618,7 @@ static void erofsdump_show_superblock(void)
>   		if (feat & feature_lists[i].flag)
>   			fprintf(stdout, "%s ", feature_lists[i].name);
>   	}
> -#ifdef HAVE_LIBUUID
> -	uuid_unparse_lower(sbi.uuid, uuid_str);
> -#endif
> +	erofs_uuid_unparse_lower(sbi.uuid, uuid_str);
>   	fprintf(stdout, "\nFilesystem UUID:                              %s\n",
>   			uuid_str);
>   }
> diff --git a/fsck/Makefile.am b/fsck/Makefile.am
> index e6a1fb6..4176d86 100644
> --- a/fsck/Makefile.am
> +++ b/fsck/Makefile.am
> @@ -7,4 +7,4 @@ AM_CPPFLAGS = ${libuuid_CFLAGS}
>   fsck_erofs_SOURCES = main.c
>   fsck_erofs_CFLAGS = -Wall -I$(top_srcdir)/include
>   fsck_erofs_LDADD = $(top_builddir)/lib/liberofs.la ${libselinux_LIBS} \
> -	${libuuid_LIBS} ${liblz4_LIBS} ${liblzma_LIBS}
> +	${liblz4_LIBS} ${liblzma_LIBS}
> diff --git a/lib/Makefile.am b/lib/Makefile.am
> index faa7311..e243c1c 100644
> --- a/lib/Makefile.am
> +++ b/lib/Makefile.am
> @@ -29,9 +29,9 @@ noinst_HEADERS += compressor.h
>   liberofs_la_SOURCES = config.c io.c cache.c super.c inode.c xattr.c exclude.c \
>   		      namei.c data.c compress.c compressor.c zmap.c decompress.c \
>   		      compress_hints.c hashmap.c sha256.c blobchunk.c dir.c \
> -		      fragments.c rb_tree.c dedupe.c
> +		      fragments.c rb_tree.c dedupe.c uuid_unparse.c uuid.c
>   
> -liberofs_la_CFLAGS = -Wall -I$(top_srcdir)/include
> +liberofs_la_CFLAGS = -Wall ${libuuid_CFLAGS} -I$(top_srcdir)/include
>   if ENABLE_LZ4
>   liberofs_la_CFLAGS += ${LZ4_CFLAGS}
>   liberofs_la_SOURCES += compressor_lz4.c
> diff --git a/lib/liberofs_uuid.h b/lib/liberofs_uuid.h
> new file mode 100644
> index 0000000..d156699
> --- /dev/null
> +++ b/lib/liberofs_uuid.h
> @@ -0,0 +1,9 @@
> +/* SPDX-License-Identifier: GPL-2.0+ OR Apache-2.0 */
> +#ifndef __EROFS_LIB_UUID_H
> +#define __EROFS_LIB_UUID_H
> +
> +void erofs_uuid_generate(unsigned char *out);
> +void erofs_uuid_unparse_lower(const unsigned char *buf, char *out);
> +int erofs_uuid_parse(const char *in, unsigned char *uu);
> +
> +#endif
> \ No newline at end of file
> diff --git a/lib/uuid.c b/lib/uuid.c
> new file mode 100644
> index 0000000..acff81a
> --- /dev/null
> +++ b/lib/uuid.c
> @@ -0,0 +1,106 @@
> +// SPDX-License-Identifier: GPL-2.0+ OR Apache-2.0
> +/*
> + * Copyright (C) 2023 Norbert Lange <nolange79@gmail.com>
> + */
> +
> +#include <string.h>
> +#include <errno.h>
> +
> +#include "erofs/config.h"
> +#include "erofs/defs.h"
> +#include "liberofs_uuid.h"
> +
> +#ifdef HAVE_LIBUUID
> +#include <uuid.h>
> +#else
> +
> +#include <stdlib.h>
> +#include <sys/random.h>
> +
> +/* Flags to be used, will be modified if kernel does not support them */
> +static unsigned erofs_grnd_flag =

Could we switch to "unsigned int" for this?

> +#ifdef GRND_INSECURE
> +    GRND_INSECURE;
> +#else
> +    0x0004;
> +#endif
> +
> +static int s_getrandom(void *pUid, unsigned size, bool insecure)
> +{
> +    unsigned kflags = erofs_grnd_flag;
> +    unsigned flags = insecure ? kflags : 0;

same here.

Otherwise it looks good to me.

Thanks,
Gao Xiang