[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).



Hey Felipe,

I have some comments for this patch. Maybe I need they need a bit more clarification.

Thanks,

Simon

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.

+           select UKUNISTD

Shouldn't this be LIBUKUNISTD?

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.

+
+################################################################################
+# 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:

+
+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.

+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.

+uuid_generate_random
+__uuid_generate_time

Is __uuid_generate_time really a public interfaces provided by the library?

+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.

+#define HAVE_NETINET_IN_H 1
+#endif
+
+/* Define to 1 if you have the `socket' function. */
+#ifndef $(CONFIG_LIBLWIP)

Same here.

+#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. ;-)

+
+/* 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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.