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

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

On 2023/7/3 06:16, Norbert Lange wrote:
> Am So., 2. Juli 2023 um 13:07 Uhr schrieb 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...)
> 
> Not the usecase I am after, I need to use an already known UUID for
> mounting, even if the content changed.

Okay.

> 
>>
>>> ---
>>>    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?
> 
> Sure
> 
>>
>>> +#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.
> 
> Found few a formatting inconsistencies as well, already fixed this locally.
> Guess I will wait a bit more for feedback before posting a v2? Not sure
> whats the proper order.

Looks good to me, yet erofs-utils has rare people to work on reviewing,
I'm fine with either way.

Thanks,
Gao Xiang