On 2023/08/20 14:28:55 +0200, Stefan Sperling <stsp@stsp.name> wrote:
> The following patches provide some progress for gotd in -portable.
> The patches are based on the ta/gotd branch, not the 'portable' branch!
> 
> I am restoring chroot support where possible. This requires the main
> process to keep running as root. But that is probably better than not
> chrooting.
> 
> A blocking issue I found during testing is that 'got send' never
> exits after uploading the pack file. On the server side I see gotsh
> hanging in poll on stdin, still waiting for more pack file data to
> arrive. Any ideas how that can be fixed? We seem to be relying on
> some OpenBSD-specific behaviour on the server side?
> 
> To test the changes, add a gotd user to the system, create an empty
> directory at /var/run/gotd, put gotsh and gitwrapper in /usr/bin
> with appropriate symlinks for gitwrapper.
> 
> Try a gotd.conf such as:
> [[[
> user gotd
> 
> repository "test" {
> 	path "/git/test.git"
> 	permit rw stsp
> }
> ]]]
> 
> I don't want to commit the debug diff (#4). OK for any others?

it reads fine to me; haven't been able to test yet.

> Should I put patches on ta/gotd or do we create a separate branch?

(no comment on this)

> Diffs #1 and #2 could probably go directly to the 'portable' branch?

IMHO yes.

> -----------------------------------------------
> commit fdd7ffc98a9c9a26d83d115d398c1cd1e609e7cd
> from: Stefan Sperling <stsp@stsp.name>
> date: Sun Aug 20 10:32:34 2023 UTC
>  
>  make sure program_invocation_short_name gets defined in getprogname.c
>   
>  This is needed for gitwrapper which will fail if its program name
>  cannot be detected properly.
>  
> diff 497c206b2e1425e9a667723222a167d4f3b1dae6 fdd7ffc98a9c9a26d83d115d398c1cd1e609e7cd
> commit - 497c206b2e1425e9a667723222a167d4f3b1dae6
> commit + fdd7ffc98a9c9a26d83d115d398c1cd1e609e7cd
> blob - 1dc2f493577b073fa65b7544928ff6ff61c1c2e6
> blob + 975dd4f84e4975bf7c53aaa058770a333a96cfd4
> --- compat/getprogname.c
> +++ compat/getprogname.c
> @@ -14,10 +14,15 @@
>   * OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
>   */
>  
> +#define _GNU_SOURCE
> +
>  #include <sys/types.h>
>  
>  #include <errno.h>
> +#include <argp.h>
>  
> +#include "got_compat.h"
> +

I'd avoid explicitly defining _GNU_SOURCE and instead move the include
of got_compat.h so it's the first header.  we're already using
AC_USE_SYSTEM_EXTENSIONS which defines _GNU_SOURCE at the top of
got_compat.h on GNU systems, so we're already covered.

(We have many files that don't include got_compat.h and not all have
got_compat.h at the very start, but this is a separate issue.)

also, is argp.h standard?  Don't see it on OpenBSD.  We should check
its presence in configure.ac (add an entry to the AC_CHECK_HEADERS
macro) and guard the include with #ifdef HAVE_ARGP_H or something
along these lines.

The manpage of program_invocation_short_name(3) on GNU/Linux only
lists errno.h as required header moreover.

>  #if defined(HAVE_PROGRAM_INVOCATION_SHORT_NAME)
>  const char *
>  getprogname(void)
> 
> -----------------------------------------------
> commit 13c8c4fddb5a359fdfb781028cfaa3529ae65043
> from: Stefan Sperling <stsp@stsp.name>
> date: Sun Aug 20 10:32:34 2023 UTC
>  
>  use a workng git libexec directory default on Linux
>  
>  Tested on ubuntu 22.04
>  
> diff fdd7ffc98a9c9a26d83d115d398c1cd1e609e7cd 13c8c4fddb5a359fdfb781028cfaa3529ae65043
> commit - fdd7ffc98a9c9a26d83d115d398c1cd1e609e7cd
> commit + 13c8c4fddb5a359fdfb781028cfaa3529ae65043
> blob - 2b1255118b91d4c7edef0a6016130ed9de07bfcd
> blob + af3cf15215d61e94a762d704ddca0ecf37080994
> --- gitwrapper/gitwrapper.c
> +++ gitwrapper/gitwrapper.c
> @@ -43,8 +43,12 @@
>  #include "log.h"
>  
>  #ifndef GITWRAPPER_GIT_LIBEXEC_DIR
> +#ifdef __linux__
> +#define GITWRAPPER_GIT_LIBEXEC_DIR "/usr/lib/git-core/"
> +#else
>  #define GITWRAPPER_GIT_LIBEXEC_DIR "/usr/local/libexec/git"
>  #endif
> +#endif

not to block progress but these should also not be hardcoded because

 1. it's ugly to #ifdef __linux__; this seems to affect other systems
    as well

 2. different systems/distro install different things in different
    places.  (I won't be surprised if DragonflyBSD has git in /usr/bin
    for example.)

so this could be fine for a first stab, but would then need some work
to allow changing these paths at configure-time (at least.)

One way would be to detect at configure time where the git libexecs
are installed and provide a --with flag to override the automatic
guessing game.

>  #ifndef GITWRAPPER_MY_SERVER_PROG
>  #define GITWRAPPER_MY_SERVER_PROG "gotsh"
> 
> -----------------------------------------------
> commit bf9a5775e4bca9539f7e1064a16fe20f611028ef
> from: Stefan Sperling <stsp@stsp.name>
> date: Sun Aug 20 10:32:34 2023 UTC
>  
>  re-enable chroot in -portable gotd
>  
>  Reads (git clone) are working but writes (git push) run into an error
>  
> diff 13c8c4fddb5a359fdfb781028cfaa3529ae65043 bf9a5775e4bca9539f7e1064a16fe20f611028ef
> commit - 13c8c4fddb5a359fdfb781028cfaa3529ae65043
> commit + bf9a5775e4bca9539f7e1064a16fe20f611028ef
> blob - d847e923a6c4676e526a8171f6813cb9686f9698
> blob + 4fd361f420a38ab5a9dfe6a68516b93b5d1f9d2c
> [...]
> --- gotd/gotd.h
> +++ gotd/gotd.h
> @@ -20,7 +20,11 @@
>  #define GOTD_UNIX_SOCKET_BACKLOG 10
>  #define GOTD_USER	"_gotd"
>  #define GOTD_CONF_PATH	"/etc/gotd.conf"
> +#ifdef __linux__
> +#define GOTD_EMPTY_PATH	"/var/run/gotd"
> +#else
>  #define GOTD_EMPTY_PATH	"/var/empty"
> +#endif

same thing here, these should be changeable from the configure script
IMHO.  It's fine for me if you want to land these as-is, I can add the
autoconf bits later (assuming thomas agrees.)

>  #define GOTD_MAXCLIENTS		1024
>  #define GOTD_MAX_CONN_PER_UID	4
> 
[...]