[Libguestfs] [PATCH nbdkit 3/9] server: Add general replacements for missing functions using LIBOBJS.

Tue Aug 18 13:20:44 UTC 2020

On 8/18/20 5:50 AM, Richard W.M. Jones wrote:
> Especially on Windows, some common functions are missing.  Use the
> autoconf LIBOBJS mechanism to replace these functions.
> 
> This includes replacement functions for:
> 
>    Function names     Implementation   Origin
> 
>    getdelim, getline  general purpose  NetBSD under a compatible license
> 
>    openlog, syslog,   Win32            written by me
>    vsyslog
> 
>    realpath           Win32            written by me
> 
>    strndup            general purpose  written by me
> 
> This should do nothing on existing supported platforms.  It is only
> intended in preparation for porting nbdkit to Windows.
> ---

> @@ -464,6 +475,15 @@ AS_CASE([$host_os],
>   AC_MSG_RESULT([$is_windows])
>   AC_SUBST([NO_UNDEFINED_ON_WINDOWS])
>   AC_SUBST([LINK_LIBNBDKIT_ON_WINDOWS])
> +AM_CONDITIONAL([IS_WINDOWS],[test "x$is_windows" = "xyes"])
> +
> +dnl For Windows, look for the mc/windmc utility.
> +dnl XXX Do we need to check for mc.exe as well?
> +AS_IF([test "x$is_windows" = "xyes"],[
> +    AC_CHECK_TOOLS([MC],[windmc mc],[no])
> +    AS_IF([test "x$MC" = "xno"],
> +          [AC_MSG_ERROR([mc/windmc utility must be available when compiling for Windows])])
> +])

That's true for native windows, but not for cygwin.  This one looks like 
our grouping of Cygwin as being 'is_windows' will bite us.

> +++ b/common/replacements/Makefile.am
> @@ -0,0 +1,53 @@
> +# nbdkit
> +# Copyright (C) 2019 Red Hat Inc.

You can add 2020 if desired.

> +++ b/common/replacements/win32/Makefile.am
> @@ -0,0 +1,46 @@
> +# nbdkit
> +# Copyright (C) 2019 Red Hat Inc.

and again

> +# All rights reserved.

Stale copy-and-paste; we got rid of these lines in 1.12.

> +EXTRA_DIST = nbdkit-cat.mc
> +
> +if IS_WINDOWS
> +
> +# Build the message catalog.
> +noinst_DATA = MSG00001.bin nbdkit-cat.h nbdkit-cat.rc
> +
> +$(noinst_DATA): nbdkit-cat.mc
> +	rm -f $@
> +	$(MC) $<

Is the message catalog integral to the general function replacements, or 
should this be split into a separate commit?

> +++ b/common/replacements/getline.h
> @@ -0,0 +1,50 @@
> +/* nbdkit
> + * Copyright (C) 2019 Red Hat Inc.
> + * All rights reserved.

More of this line; looks like you'll want to scrub the series.

> +
> +#ifndef NBDKIT_GETLINE_H
> +#define NBDKIT_GETLINE_H
> +
> +#include <config.h>
> +
> +#ifdef HAVE_GETLINE
> +
> +#include <stdio.h>

You'll want this include to be unconditional, since...

> +
> +#else
> +
> +ssize_t getline (char **lineptr, size_t *n, FILE *stream);

the reference to ssize_t and FILE* depend on it.

> +++ b/common/replacements/realpath.h

> +
> +#include <config.h>
> +
> +#ifdef HAVE_REALPATH
> +
> +#include <limits.h>
> +#include <stdlib.h>

Why limits.h?

> +
> +#else
> +
> +char *realpath (const char *path, char *out);
> +
> +#endif
> +
> +#endif /* NBDKIT_REALPATH_H */
> diff --git a/common/replacements/strndup.h b/common/replacements/strndup.h
> new file mode 100644
> index 00000000..464ce954
> --- /dev/null
> +++ b/common/replacements/strndup.h

> +#ifndef NBDKIT_STRNDUP_H
> +#define NBDKIT_STRNDUP_H
> +
> +#include <config.h>
> +
> +#ifdef HAVE_STRNDUP
> +
> +#include <string.h>
> +
> +#else
> +
> +char *strndup(const char *s, size_t n);

You'll probably want a header to guarantee size_t is defined (either 
stddef.h or sys/types.h works).

> +
> +#endif
> +
> +#endif /* NBDKIT_STRNDUP_H */
> diff --git a/common/replacements/syslog.h b/common/replacements/syslog.h
> new file mode 100644
> index 00000000..e4d76677
> --- /dev/null
> +++ b/common/replacements/syslog.h

> +#include <config.h>
> +
> +#ifdef HAVE_SYSLOG_H
> +
> +#include_next <syslog.h>

I guess our insistence on a decent compiler (for other reasons such as 
__attribute__((cleanup))) means we can rely on #include_next ;)

> +++ b/common/replacements/getdelim.c
> @@ -0,0 +1,84 @@
> +/*	$NetBSD: getdelim.c,v 1.2 2015/12/25 20:12:46 joerg Exp $	*/
> +/*	NetBSD-src: getline.c,v 1.2 2014/09/16 17:23:50 christos Exp 	*/
> +
> +/*-
> + * Copyright (c) 2011 The NetBSD Foundation, Inc.
> + * All rights reserved.

This one makes sense; you are lifting it straight out of another source 
verbatim.

> +++ b/common/replacements/openlog.c

> +#ifdef WIN32
> +
> +/* Replacement openlog for Win32. */
> +
> +#include <windows.h>
> +
> +HANDLE event_source = INVALID_HANDLE_VALUE;
> +
> +void
> +openlog (const char *ident, int option, int facility)
> +{
> +  event_source = RegisterEventSource (NULL, ident);
> +}

Ah, so if I understand, the message catalog compilation matters when 
replacing openlog?  Cygwin has openlog, and thus does not need the 
replacement and thus not the message catalog.

> +
> +#else /* !WIN32 */
> +#error "no replacement openlog is available on this platform"
> +#endif
> +
> +#endif /* !HAVE_SYSLOG_H */
> diff --git a/common/replacements/realpath.c b/common/replacements/realpath.c
> new file mode 100644
> index 00000000..622ee7ea
> --- /dev/null
> +++ b/common/replacements/realpath.c

> +++ b/common/replacements/strndup.c

> +
> +char *
> +strndup(const char *s, size_t n)
> +{
> +  size_t len = strlen (s);

You _must_ use strnlen here.  Otherwise, a user that passes in a pointer 
to bytes that are bounded by a memory page transition rather than a NUL 
can cause you to SEGV reading beyond n bytes.

> +  char *ret;
> +
> +  if (len > n)
> +    len = n;

and once you've used strnlen, this is dead.

> +
> +  ret = malloc (len+1);
> +  if (ret == NULL)
> +    return NULL;
> +  memcpy (ret, s, len);
> +  ret[len] = '\0';
> +
> +  return ret;
> +}
> +
> +#endif /* !HAVE_STRNDUP */
> diff --git a/common/replacements/syslog.c b/common/replacements/syslog.c
> new file mode 100644
> index 00000000..6f41cbd8
> --- /dev/null
> +++ b/common/replacements/syslog.c

> +++ b/common/replacements/vsyslog.c

> +++ b/common/replacements/win32/nbdkit-cat.mc
> @@ -0,0 +1,6 @@
> +MessageId=1
> +Severity=Error
> +SymbolicName=NBDKIT_SYSLOG_ERROR
> +Language=English
> +%1
> +.

Does this file not allow for a copyright blurb?  Or is it small enough 
to be trivial and not need one?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org