Re: [PATCH v2 2/2] block: deprecate the sheepdog block driver

[Markus Armbruster <armbru@redhat.com>]

Daniel P. Berrangé <berrange@redhat.com> writes:

> This thread from a little over a year ago:
>
>   http://lists.wpkg.org/pipermail/sheepdog/2019-March/thread.html
>
> states that sheepdog is no longer actively developed. The only mentioned
> users are some companies who are said to have it for legacy reasons with
> plans to replace it by Ceph. There is talk about cutting out existing
> features to turn it into a simple demo of how to write a distributed
> block service. There is no evidence of anyone working on that idea:
>
>   https://github.com/sheepdog/sheepdog/commits/master
>
> No real commits to git since Jan 2018, and before then just some minor
> technical debt cleanup..

Drop the extra period.

>
> There is essentially no activity on the mailing list aside from
> patches to QEMU that get CC'd due to our MAINTAINERS entry.
>
> Fedora packages for sheepdog failed to build from upstream source
> because of the more strict linker that no longer merges duplicate
> global symbols. Fedora patches it to add the missing "extern"
> annotations and presumably other distros do to, but upstream source
> remains broken.
>
> There is only basic compile testing, no functional testing of the
> driver.
>
> Since there are no build pre-requisites the sheepdog driver is currently
> enabled unconditionally. This would result in configure issuing a
> deprecation warning by default for all users. Thus the configure default
> is changed to disable it, requiring users to pass --enable-sheepdog to
> build the driver.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  block/sheepdog.c           | 15 +++++++++++++++
>  configure                  |  5 +++--
>  docs/system/deprecated.rst |  9 +++++++++
>  3 files changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index cbbebc1aaf..7f68bd6a1a 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -242,6 +242,17 @@ typedef struct SheepdogInode {
>   */
>  #define FNV1A_64_INIT ((uint64_t)0xcbf29ce484222325ULL)
>  
> +static void deprecation_warning(void)
> +{
> +    static bool warned = false;

Obey checkpatch :)

> +
> +    if (!warned) {
> +        warn_report("the sheepdog block driver is deprecated and will be "
> +                    "removed in a future release");

Similar warnings elsewhere don't say "will be removed".

Some of them are nice enough to advise what to use instead, but that may
not be practical here.

> +        warned = true;
> +    }
> +}
> +
>  /*
>   * 64 bit Fowler/Noll/Vo FNV-1a hash code
>   */
> @@ -1548,6 +1559,8 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags,
>      char *buf = NULL;
>      QemuOpts *opts;
>  
> +    deprecation_warning();
> +
>      s->bs = bs;
>      s->aio_context = bdrv_get_aio_context(bs);
>  
> @@ -2007,6 +2020,8 @@ static int sd_co_create(BlockdevCreateOptions *options, Error **errp)
>  
>      assert(options->driver == BLOCKDEV_DRIVER_SHEEPDOG);
>  
> +    deprecation_warning();
> +
>      s = g_new0(BDRVSheepdogState, 1);
>  
>      /* Steal SocketAddress from QAPI, set NULL to prevent double free */
> diff --git a/configure b/configure
> index 7564479008..c6af83f2e6 100755
> --- a/configure
> +++ b/configure
> @@ -533,7 +533,7 @@ vdi="yes"
>  vvfat="yes"
>  qed="yes"
>  parallels="yes"
> -sheepdog="yes"
> +sheepdog="no"
>  libxml2=""
>  debug_mutex="no"
>  libpmem=""
> @@ -1941,7 +1941,7 @@ disabled with --disable-FEATURE, default is enabled if available:
>    vvfat           vvfat image format support
>    qed             qed image format support
>    parallels       parallels image format support
> -  sheepdog        sheepdog block driver support
> +  sheepdog        sheepdog block driver support (deprecated)
>    crypto-afalg    Linux AF_ALG crypto backend driver
>    capstone        capstone disassembler support
>    debug-mutex     mutex debugging support
> @@ -7350,6 +7350,7 @@ if test "$parallels" = "yes" ; then
>    echo "CONFIG_PARALLELS=y" >> $config_host_mak
>  fi
>  if test "$sheepdog" = "yes" ; then
> +  add_to deprecated_features "sheepdog"
>    echo "CONFIG_SHEEPDOG=y" >> $config_host_mak
>  fi
>  if test "$pty_h" = "yes" ; then
> diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
> index 0cb8b01424..49b9f4b02e 100644
> --- a/docs/system/deprecated.rst
> +++ b/docs/system/deprecated.rst
> @@ -405,6 +405,15 @@ The above, converted to the current supported format::
>  
>    json:{"file.driver":"rbd", "file.pool":"rbd", "file.image":"name"}
>  
> +``sheepdog`` driver (since 5.2.0)
> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> +
> +The ``sheepdog`` block device driver is deprecated. The corresponding upstream
> +server project is no longer actively maintained. Users are recommended to switch
> +to an alternative distributed block device driver such as RBD. The
> +``qemu-img convert`` command can be used to liberate existing data by moving
> +it out of sheepdog volumes into an alternative storage backend.
> +
>  linux-user mode CPUs
>  --------------------

Preferably with my nits addressed:
Reviewed-by: Markus Armbruster <armbru@redhat.com>