|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Minios-devel] [UNIKRAFT/LIBUUID PATCH v2 2/2] Initial port of the libuuid library (version 1.0.3).
Hi Simon,
Thanks for the review, please see inline.
-- Felipe
On 15.04.19 10:21, Felipe Huici wrote:
> Note newlib is required.
>
> Signed-off-by: Felipe Huici <felipe.huici@xxxxxxxxx>
> ---
> Config.uk | 5 ++
> Makefile.uk | 100
+++++++++++++++++++++++++
> exportsyms.uk | 19 +++++
> include/config.h | 77 +++++++++++++++++++
> patches/0001-add-syscall-h-compile-guard.patch | 12 +++
> 5 files changed, 213 insertions(+)
> create mode 100644 Config.uk
> create mode 100644 Makefile.uk
> create mode 100644 exportsyms.uk
> create mode 100644 include/config.h
> create mode 100644 patches/0001-add-syscall-h-compile-guard.patch
>
> diff --git a/Config.uk b/Config.uk
> new file mode 100644
> index 0000000..c6d0769
> --- /dev/null
> +++ b/Config.uk
> @@ -0,0 +1,5 @@
> +menuconfig LIBUUID
> + bool "libuuid - library for unique id generation"
> + default n
> + select HAVE_LIBC
You should use "depends on HAVE_LIBC" instead. We introduced these
HAVE_* feature variables to communicate that some "standard" features
are provided by a library. Your line is stating that LIBUUID is a libc
"provider" (see libs/Config.uk). I know, this stuff is confusing.
> Ok, will do.
> + select UKUNISTD
Shouldn't this be LIBUKUNISTD?
> No, it's UKUNISTD. If you want it to be LIBUKUNISTD I guess we would need to
> submit a patch to change that lib's name first.
> diff --git a/Makefile.uk b/Makefile.uk
> new file mode 100644
> index 0000000..ed1bc39
> --- /dev/null
> +++ b/Makefile.uk
> @@ -0,0 +1,100 @@
> +# libuuid Makefile.uc
> +#
> +# Authors: Felipe Huici <felipe.huici@xxxxxxxxx>
> +#
> +#
> +# Copyright (c) 2019, NEC Europe Ltd., NEC Corporation. All rights
reserved.
> +#
> +# Redistribution and use in source and binary forms, with or without
> +# modification, are permitted provided that the following conditions
> +# are met:
> +#
> +# 1. Redistributions of source code must retain the above copyright
> +# notice, this list of conditions and the following disclaimer.
> +# 2. Redistributions in binary form must reproduce the above copyright
> +# notice, this list of conditions and the following disclaimer in the
> +# documentation and/or other materials provided with the
distribution.
> +# 3. Neither the name of the copyright holder nor the names of its
> +# contributors may be used to endorse or promote products derived
from
> +# this software without specific prior written permission.
> +#
> +# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
"AS IS"
> +# AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
THE
> +# IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
PURPOSE
> +# ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR
CONTRIBUTORS BE
> +# LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> +# CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> +# SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
BUSINESS
> +# INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER
IN
> +# CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR
OTHERWISE)
> +# ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED
OF THE
> +# POSSIBILITY OF SUCH DAMAGE.
> +#
> +# THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
> +#
> +
>
+################################################################################
> +# Library registration
>
+################################################################################
> +$(eval $(call addlib_s,libuuid,$(CONFIG_LIBUUID)))
> +
>
+################################################################################
> +# Sources
>
+################################################################################
> +LIBUUID_VERSION=1.0.3
>
+LIBUUID_URL=https://sourceforge.net/projects/libuuid/files/libuuid-$(LIBUUID_VERSION).tar.gz/download
> +LIBUUID_PATCHDIR=$(LIBUUID_BASE)/patches
> +$(eval $(call fetchas,libuuid,$(LIBUUID_URL),$(LIBUUID_VERSION).tgz))
> +$(eval $(call
patch,libuuid,$(LIBUUID_PATCHDIR),libuuid-$(LIBUUID_VERSION)))
> +
>
+################################################################################
> +# Helpers
>
+################################################################################
> +LIBUUID_SUBDIR=libuuid-$(LIBUUID_VERSION)
> +LIBUUID_SRC=$(LIBUUID_ORIGIN)/$(LIBUUID_SUBDIR)
> +
>
+################################################################################
> +# Library includes
>
+################################################################################
> +CINCLUDES-$(CONFIG_LIBUUID) += -I$(LIBUUID_BASE)/include
Oh, this is making <config.h> available also outside of this library.
Are you sure you want this? I think this is dangerous and will
potentially clash with other libraries later.
I actually would expect that this config header is only used within the
library scope.
> Yeah, right, probably not a good idea, this would be brittle. I'll fix this
> in a v2.
> +
>
+################################################################################
> +# Global flags
>
+################################################################################
> +LIBUUID_CFLAGS-y += -DHAVE_CONFIG_H
> +
> +# Suppress some warnings to make the build process look neater
> +SUPPRESS_FLAGS += -Wno-unused-parameter -Wno-unused-variable
-Wno-nonnull \
> +-Wno-unused-but-set-variable -Wno-unused-label -Wno-char-subscripts
\
> +-Wno-unused-function -Wno-missing-field-initializers -Wno-uninitialized
\
> +-Wno-array-bounds -Wno-maybe-uninitialized -Wno-pointer-sign
-Wno-unused-value \
> +-Wno-unused-macros -Wno-parentheses -Wno-implicit-function-declaration
\
> +-Wno-missing-braces -Wno-endif-labels -Wno-unused-but-set-variable
\
> +-Wno-implicit-function-declaration -Wno-type-limits -Wno-sign-compare
Are you sure you want all of them? I am especially concerned about
-Wno-array-bounds, -Wno-type-limits, and -Wno-sign-compare. You should
also namespace your SUPPRESS_FLAGS variable name:
> Will fix.
> +
> +LIBUUID_CFLAGS-y += $(SUPPRESS_FLAGS)
> +LIBUUID_CXXFLAGS-y += $(SUPPRESS_FLAGS)
> +
>
+################################################################################
> +# Sources
>
+################################################################################
> +LIBUUID_SRCS-y += $(LIBUUID_SRC)/clear.c
> +LIBUUID_SRCS-y += $(LIBUUID_SRC)/copy.c
> +LIBUUID_SRCS-y += $(LIBUUID_SRC)/isnull.c
> +LIBUUID_SRCS-y += $(LIBUUID_SRC)/parse.c
> +LIBUUID_SRCS-y += $(LIBUUID_SRC)/unparse.c
> +LIBUUID_SRCS-y += $(LIBUUID_SRC)/compare.c
> +LIBUUID_SRCS-y += $(LIBUUID_SRC)/gen_uuid.c
> +LIBUUID_SRCS-y += $(LIBUUID_SRC)/pack.c
> +LIBUUID_SRCS-y += $(LIBUUID_SRC)/randutils.c
> +LIBUUID_SRCS-y += $(LIBUUID_SRC)/unpack.c
> +LIBUUID_SRCS-y += $(LIBUUID_SRC)/uuid_time.c
> +
>
+################################################################################
> +# Lib-specific Targets - ensure users can #include <uuid/uuid.h>
>
+################################################################################
> +$(LIBUUID_BUILD)/.prepared:
> + $(call verbose_cmd,CONFIGURE,libuuid: $@,\
Instead of CONFIGURE, you should use LN as command description. You
probably can also use build_cmd instead of verbose_cmd.
> + ln -s $(LIBUUID_SRC) $(LIBUUID_BASE)/include/uuid && \
> + $(TOUCH) $@)
Instead of this I would actually create a new library sub build
directory and link only necessary public headers there. This way you do
a much cleaner separation of internal and external headers:
$(call mk_sub_build_dir,libuuid,include/uuid)
$(LIBUUID_BUILD)/.prepared:
$(call verbose_cmd,LN,libuuid: header1.h,\
ln -s $(LIBUUID_SRC)/header1.h \
$(call sub_build_dir,libuuid,include)/uuid/header1.h)
$(call verbose_cmd,LN,libuuid: header2.h,\
ln -s $(LIBUUID_SRC)/header2.h \
$(call sub_build_dir,libuuid,include)/uuid/header2.h)
@$(TOUCH) $@
You then register this new include folder globally:
CINCLUDES-$(CONFIG_LIBUUID) += -I$(call sub_build_dir,libuuid,include)
Please check this code example, it may not work because I did not tested
it. I just wanted to give an idea.
> Ok, I think I get the point, I'll fix this in v2.
> +UK_PREPARE += $(LIBUUID_BUILD)/.prepared
> diff --git a/exportsyms.uk b/exportsyms.uk
> new file mode 100644
> index 0000000..e94d21e
> --- /dev/null
> +++ b/exportsyms.uk
> @@ -0,0 +1,19 @@
> +uuid_clear
> +uuid_compare
> +uuid_copy
> +uuid_generate
> +__uuid_generate_random
Is __uuid_generate_random really a public interfaces provided by the
library? I guess it is not because there is also uuid_generate_random.
> No, it's not, it's just me being lazy and dumping the output of nm. I'll fix
> this.
> +uuid_generate_random
> +__uuid_generate_time
Is __uuid_generate_time really a public interfaces provided by the library?
> Same as above.
> +uuid_generate_time
> +uuid_generate_time_safe
> +uuid_is_null
> +uuid_pack
> +uuid_parse
> +uuid_time
> +uuid_type
> +uuid_unpack
> +uuid_unparse
> +uuid_unparse_lower
> +uuid_unparse_upper
> +uuid_variant
> diff --git a/include/config.h b/include/config.h
> new file mode 100644
> index 0000000..b818210
> --- /dev/null
> +++ b/include/config.h
> @@ -0,0 +1,77 @@
> +/* config.h. Generated from config.h.in by libuuid's configure. */
> +/* config.h.in. Generated from configure.ac by autoheader. */
> +
> +/* Define to 1 if you have the <fcntl.h> header file. */
> +#define HAVE_FCNTL_H 1
> +
> +/* Define to 1 if you have the `ftruncate' function. */
> +#define HAVE_FTRUNCATE 1
> +
> +/* Define to 1 if you have the `gettimeofday' function. */
> +#define HAVE_GETTIMEOFDAY 1
> +
> +/* Define to 1 if you have the <inttypes.h> header file. */
> +#define HAVE_INTTYPES_H 1
> +
> +/* Define to 1 if you have the <limits.h> header file. */
> +#define HAVE_LIMITS_H 1
> +
> +/* Define to 1 if you have the <memory.h> header file. */
> +#define HAVE_MEMORY_H 1
> +
> +/* Define to 1 if you have the `memset' function. */
> +#define HAVE_MEMSET 1
> +
> +/* Define to 1 if you have the <netinet/in.h> header file. */
> +#ifndef $(CONFIG_LIBLWIP)
Shouldn't this be the opposite way around?: #ifdef $(CONFIG_LIBLWIP)
Btw, you could also depend on the generic feature flag: $(HAVE_NW_STACK)
instead.
> Yes, will fix.
> +#define HAVE_NETINET_IN_H 1
> +#endif
> +
> +/* Define to 1 if you have the `socket' function. */
> +#ifndef $(CONFIG_LIBLWIP)
Same here.
> Will fix.
> +#define HAVE_SOCKET 1
> +#endif
> +
> +/* Define to 1 if you have the `srandom' function. */
> +#define HAVE_SRANDOM 1
> +
> +/* Define to 1 if you have the <stdint.h> header file. */
> +#define HAVE_STDINT_H 1
> +
> +/* Define to 1 if you have the <stdlib.h> header file. */
> +#define HAVE_STDLIB_H 1
> +
> +/* Define to 1 if you have the <strings.h> header file. */
> +#define HAVE_STRINGS_H 1
> +
> +/* Define to 1 if you have the <string.h> header file. */
> +#define HAVE_STRING_H 1
> +
> +/* Define to 1 if you have the `strtoul' function. */
> +#define HAVE_STRTOUL 1
> +
> +/* Define to 1 if you have the <sys/file.h> header file. */
> +#define HAVE_SYS_FILE_H 1
> +
> +/* Define to 1 if you have the <sys/ioctl.h> header file. */
> +#define HAVE_SYS_IOCTL_H 1
> +
> +/* Define to 1 if you have the <sys/socket.h> header file. */
> +#ifndef $(CONFIG_LIBLWIP)
> +#define HAVE_SYS_SOCKET_H 1
> +#endif
...and here again. ;-)
> Will fix.
> +
> +/* Define to 1 if you have the <sys/stat.h> header file. */
> +#define HAVE_SYS_STAT_H 1
> +
> +/* Define to 1 if you have the <sys/time.h> header file. */
> +#define HAVE_SYS_TIME_H 1
> +
> +/* Define to 1 if you have the <sys/types.h> header file. */
> +#define HAVE_SYS_TYPES_H 1
> +
> +/* Define to 1 if you have the <unistd.h> header file. */
> +#define HAVE_UNISTD_H 1
> +
> +/* Define to 1 if you have the `usleep' function. */
> +#define HAVE_USLEEP 1
> diff --git a/patches/0001-add-syscall-h-compile-guard.patch
b/patches/0001-add-syscall-h-compile-guard.patch
> new file mode 100644
> index 0000000..adea66a
> --- /dev/null
> +++ b/patches/0001-add-syscall-h-compile-guard.patch
> @@ -0,0 +1,12 @@
> +--- a/randutils.c 2019-04-03 14:46:14.827682485 +0200
> ++++ b/randutils.c 2019-04-03 14:46:48.375286950 +0200
> +@@ -13,7 +13,9 @@
> + #include <string.h>
> + #include <sys/time.h>
> +
> ++#ifdef DO_JRAND_MIX
> + #include <sys/syscall.h>
> ++#endif
> +
> + #include "randutils.h"
> +
>
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |