Re: [PATCH v19 03/15] net/tap: deprecate "no" as special value for script/downscript

[Markus Armbruster <armbru@redhat.com>]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:

> The interface is ambiguous, as "no" is valid file name. So,
> using "no" as a special value to disable script is deprecated.
> Use an empty string ("script=" / "downscript=") instead.
>
> In a future version, "no" will be treated as a plain file name, just
> like any other non-empty value.
>
> Document the deprecation in docs/about/deprecated.rst, qapi/net.json,
> and qemu-options.hx. Update other docs to use empty string instead of
> "no". Add a warning.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  docs/about/deprecated.rst                  | 18 ++++++++++++++
>  docs/system/i386/microvm.rst               |  4 +--
>  docs/system/i386/xenpvh.rst                |  2 +-
>  docs/system/ppc/ppce500.rst                |  4 +--
>  docs/system/riscv/microchip-icicle-kit.rst |  2 +-
>  docs/system/riscv/sifive_u.rst             |  2 +-
>  net/tap.c                                  | 29 ++++++++++++++--------
>  qapi/net.json                              | 12 ++++++---
>  qemu-options.hx                            |  7 ++++--
>  9 files changed, 56 insertions(+), 24 deletions(-)
>
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index 0c656a968fc..c4929317e3a 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -71,6 +71,15 @@ flexible enough. The monitor objects have been converted to QOM, so
>  ``-mon mode=control`` is replaced by ``-object monitor-qmp``. The
>  short convenience options are not deprecated, only ``-mon``.
>  
> +``script=no`` and ``downscript=no`` for ``-netdev tap`` (since 11.2)
> +'''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
> +
> +The special value ``"no"`` for the ``script`` and ``downscript``
> +parameters of ``-netdev tap`` disables script execution.  This special
> +treatment of ``"no"`` is deprecated.  Use an empty string (``script=``
> +or ``downscript=``) to disable script execution instead.  In a future
> +version, ``"no"`` will be treated as a plain file name.
> +
>  QEMU Machine Protocol (QMP) commands
>  ------------------------------------
>  
> @@ -164,6 +173,15 @@ Use ``job-finalize`` instead.
>  
>  Use ``query-accelerators`` instead.
>  
> +``"no"`` as value of ``script``/``downscript`` for tap in ``netdev_add`` (since 11.2)
> +'''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
> +
> +The special value ``"no"`` for the ``script`` and ``downscript``
> +parameters of ``netdev_add`` with ``type=tap`` disables script
> +execution.  This special treatment of ``"no"`` is deprecated.  Use an
> +empty string instead.  In a future version, ``"no"`` will be treated as
> +a plain file name.
> +
>  Human Machine Protocol (HMP) commands
>  -------------------------------------
>  
> diff --git a/docs/system/i386/microvm.rst b/docs/system/i386/microvm.rst
> index 1675e37d3e7..077ea15751e 100644
> --- a/docs/system/i386/microvm.rst
> +++ b/docs/system/i386/microvm.rst
> @@ -79,7 +79,7 @@ legacy ``ISA serial`` device as console::
>       -serial stdio \
>       -drive id=test,file=test.img,format=raw,if=none \
>       -device virtio-blk-device,drive=test \
> -     -netdev tap,id=tap0,script=no,downscript=no \
> +     -netdev tap,id=tap0,script=,downscript= \
>       -device virtio-net-device,netdev=tap0
>  
>  While the example above works, you might be interested in reducing the
> @@ -103,7 +103,7 @@ disabled::
>       -device virtconsole,chardev=virtiocon0 \
>       -drive id=test,file=test.img,format=raw,if=none \
>       -device virtio-blk-device,drive=test \
> -     -netdev tap,id=tap0,script=no,downscript=no \
> +     -netdev tap,id=tap0,script=,downscript= \
>       -device virtio-net-device,netdev=tap0
>  
>  
> diff --git a/docs/system/i386/xenpvh.rst b/docs/system/i386/xenpvh.rst
> index 904778e3f5c..862f38830b1 100644
> --- a/docs/system/i386/xenpvh.rst
> +++ b/docs/system/i386/xenpvh.rst
> @@ -42,7 +42,7 @@ case you need to construct one manually:
>        -vnc none                                       \
>        -display none                                   \
>        -device virtio-net-pci,id=nic0,netdev=net0,mac=00:16:3e:5c:81:78 \
> -      -netdev type=tap,id=net0,ifname=vif3.0-emu,br=xenbr0,script=no,downscript=no \
> +      -netdev type=tap,id=net0,ifname=vif3.0-emu,br=xenbr0,script=,downscript= \
>        -smp 4,maxcpus=4                                \
>        -nographic                                      \
>        -machine xenpvh,ram-low-base=0,ram-low-size=2147483648,ram-high-base=4294967296,ram-high-size=2147483648,pci-ecam-base=824633720832,pci-ecam-size=268435456,pci-mmio-base=4026531840,pci-mmio-size=33554432,pci-mmio-high-base=824902156288,pci-mmio-high-size=68719476736 \
> diff --git a/docs/system/ppc/ppce500.rst b/docs/system/ppc/ppce500.rst
> index c9fe0915dc5..ec5aaf14fd9 100644
> --- a/docs/system/ppc/ppce500.rst
> +++ b/docs/system/ppc/ppce500.rst
> @@ -158,14 +158,14 @@ interface at PCI address 0.1.0, but we can switch that to an e1000 NIC by:
>    $ qemu-system-ppc64 -M ppce500 -smp 4 -m 2G \
>                        -display none -serial stdio \
>                        -bios u-boot \
> -                      -nic tap,ifname=tap0,script=no,downscript=no,model=e1000
> +                      -nic tap,ifname=tap0,script=,downscript=,model=e1000
>  
>  The QEMU ``ppce500`` machine can also dynamically instantiate an eTSEC device
>  if “-device eTSEC” is given to QEMU:
>  
>  .. code-block:: bash
>  
> -  -netdev tap,ifname=tap0,script=no,downscript=no,id=net0 -device eTSEC,netdev=net0
> +  -netdev tap,ifname=tap0,script=,downscript=,id=net0 -device eTSEC,netdev=net0
>  
>  Root file system on flash drive
>  -------------------------------
> diff --git a/docs/system/riscv/microchip-icicle-kit.rst b/docs/system/riscv/microchip-icicle-kit.rst
> index 9809e94b84b..7fdb96601ad 100644
> --- a/docs/system/riscv/microchip-icicle-kit.rst
> +++ b/docs/system/riscv/microchip-icicle-kit.rst
> @@ -84,7 +84,7 @@ Then we can boot the machine by:
>    $ qemu-system-riscv64 -M microchip-icicle-kit -smp 5 -m 2G \
>        -sd path/to/sdcard.img \
>        -nic user,model=cadence_gem \
> -      -nic tap,ifname=tap,model=cadence_gem,script=no \
> +      -nic tap,ifname=tap,model=cadence_gem,script= \
>        -display none -serial stdio \
>        -kernel path/to/u-boot/build/dir/u-boot.bin \
>        -dtb path/to/u-boot/build/dir/u-boot.dtb
> diff --git a/docs/system/riscv/sifive_u.rst b/docs/system/riscv/sifive_u.rst
> index 8f55ae8e313..0e4dcf3e70c 100644
> --- a/docs/system/riscv/sifive_u.rst
> +++ b/docs/system/riscv/sifive_u.rst
> @@ -199,7 +199,7 @@ To boot the VxWorks kernel in QEMU with the ``sifive_u`` machine, use:
>  
>    $ qemu-system-riscv64 -M sifive_u -smp 5 -m 2G \
>        -display none -serial stdio \
> -      -nic tap,ifname=tap0,script=no,downscript=no \
> +      -nic tap,ifname=tap0,script=,downscript= \
>        -kernel /path/to/vxWorks \
>        -append "gem(0,0)host:vxWorks h=192.168.200.1 e=192.168.200.2:ffffff00 u=target pw=vxTarget f=0x01"
>  
> diff --git a/net/tap.c b/net/tap.c
> index fedd48c48d2..2e94b4015f6 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -92,34 +92,40 @@ static void launch_script(const char *setup_script, const char *ifname,
>  static void tap_send(void *opaque);
>  static void tap_writable(void *opaque);
>  
> -static bool tap_is_explicit_no_script(const char *script_arg)
> +static bool tap_is_explicit_no_script(const char *script_arg_name,
> +                                      const char *script_arg_value)
>  {
> -    if (!script_arg) {
> +    if (!script_arg_value) {
>          return false;
>      }
>  
> -    if (script_arg[0] == '\0') {
> +    if (script_arg_value[0] == '\0') {
>          return true;
>      }
>  
> -    if (strcmp(script_arg, "no") == 0) {
> +    if (strcmp(script_arg_value, "no") == 0) {
> +        warn_report("%s=no is deprecated; use %s= instead "

Maybe "'%s=no' is deprecated; use '%s=' instead ".

> +                    "(empty string instead of 'no')",

Not sure this is needed.  

> +                    script_arg_name, script_arg_name);
>          return true;
>      }
>  
>      return false;
>  }

Less churn if you name the argument @script_arg_value in PATCH 01.

>  
> -static char *tap_parse_script(const char *script_arg, const char *default_path)
> +static char *tap_parse_script(const char *script_arg_name,
> +                              const char *script_arg_value,
> +                              const char *default_path)
>  {
> -    if (tap_is_explicit_no_script(script_arg)) {
> +    if (tap_is_explicit_no_script(script_arg_name, script_arg_value)) {
>          return NULL;
>      }
>  
> -    if (!script_arg) {
> +    if (!script_arg_value) {
>          return get_relocated_path(default_path);
>      }
>  
> -    return g_strdup(script_arg);
> +    return g_strdup(script_arg_value);
>  }
>  
>  static void tap_update_fd_handler(TAPState *s)
> @@ -740,7 +746,7 @@ static bool net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
>          qemu_set_info_str(&s->nc, "helper=%s", tap->helper);
>      } else {
>          qemu_set_info_str(&s->nc, "ifname=%s,script=%s,downscript=%s", ifname,
> -                          script ?: "no", downscript ?: "no");
> +                          script ?: "", downscript ?: "");
>  
>          if (downscript) {
>              snprintf(s->down_script, sizeof(s->down_script), "%s", downscript);
> @@ -946,9 +952,10 @@ int net_init_tap(const Netdev *netdev, const char *name,
>          }
>      } else {
>          g_autofree char *script =
> -            tap_parse_script(tap->script, DEFAULT_NETWORK_SCRIPT);
> +            tap_parse_script("script", tap->script, DEFAULT_NETWORK_SCRIPT);
>          g_autofree char *downscript =
> -            tap_parse_script(tap->downscript, DEFAULT_NETWORK_DOWN_SCRIPT);
> +            tap_parse_script("downscript", tap->downscript,
> +                             DEFAULT_NETWORK_DOWN_SCRIPT);
>  
>          if (tap->ifname) {
>              pstrcpy(ifname, sizeof ifname, tap->ifname);
> diff --git a/qapi/net.json b/qapi/net.json
> index 8f0915c4d86..ada0329ef9d 100644
> --- a/qapi/net.json
> +++ b/qapi/net.json
> @@ -399,15 +399,19 @@
>  # @fds: multiple file descriptors of already opened multiqueue capable
>  #     tap
>  #
> -# @script: script to initialize the interface.  An empty string or
> -#     "no" disables script execution.  Defaults to
> +# @script: script to initialize the interface.  An empty string
> +#     disables script execution.  Defaults to
>  #     ``<sysconfdir>/qemu-ifup``, where ``<sysconfdir>`` is the
>  #     system configuration directory at build time (typically /etc).
> +#     Using "no" to disable script execution is deprecated (since
> +#     11.2); use an empty string instead.
>  #
> -# @downscript: script to shut down the interface.  An empty string or
> -#     "no" disables script execution.  Defaults to
> +# @downscript: script to shut down the interface.  An empty string
> +#     disables script execution.  Defaults to
>  #     ``<sysconfdir>/qemu-ifdown``, where ``<sysconfdir>`` is the
>  #     system configuration directory at build time (typically /etc).
> +#     Using "no" to disable script execution is deprecated (since
> +#     11.2); use an empty string instead.
>  #
>  # @br: bridge name (since 2.8)
>  #
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 200949655ea..7b9a14f5b38 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -3014,7 +3014,8 @@ DEF("netdev", HAS_ARG, QEMU_OPTION_netdev,
>      "                use network scripts 'file' (default=" DEFAULT_NETWORK_SCRIPT ")\n"
>      "                to configure it and 'dfile' (default=" DEFAULT_NETWORK_DOWN_SCRIPT ")\n"
>      "                to deconfigure it\n"
> -    "                use '[down]script=no' or '[down]script=' to disable script execution\n"
> +    "                use '[down]script=' to disable script execution\n"
> +    "                ('[down]script=no' is deprecated and will be treated as a file name in future)\n"
>      "                use network helper 'helper' (default=" DEFAULT_BRIDGE_HELPER ") to\n"
>      "                configure it\n"
>      "                use 'fd=h' to connect to an already opened TAP interface\n"
> @@ -3553,7 +3554,9 @@ SRST
>      ``<sysconfdir>/qemu-ifup`` and the default network deconfigure script is
>      ``<sysconfdir>/qemu-ifdown``, where ``<sysconfdir>`` is the system
>      configuration directory at build time (typically ``/etc``).
> -    Use ``[down]script=no`` or ``[down]script=`` to disable script execution.
> +    Use ``[down]script=`` to disable script execution.
> +    Using ``[down]script=no`` is deprecated; in a future version it will
> +    be treated as a plain file name.
>  
>      If running QEMU as an unprivileged user, use the network helper
>      to configure the TAP interface and attach it to the bridge.

You mention what "no" does only in QMP documentation.

You mention what "no" will do in a future version only in CLI
documentation.

If information is useful enough to justify the space in one place, it's
likely useful enough in the other place, too.

Up to you.

Reviewed-by: Markus Armbruster <armbru@redhat.com>