Re: [Qemu-devel] [PATCH] RFC: build-sys: move slirp as git submodule project

[Eric Blake]

On 3/25/19 1:04 PM, Marc-André Lureau wrote:
> The slirp project is now hosted on freedesktop at:
> https://gitlab.freedesktop.org/slirp.
> 
> The libslirp source tree there is based on current qemu/slirp filtered
> through clang-format, and can thus be directly used through a
> git submodule.

Indeed, doing 'diff -ur slirp /path/to/libslirp.git' shows a lot of
formatting differences. Is there an easy command to see that the
contents of the two are identical modulo the formatting changes?

> 
> RFC: is this acceptable during freeze? (not a feature or code change)

It's a big enough change that I'd personally feel more comfortable with
doing it in 4.1, but I'm not opposed if others want it in 4.0.

> 
> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
>  slirp/src/bootp.h      |  129 ----
>  slirp/src/debug.h      |   46 --
>  slirp/src/dhcpv6.h     |   53 --
>  slirp/src/if.h         |   21 -
>  slirp/src/ip.h         |  242 -------
>  slirp/src/ip6.h        |  160 -----
>  slirp/src/ip6_icmp.h   |  232 ------
>  slirp/src/ip_icmp.h    |  166 -----
>  slirp/src/libslirp.h   |  118 ---
>  slirp/src/main.h       |   16 -
>  slirp/src/mbuf.h       |  127 ----
>  slirp/src/misc.h       |   66 --
>  slirp/src/ncsi-pkt.h   |  445 ------------
>  slirp/src/qtailq.h     |  194 -----
>  slirp/src/sbuf.h       |   27 -
>  slirp/src/slirp.h      |  275 -------
>  slirp/src/socket.h     |  160 -----
>  slirp/src/stream.h     |   35 -
>  slirp/src/tcp.h        |  181 -----
>  slirp/src/tcp_timer.h  |  128 ----
>  slirp/src/tcp_var.h    |  162 -----
>  slirp/src/tcpip.h      |  102 ---
>  slirp/src/tftp.h       |   52 --
>  slirp/src/udp.h        |   92 ---
>  slirp/src/util.h       |  175 -----
>  slirp/src/vmstate.h    |  409 -----------
>  slirp/src/arp_table.c  |   92 ---
>  slirp/src/bootp.c      |  371 ----------
>  slirp/src/cksum.c      |  161 -----
>  slirp/src/dhcpv6.c     |  224 ------
>  slirp/src/dnssearch.c  |  311 --------
>  slirp/src/if.c         |  218 ------
>  slirp/src/ip6_icmp.c   |  438 -----------
>  slirp/src/ip6_input.c  |   78 --
>  slirp/src/ip6_output.c |   39 -
>  slirp/src/ip_icmp.c    |  470 ------------
>  slirp/src/ip_input.c   |  469 ------------
>  slirp/src/ip_output.c  |  170 -----
>  slirp/src/mbuf.c       |  235 ------
>  slirp/src/misc.c       |  321 ---------
>  slirp/src/ncsi.c       |  194 -----
>  slirp/src/ndp_table.c  |   87 ---
>  slirp/src/sbuf.c       |  186 -----
>  slirp/src/slirp.c      | 1118 -----------------------------
>  slirp/src/socket.c     |  942 ------------------------
>  slirp/src/state.c      |  388 ----------
>  slirp/src/stream.c     |  120 ----
>  slirp/src/tcp_input.c  | 1554 ----------------------------------------
>  slirp/src/tcp_output.c |  522 --------------
>  slirp/src/tcp_subr.c   |  987 -------------------------
>  slirp/src/tcp_timer.c  |  294 --------
>  slirp/src/tftp.c       |  463 ------------
>  slirp/src/udp.c        |  363 ----------
>  slirp/src/udp6.c       |  173 -----
>  slirp/src/util.c       |  368 ----------
>  slirp/src/vmstate.c    |  441 ------------
>  .gitmodules            |    3 +
>  configure              |   11 +-
>  slirp                  |    1 +
>  slirp/COPYRIGHT        |   62 --
>  slirp/Makefile         |   47 --

Hmm - no use of git's orderfile directive to float the additions first
into your patch?  The deletions are fluff in comparison.


> +++ b/.gitmodules
> @@ -52,3 +52,6 @@
>  [submodule "roms/edk2"]
>       path = roms/edk2
>       url = https://github.com/tianocore/edk2.git
> +[submodule "slirp"]
> +     path = slirp
> +     url = https://gitlab.freedesktop.org/slirp/libslirp.git

Do we also need to get qemu.org to start mirroring this?

> diff --git a/configure b/configure
> index 1c563a7027..a6aaf1276d 100755
> --- a/configure
> +++ b/configure
> @@ -1120,6 +1120,8 @@ for opt do
>    ;;
>    --disable-slirp) slirp="no"
>    ;;
> +  --enable-slirp=git) slirp="git"
> +  ;;
>    --enable-slirp=system) slirp="system"
>    ;;
>    --disable-vde) vde="no"
> @@ -5891,6 +5893,8 @@ case "$slirp" in
>    "" | yes)
>      if $pkg_config slirp; then
>        slirp=system
> +    elif test -e "${source_path}/.git" && test $git_update = 'yes' ; then
> +      slirp=git
>      elif test -e "${source_path}/slirp/Makefile" ; then
>        slirp=internal
>      elif test -z "$slirp" ; then
> @@ -5908,7 +5912,10 @@ case "$slirp" in
>  esac
>  
>  case "$slirp" in
> -  internal)
> +  git | internal)
> +    if test "$slirp" = git; then
> +      git_submodules="${git_submodules} slirp"
> +    fi
>      mkdir -p slirp
>      slirp_cflags="-I\$(SRC_PATH)/slirp/src -I\$(BUILD_DIR)/slirp/src"
>      slirp_libs="-L\$(BUILD_DIR)/slirp -lslirp"
> @@ -6571,7 +6578,7 @@ if test "$slirp" != "no"; then
>    echo "SLIRP_CFLAGS=$slirp_cflags" >> $config_host_mak
>    echo "SLIRP_LIBS=$slirp_libs" >> $config_host_mak
>  fi
> -if [ "$slirp" = "internal" ]; then
> +if [ "$slirp" = "git" -o "$slirp" = "internal" ]; then
>      echo "config-host.h: subdir-slirp" >> $config_host_mak
>  fi
>  if test "$vde" = "yes" ; then
> diff --git a/slirp b/slirp
> new file mode 160000
> index 0000000000..49b04bc2e0
> --- /dev/null
> +++ b/slirp
> @@ -0,0 +1 @@
> +Subproject commit 49b04bc2e092611ed87f3e2122a67c67a9a109ba

Overall, the patch looks sane, although I was unable to quickly verify
if libslirp 49b04bc2 is identical in semantics to what you deleted, so
I'll withhold R-b for now while waiting for other reviewers to chime in.

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

signature.asc
Description: OpenPGP digital signature