Re: [Qemu-devel] [RFC PATCH] configure: remove --enable-replication/--disable-replication

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Bruce Rogers (brogers@suse.com) wrote:
> >>> On 2/6/2017 at 4:57 AM, <dgilbert@redhat.com> wrote:
> > * Paolo Bonzini (pbonzini@redhat.com) wrote:
> >> 
> >> 
> >> On 03/02/2017 07:00, Stefan Hajnoczi wrote:
> >> > On Thu, Feb 02, 2017 at 07:05:30AM ‑0800, Paolo Bonzini wrote:
> >> >> The replication feature is a small amount of code, does not
> >> >> require any external library and unless used does not add
> >> >> anything to the guest's attack surface.  Since any extra
> >> >> configure option affects maintainability on the other hand
> >> >> and is subject to bit rot, I think there is no need to
> >> >> make it configurable.
> >> > 
> >> > I think the current state is good: replication is enabled by default but
> >> > can be compiled out if desired.
> >> > 
> >> > Downstreams may not be comfortable supporting this feature yet since
> >> > it's incomplete.  It's fair to offer an option to disable it, otherwise
> >> > downstreams will have to patch this themselves.
> >> 
> >> I understand‑‑‑I just am not sure where to draw the line because there's
> >> plenty of other incomplete features, hence the RFC.  For example,
> >> record/replay cannot be enabled or disabled on the configure command
> >> line.  That was the case even in the beginning, where it didn't support
> >> either block or character device replay.
> > 
> > The line is certainly fuzzy, but I think it's worth making the following
> > type of things configurable:
> >    Features that have a large chunk of code
> >      ‑ dont lets try and configure tiny things on and off
> >    That can be trivially configured
> >      ‑ lets not put big chunks of code around making them configurable
> > and   that are incomplete
> >    or are unused by large chunks of the users
> > 
> > Dave
> > 
> >> ‑‑enable‑coroutine‑pool is a relic of when Windows builds needed it, but
> >> all other ‑‑enable‑* options require an external library or at least a
> >> specific operating system.  See for example this patch:
> >> 
> >>     commit 52b53c04faab9f7a9879c8dc014930649a3e698d
> >>     Author: Fam Zheng <famz@redhat.com>
> >>     Date:   Wed Sep 10 14:17:51 2014 +0800
> >> 
> >>     block: Always compile virtio‑blk dataplane
> >> 
> >>     Dataplane doesn't depend on linux‑aio any more, so we don't need the
> >>     compiling condition now.
> >> 
> >>     Configure options are kept but just print a message.
> >> 
> >>     Signed‑off‑by: Fam Zheng <famz@redhat.com>
> >>     Reviewed‑by: Paolo Bonzini <pbonzini@redhat.com>
> >>     Message‑id: 1410329871‑28885‑4‑git‑send‑email‑famz@redhat.com 
> >>     Signed‑off‑by: Stefan Hajnoczi <stefanha@redhat.com>
> >> 
> >> 
> >> I would actually prefer to remove many of the latter
> >> (‑‑enable‑vhost‑net, ‑‑enable‑vhost‑scsi, ‑‑enable‑vhost‑socket) and
> >> just use default‑configs.  We are already doing it for ivshmem for example:
> >> 
> >>     CONFIG_IVSHMEM=$(CONFIG_EVENTFD)
> >> 
> >> Paolo
> > ‑‑
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 
> Was there ever a conclusion here? The reason I ask is that I see that currently
> using --disable-replication fails for me as follows:
> 
> # ./configure --disable-replication
> ...
> # make
> ...
> make  all-recursive
> Making all in pixman
> make[3]: Nothing to be done for 'all'.
> Making all in demos
> make[3]: Nothing to be done for 'all'.
> Making all in test
> make[3]: Nothing to be done for 'all'.
> 	CHK version_gen.h
>   LINK    aarch64-softmmu/qemu-system-aarch64
> ../migration/colo.o: In function `qmp_query_xen_replication_status':
> /home/brogers/osr/git/qemu/migration/colo.c:181: undefined reference to `replication_get_error_all'
> ../migration/colo.o: In function `qmp_xen_set_replication':
> /home/brogers/osr/git/qemu/migration/colo.c:172: undefined reference to `replication_stop_all'
> /home/brogers/osr/git/qemu/migration/colo.c:172: undefined reference to `replication_stop_all'
> /home/brogers/osr/git/qemu/migration/colo.c:167: undefined reference to `replication_start_all'
> ../migration/colo.o: In function `qmp_xen_colo_do_checkpoint':
> /home/brogers/osr/git/qemu/migration/colo.c:196: undefined reference to `replication_do_checkpoint_all'
> collect2: error: ld returned 1 exit status
> make[1]: *** [Makefile:208: qemu-system-aarch64] Error 1
> make: *** [Makefile:322: subdir-aarch64-softmmu] Error 2

We should fix that.

Dave

> -- 
> Bruce
>  
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK