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

[Richard W.M. Jones]

On Tue, Aug 18, 2020 at 08:20:44AM -0500, Eric Blake wrote:
> 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.

Yes - I think so (see below for more).

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

This code was actually written in 2019, but yes I do need
to update this lot :-)

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

I honestly cannot remember now how all this worked, except that it was
apparently necessary for something - syslog support I think?  I'll try
and investigate what it was used for.

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

That's what realpath(3) says.  However I think it might be a mistake
in the man page, or it could be because the man page refers to
PATH_MAX which is defined in <limits.h>.  POSIX says only <stdlib.h>
is necessary.

> >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 ;)

Huh, and I thought include_next was part of C ...

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

I believe so, but I wrote this 18 months ago and can't remember the
details.

Re Cygwin: I wonder if we actually care about Cygwin at all.  It's a
sufficiently different target that we would probably consider it to be
a new platform.  But if anyone had even a partially working Windows
port using the native APIs would they want a Cygwin port?  Maybe
the way to go is to remove |msys*|cygwin* from configure.ac.

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

Apparently it does support a comment syntax.  It's "unique":

https://docs.microsoft.com/en-us/windows/win32/eventlog/message-text-files

I'll take a look at all the other stuff you mentioned too, thanks.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top