In early times, network backends were specified by a "vlan" and "name"
tuple. With the introduction of netdevs, the "name" was replaced by an
"id" (which is supposed to be unique), but the "name" parameter stayed
as an alias which could be used instead of "id". Unfortunately, we miss
the duplication check for "name":

 $ qemu-system-x86_64 -net user,name=n1 -net user,name=n1

... starts without an error, while "id" correctly complains:

 $ qemu-system-x86_64 -net user,id=n1 -net user,id=n1
 qemu-system-x86_64: -net user,id=n1: Duplicate ID 'n1' for net

Instead of trying to fix the code for the legacy "name" parameter, let's
rather get rid of this old interface and deprecate the "name" parameter
now - this will also be less confusing for the users in the long run.

While we're at it, also deprecate the old syntax for the hostfwd_add
and hostfwd_remove commands that still work with this legacy "hub"
plus "name" tuple. It is enough to specify the netdev id there instead.

Also add a missing dependency to the Makefile to make sure that the
docs get correctly regenerated when qemu-deprecated.texi is changed.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 Makefile             |  2 +-
 net/net.c            |  4 ++++
 net/slirp.c          |  2 ++
 qemu-deprecated.texi | 13 +++++++++++++
 4 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index fe623e4..f42e176 100644
--- a/Makefile
+++ b/Makefile
@@ -978,7 +978,7 @@ txt: qemu-doc.txt docs/interop/qemu-qmp-ref.txt docs/interop/qemu-ga-ref.txt
 
 qemu-doc.html qemu-doc.info qemu-doc.pdf qemu-doc.txt: \
 	qemu-img.texi qemu-nbd.texi qemu-options.texi qemu-option-trace.texi \
-	qemu-monitor.texi qemu-img-cmds.texi qemu-ga.texi \
+	qemu-deprecated.texi qemu-monitor.texi qemu-img-cmds.texi qemu-ga.texi \
 	qemu-monitor-info.texi docs/qemu-block-drivers.texi \
 	docs/qemu-cpu-models.texi
 
diff --git a/net/net.c b/net/net.c
index 2a31339..cdcd5cf 100644
--- a/net/net.c
+++ b/net/net.c
@@ -984,6 +984,10 @@ static int net_client_init1(const void *object, bool is_netdev, Error **errp)
         /* missing optional values have been initialized to "all bits zero" */
         name = net->has_id ? net->id : net->name;
 
+        if (net->has_name) {
+            warn_report("The 'name' parameter is deprecated, use 'id' instead");
+        }
+
         /* Map the old options to the new flat type */
         switch (opts->type) {
         case NET_LEGACY_OPTIONS_TYPE_NONE:
diff --git a/net/slirp.c b/net/slirp.c
index c18060f..073bc69 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -404,6 +404,8 @@ static SlirpState *slirp_lookup(Monitor *mon, const char *hub_id,
                 monitor_printf(mon, "unrecognized (hub-id, stackname) pair\n");
                 return NULL;
             }
+            warn_report("Using 'hub-id' is deprecated. Specify the netdev id "
+                        "directly instead");
         } else {
             nc = qemu_find_netdev(name);
             if (!nc) {
diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 8a2e399..eed7132 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -83,6 +83,11 @@ The 'file' driver for drives is no longer appropriate for character or host
 devices and will only accept regular files (S_IFREG). The correct driver
 for these file types is 'host_cdrom' or 'host_device' as appropriate.
 
+@subsection -net ...,name=@var{name} (since 3.1)
+
+The @option{name} parameter of the @option{-net} option is an old synonym
+for the @option{id} parameter which should now be used instead.
+
 @section QEMU Machine Protocol (QMP) commands
 
 @subsection block-dirty-bitmap-add "autoload" parameter (since 2.12.0)
@@ -99,6 +104,14 @@ The ``query-cpus'' command is replaced by the ``query-cpus-fast'' command.
 The ``arch'' output member of the ``query-cpus-fast'' command is
 replaced by the ``target'' output member.
 
+@section System emulator human monitor commands
+
+@subsection The hub_id parameter of 'hostfwd_add' / 'hostfwd_remove' (since 3.1)
+
+The @option{hub_id} parameter of the 'hostfwd_add' and 'hostfwd_remove' HMP
+commands is redundant. It is enough to specify the netdev ID of the backend
+that should be changed.
+
 @section System emulator devices
 
 @subsection ivshmem (since 2.6.0)
-- 
1.8.3.1

On 9/19/18 10:12 PM, Thomas Huth wrote:
> In early times, network backends were specified by a "vlan" and "name"
> tuple. With the introduction of netdevs, the "name" was replaced by an
> "id" (which is supposed to be unique), but the "name" parameter stayed
> as an alias which could be used instead of "id". Unfortunately, we miss
> the duplication check for "name":
> 
>  $ qemu-system-x86_64 -net user,name=n1 -net user,name=n1
> 
> ... starts without an error, while "id" correctly complains:
> 
>  $ qemu-system-x86_64 -net user,id=n1 -net user,id=n1
>  qemu-system-x86_64: -net user,id=n1: Duplicate ID 'n1' for net
> 
> Instead of trying to fix the code for the legacy "name" parameter, let's
> rather get rid of this old interface and deprecate the "name" parameter
> now - this will also be less confusing for the users in the long run.
> 
> While we're at it, also deprecate the old syntax for the hostfwd_add
> and hostfwd_remove commands that still work with this legacy "hub"
> plus "name" tuple. It is enough to specify the netdev id there instead.
> 
> Also add a missing dependency to the Makefile to make sure that the
> docs get correctly regenerated when qemu-deprecated.texi is changed.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  Makefile             |  2 +-
>  net/net.c            |  4 ++++
>  net/slirp.c          |  2 ++
>  qemu-deprecated.texi | 13 +++++++++++++
>  4 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/Makefile b/Makefile
> index fe623e4..f42e176 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -978,7 +978,7 @@ txt: qemu-doc.txt docs/interop/qemu-qmp-ref.txt docs/interop/qemu-ga-ref.txt
>  
>  qemu-doc.html qemu-doc.info qemu-doc.pdf qemu-doc.txt: \
>  	qemu-img.texi qemu-nbd.texi qemu-options.texi qemu-option-trace.texi \
> -	qemu-monitor.texi qemu-img-cmds.texi qemu-ga.texi \
> +	qemu-deprecated.texi qemu-monitor.texi qemu-img-cmds.texi qemu-ga.texi \
>  	qemu-monitor-info.texi docs/qemu-block-drivers.texi \
>  	docs/qemu-cpu-models.texi
>  
> diff --git a/net/net.c b/net/net.c
> index 2a31339..cdcd5cf 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -984,6 +984,10 @@ static int net_client_init1(const void *object, bool is_netdev, Error **errp)
>          /* missing optional values have been initialized to "all bits zero" */
>          name = net->has_id ? net->id : net->name;
>  
> +        if (net->has_name) {
> +            warn_report("The 'name' parameter is deprecated, use 'id' instead");
> +        }
> +
>          /* Map the old options to the new flat type */
>          switch (opts->type) {
>          case NET_LEGACY_OPTIONS_TYPE_NONE:
> diff --git a/net/slirp.c b/net/slirp.c
> index c18060f..073bc69 100644
> --- a/net/slirp.c
> +++ b/net/slirp.c
> @@ -404,6 +404,8 @@ static SlirpState *slirp_lookup(Monitor *mon, const char *hub_id,
>                  monitor_printf(mon, "unrecognized (hub-id, stackname) pair\n");
>                  return NULL;
>              }
> +            warn_report("Using 'hub-id' is deprecated. Specify the netdev id "
> +                        "directly instead");
>          } else {
>              nc = qemu_find_netdev(name);
>              if (!nc) {
> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> index 8a2e399..eed7132 100644
> --- a/qemu-deprecated.texi
> +++ b/qemu-deprecated.texi
> @@ -83,6 +83,11 @@ The 'file' driver for drives is no longer appropriate for character or host
>  devices and will only accept regular files (S_IFREG). The correct driver
>  for these file types is 'host_cdrom' or 'host_device' as appropriate.
>  
> +@subsection -net ...,name=@var{name} (since 3.1)
> +
> +The @option{name} parameter of the @option{-net} option is an old synonym
> +for the @option{id} parameter which should now be used instead.
> +
>  @section QEMU Machine Protocol (QMP) commands
>  
>  @subsection block-dirty-bitmap-add "autoload" parameter (since 2.12.0)
> @@ -99,6 +104,14 @@ The ``query-cpus'' command is replaced by the ``query-cpus-fast'' command.
>  The ``arch'' output member of the ``query-cpus-fast'' command is
>  replaced by the ``target'' output member.
>  
> +@section System emulator human monitor commands
> +
> +@subsection The hub_id parameter of 'hostfwd_add' / 'hostfwd_remove' (since 3.1)
> +
> +The @option{hub_id} parameter of the 'hostfwd_add' and 'hostfwd_remove' HMP
> +commands is redundant. It is enough to specify the netdev ID of the backend
> +that should be changed.
> +
>  @section System emulator devices
>  
>  @subsection ivshmem (since 2.6.0)
> 

Thomas Huth <thuth@redhat.com> writes:

> In early times, network backends were specified by a "vlan" and "name"
> tuple. With the introduction of netdevs, the "name" was replaced by an
> "id" (which is supposed to be unique), but the "name" parameter stayed
> as an alias which could be used instead of "id". Unfortunately, we miss
> the duplication check for "name":
>
>  $ qemu-system-x86_64 -net user,name=n1 -net user,name=n1
>
> ... starts without an error, while "id" correctly complains:
>
>  $ qemu-system-x86_64 -net user,id=n1 -net user,id=n1
>  qemu-system-x86_64: -net user,id=n1: Duplicate ID 'n1' for net
>
> Instead of trying to fix the code for the legacy "name" parameter, let's
> rather get rid of this old interface and deprecate the "name" parameter
> now - this will also be less confusing for the users in the long run.
>
> While we're at it, also deprecate the old syntax for the hostfwd_add
> and hostfwd_remove commands that still work with this legacy "hub"
> plus "name" tuple. It is enough to specify the netdev id there instead.
>
> Also add a missing dependency to the Makefile to make sure that the
> docs get correctly regenerated when qemu-deprecated.texi is changed.

Sure sounds like three patches to me.

> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  Makefile             |  2 +-
>  net/net.c            |  4 ++++
>  net/slirp.c          |  2 ++
>  qemu-deprecated.texi | 13 +++++++++++++
>  4 files changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/Makefile b/Makefile
> index fe623e4..f42e176 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -978,7 +978,7 @@ txt: qemu-doc.txt docs/interop/qemu-qmp-ref.txt docs/interop/qemu-ga-ref.txt
>  
>  qemu-doc.html qemu-doc.info qemu-doc.pdf qemu-doc.txt: \
>  	qemu-img.texi qemu-nbd.texi qemu-options.texi qemu-option-trace.texi \
> -	qemu-monitor.texi qemu-img-cmds.texi qemu-ga.texi \
> +	qemu-deprecated.texi qemu-monitor.texi qemu-img-cmds.texi qemu-ga.texi \
>  	qemu-monitor-info.texi docs/qemu-block-drivers.texi \
>  	docs/qemu-cpu-models.texi
>  

Missed in commit 44c67847e32 by yours truly.

> diff --git a/net/net.c b/net/net.c
> index 2a31339..cdcd5cf 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -984,6 +984,10 @@ static int net_client_init1(const void *object, bool is_netdev, Error **errp)
>          /* missing optional values have been initialized to "all bits zero" */
>          name = net->has_id ? net->id : net->name;
>  
> +        if (net->has_name) {
> +            warn_report("The 'name' parameter is deprecated, use 'id' instead");
> +        }
> +
>          /* Map the old options to the new flat type */
>          switch (opts->type) {
>          case NET_LEGACY_OPTIONS_TYPE_NONE:
> diff --git a/net/slirp.c b/net/slirp.c
> index c18060f..073bc69 100644
> --- a/net/slirp.c
> +++ b/net/slirp.c
> @@ -404,6 +404,8 @@ static SlirpState *slirp_lookup(Monitor *mon, const char *hub_id,
>                  monitor_printf(mon, "unrecognized (hub-id, stackname) pair\n");
>                  return NULL;
>              }
> +            warn_report("Using 'hub-id' is deprecated. Specify the netdev id "
> +                        "directly instead");

Comma instead of period, please.

>          } else {
>              nc = qemu_find_netdev(name);
>              if (!nc) {
> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> index 8a2e399..eed7132 100644
> --- a/qemu-deprecated.texi
> +++ b/qemu-deprecated.texi
> @@ -83,6 +83,11 @@ The 'file' driver for drives is no longer appropriate for character or host
>  devices and will only accept regular files (S_IFREG). The correct driver
>  for these file types is 'host_cdrom' or 'host_device' as appropriate.
>  
> +@subsection -net ...,name=@var{name} (since 3.1)
> +
> +The @option{name} parameter of the @option{-net} option is an old synonym
> +for the @option{id} parameter which should now be used instead.
> +

Suggest to scratch "old".  Comma before "which".

>  @section QEMU Machine Protocol (QMP) commands
>  
>  @subsection block-dirty-bitmap-add "autoload" parameter (since 2.12.0)
> @@ -99,6 +104,14 @@ The ``query-cpus'' command is replaced by the ``query-cpus-fast'' command.
>  The ``arch'' output member of the ``query-cpus-fast'' command is
>  replaced by the ``target'' output member.
>  
> +@section System emulator human monitor commands
> +
> +@subsection The hub_id parameter of 'hostfwd_add' / 'hostfwd_remove' (since 3.1)
> +
> +The @option{hub_id} parameter of the 'hostfwd_add' and 'hostfwd_remove' HMP
> +commands is redundant. It is enough to specify the netdev ID of the backend
> +that should be changed.

Hmm.  What's redundant is the [hub_id name] alternative in

    .params     = "[hub_id name]|[netdev_id] ..."

The [netdev_id] alternative was added in January (commit 93653066445).
Suggest to word it more like other sections do:

    Parameters @option{hub_id} and @option{name} have been replaced by
    @option{netdev_id}.

> +
>  @section System emulator devices
>  
>  @subsection ivshmem (since 2.6.0)

On 19 September 2018 at 23:07, Markus Armbruster <armbru@redhat.com> wrote:
> Thomas Huth <thuth@redhat.com> writes:
>
>> In early times, network backends were specified by a "vlan" and "name"
>> tuple. With the introduction of netdevs, the "name" was replaced by an
>> "id" (which is supposed to be unique), but the "name" parameter stayed
>> as an alias which could be used instead of "id". Unfortunately, we miss
>> the duplication check for "name":
>>
>>  $ qemu-system-x86_64 -net user,name=n1 -net user,name=n1
>>
>> ... starts without an error, while "id" correctly complains:
>>
>>  $ qemu-system-x86_64 -net user,id=n1 -net user,id=n1
>>  qemu-system-x86_64: -net user,id=n1: Duplicate ID 'n1' for net
>>
>> Instead of trying to fix the code for the legacy "name" parameter, let's
>> rather get rid of this old interface and deprecate the "name" parameter
>> now - this will also be less confusing for the users in the long run.
>>
>> While we're at it, also deprecate the old syntax for the hostfwd_add
>> and hostfwd_remove commands that still work with this legacy "hub"
>> plus "name" tuple. It is enough to specify the netdev id there instead.
>>
>> Also add a missing dependency to the Makefile to make sure that the
>> docs get correctly regenerated when qemu-deprecated.texi is changed.
>
> Sure sounds like three patches to me.

I've occasionally wondered if we should have a checkpatch warning
"Commit message contains the string 'Also'" :-)

thanks
-- PMM

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 2018-09-20 08:07, Markus Armbruster wrote:
> Thomas Huth <thuth@redhat.com> writes:
> 
>> In early times, network backends were specified by a "vlan" and "name"
>> tuple. With the introduction of netdevs, the "name" was replaced by an
>> "id" (which is supposed to be unique), but the "name" parameter stayed
>> as an alias which could be used instead of "id". Unfortunately, we miss
>> the duplication check for "name":
>>
>>  $ qemu-system-x86_64 -net user,name=n1 -net user,name=n1
>>
>> ... starts without an error, while "id" correctly complains:
>>
>>  $ qemu-system-x86_64 -net user,id=n1 -net user,id=n1
>>  qemu-system-x86_64: -net user,id=n1: Duplicate ID 'n1' for net
>>
>> Instead of trying to fix the code for the legacy "name" parameter, let's
>> rather get rid of this old interface and deprecate the "name" parameter
>> now - this will also be less confusing for the users in the long run.
>>
>> While we're at it, also deprecate the old syntax for the hostfwd_add
>> and hostfwd_remove commands that still work with this legacy "hub"
>> plus "name" tuple. It is enough to specify the netdev id there instead.
>>
>> Also add a missing dependency to the Makefile to make sure that the
>> docs get correctly regenerated when qemu-deprecated.texi is changed.
> 
> Sure sounds like three patches to me.

Since the changes are really small and slightly related, one compact
patch sounded fine for me, too. But ok, I just discovered yet another
unrelated thing that should be added to the deprecation chapter
("enforce-config-section"), so I'll turn this into a small patch series
instead.

>> @@ -99,6 +104,14 @@ The ``query-cpus'' command is replaced by the ``query-cpus-fast'' command.
>>  The ``arch'' output member of the ``query-cpus-fast'' command is
>>  replaced by the ``target'' output member.
>>  
>> +@section System emulator human monitor commands
>> +
>> +@subsection The hub_id parameter of 'hostfwd_add' / 'hostfwd_remove' (since 3.1)
>> +
>> +The @option{hub_id} parameter of the 'hostfwd_add' and 'hostfwd_remove' HMP
>> +commands is redundant. It is enough to specify the netdev ID of the backend
>> +that should be changed.
> 
> Hmm.  What's redundant is the [hub_id name] alternative in
> 
>     .params     = "[hub_id name]|[netdev_id] ..."
> 
> The [netdev_id] alternative was added in January (commit 93653066445).
> Suggest to word it more like other sections do:
> 
>     Parameters @option{hub_id} and @option{name} have been replaced by
>     @option{netdev_id}.

Ok. The funny thing is that IIUC "name" and "netdev_id" are the very
same identifier nowadays (I was not aware of this in the past), so the
descriptions could also have been written as "[[hub_id] netdev_id]"
instead. ==> This "name" vs. "id" thing is really confusing, it's really
a good idea to remove it soon.

 Thomas

Thomas Huth <thuth@redhat.com> writes:

> On 2018-09-20 08:07, Markus Armbruster wrote:
>> Thomas Huth <thuth@redhat.com> writes:
>> 
>>> In early times, network backends were specified by a "vlan" and "name"
>>> tuple. With the introduction of netdevs, the "name" was replaced by an
>>> "id" (which is supposed to be unique), but the "name" parameter stayed
>>> as an alias which could be used instead of "id". Unfortunately, we miss
>>> the duplication check for "name":
>>>
>>>  $ qemu-system-x86_64 -net user,name=n1 -net user,name=n1
>>>
>>> ... starts without an error, while "id" correctly complains:
>>>
>>>  $ qemu-system-x86_64 -net user,id=n1 -net user,id=n1
>>>  qemu-system-x86_64: -net user,id=n1: Duplicate ID 'n1' for net
>>>
>>> Instead of trying to fix the code for the legacy "name" parameter, let's
>>> rather get rid of this old interface and deprecate the "name" parameter
>>> now - this will also be less confusing for the users in the long run.
>>>
>>> While we're at it, also deprecate the old syntax for the hostfwd_add
>>> and hostfwd_remove commands that still work with this legacy "hub"
>>> plus "name" tuple. It is enough to specify the netdev id there instead.
>>>
>>> Also add a missing dependency to the Makefile to make sure that the
>>> docs get correctly regenerated when qemu-deprecated.texi is changed.
>> 
>> Sure sounds like three patches to me.
>
> Since the changes are really small and slightly related, one compact
> patch sounded fine for me, too. But ok, I just discovered yet another
> unrelated thing that should be added to the deprecation chapter
> ("enforce-config-section"), so I'll turn this into a small patch series
> instead.

Thanks.

>>> @@ -99,6 +104,14 @@ The ``query-cpus'' command is replaced by the ``query-cpus-fast'' command.
>>>  The ``arch'' output member of the ``query-cpus-fast'' command is
>>>  replaced by the ``target'' output member.
>>>  
>>> +@section System emulator human monitor commands
>>> +
>>> +@subsection The hub_id parameter of 'hostfwd_add' / 'hostfwd_remove' (since 3.1)
>>> +
>>> +The @option{hub_id} parameter of the 'hostfwd_add' and 'hostfwd_remove' HMP
>>> +commands is redundant. It is enough to specify the netdev ID of the backend
>>> +that should be changed.
>> 
>> Hmm.  What's redundant is the [hub_id name] alternative in
>> 
>>     .params     = "[hub_id name]|[netdev_id] ..."
>> 
>> The [netdev_id] alternative was added in January (commit 93653066445).
>> Suggest to word it more like other sections do:
>> 
>>     Parameters @option{hub_id} and @option{name} have been replaced by
>>     @option{netdev_id}.
>
> Ok. The funny thing is that IIUC "name" and "netdev_id" are the very
> same identifier nowadays (I was not aware of this in the past), so the
> descriptions could also have been written as "[[hub_id] netdev_id]"
> instead. ==> This "name" vs. "id" thing is really confusing, it's really
> a good idea to remove it soon.

Much appreciated.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list