Re: [Qemu-devel] [PATCH 07/19] Convert netdev client types to use an enumeration

[Anthony Liguori <anthony@codemonkey.ws>]

On 06/07/2010 09:42 AM, Daniel P. Berrange wrote:
> Declare an enumeration for all netdev client types, values
> matching indexes in the net_client_types array. Use the
> enum helpers for the string<->  int conversion of client types.
>
> Before:
>
>    $ qemu -net type=foo,sfs
>    qemu: -net type=foo,sfs: Parameter 'type' expects a network client type
>
> After:
>
>    $ qemu -net type=foo,sfs
>    qemu: -net type=foo,sfs: Parameter 'type' expects none, nic, user, tap, socket, dump
>
> Signed-off-by: Daniel P. Berrange<berrange@redhat.com>
> ---
>   net.c |  124 ++++++++++++++++++++++++++++++++++++++--------------------------
>   1 files changed, 74 insertions(+), 50 deletions(-)
>
> diff --git a/net.c b/net.c
> index efa8b3d..5349001 100644
> --- a/net.c
> +++ b/net.c
> @@ -42,6 +42,36 @@ static QTAILQ_HEAD(, VLANClientState) non_vlan_clients;
>
>   int default_net = 1;
>
> +enum {
> +    NET_CLIENT_NONE,
> +    NET_CLIENT_NIC,
> +#ifdef CONFIG_SLIRP
> +    NET_CLIENT_USER,
> +#endif
> +    NET_CLIENT_TAP,
> +    NET_CLIENT_SOCKET,
> +#ifdef CONFIG_VDE
> +    NET_CLIENT_VDE,
> +#endif
> +    NET_CLIENT_DUMP,
> +
> +    NET_CLIENT_LAST
> +};
> +
> +QEMU_ENUM_DECL(qemu_net_type);
> +QEMU_ENUM_IMPL(qemu_net_type, NET_CLIENT_LAST,
> +	       "none",
> +	       "nic",
> +#ifdef CONFIG_SLIRP
> +	       "user",
> +#endif
> +	       "tap",
> +	       "socket",
> +#ifdef CONFIG_VDE
> +	       "vde",
> +#endif
> +	       "dump");
> +
>   /***********************************************************/
>   /* network device redirectors */
>
> @@ -844,18 +874,15 @@ typedef int (*net_client_init_func)(QemuOpts *opts,
>   #define NET_MAX_DESC 20
>
>   static const struct {
> -    const char *type;
>       net_client_init_func init;
>       QemuOptDesc desc[NET_MAX_DESC];
>   } net_client_types[] = {
>    

I think:

  [NET_CLIENT_NONE] = {
             .desc = {...}
},

Would be a bit more robust than relying on explicit ordering.

Regards,

Anthony Liguori

> -    {
> -        .type = "none",
> +    { /* NET_CLIENT_NONE */
>           .desc = {
>               NET_COMMON_PARAMS_DESC,
>               { /* end of list */ }
>           },
> -    }, {
> -        .type = "nic",
> +    }, { /* NET_CLIENT_NIC */
>           .init = net_init_nic,
>           .desc = {
>               NET_COMMON_PARAMS_DESC,
> @@ -884,8 +911,7 @@ static const struct {
>               { /* end of list */ }
>           },
>   #ifdef CONFIG_SLIRP
> -    }, {
> -        .type = "user",
> +    }, { /* NET_CLIENT_USER */
>           .init = net_init_slirp,
>           .desc = {
>               NET_COMMON_PARAMS_DESC,
> @@ -945,8 +971,7 @@ static const struct {
>               { /* end of list */ }
>           },
>   #endif
> -    }, {
> -        .type = "tap",
> +    }, { /* NET_CLIENT_TAP */
>           .init = net_init_tap,
>           .desc = {
>               NET_COMMON_PARAMS_DESC,
> @@ -988,8 +1013,7 @@ static const struct {
>   #endif /* _WIN32 */
>               { /* end of list */ }
>           },
> -    }, {
> -        .type = "socket",
> +    }, { /* NET_CLIENT_SOCKET */
>           .init = net_init_socket,
>           .desc = {
>               NET_COMMON_PARAMS_DESC,
> @@ -1013,8 +1037,7 @@ static const struct {
>               { /* end of list */ }
>           },
>   #ifdef CONFIG_VDE
> -    }, {
> -        .type = "vde",
> +    }, { /* NET_CLIENT_VDE */
>           .init = net_init_vde,
>           .desc = {
>               NET_COMMON_PARAMS_DESC,
> @@ -1038,8 +1061,7 @@ static const struct {
>               { /* end of list */ }
>           },
>   #endif
> -    }, {
> -        .type = "dump",
> +    }, { /* NET_CLIENT_DUMP */
>           .init = net_init_dump,
>           .desc = {
>               NET_COMMON_PARAMS_DESC,
> @@ -1055,30 +1077,41 @@ static const struct {
>               { /* end of list */ }
>           },
>       },
> -    { /* end of list */ }
>   };
> +verify(ARRAY_SIZE(net_client_types) == NET_CLIENT_LAST);
>
>   int net_client_init(Monitor *mon, QemuOpts *opts, int is_netdev)
>   {
>       const char *name;
> -    const char *type;
> -    int i;
> +    const char *typestr;
> +    VLANState *vlan = NULL;
> +    int type;
>
> -    type = qemu_opt_get(opts, "type");
> -    if (!type) {
> +    typestr = qemu_opt_get(opts, "type");
> +    if (!typestr) {
>           qerror_report(QERR_MISSING_PARAMETER, "type");
>           return -1;
>       }
> +    type = qemu_net_type_from_string(typestr);
> +
> +    if (type<  0) {
> +	char *valid = qemu_net_type_to_string_list();
> +	qerror_report(QERR_INVALID_PARAMETER_VALUE, "type",
> +		      valid);
> +	qemu_free(valid);
> +	return -1;
> +    }
>
>       if (is_netdev) {
> -        if (strcmp(type, "tap") != 0&&
> +	if (type<  0 ||
> +	    (type != NET_CLIENT_TAP&&
>   #ifdef CONFIG_SLIRP
> -            strcmp(type, "user") != 0&&
> +	     type != NET_CLIENT_USER&&
>   #endif
>   #ifdef CONFIG_VDE
> -            strcmp(type, "vde") != 0&&
> +	     type != NET_CLIENT_VDE&&
>   #endif
> -            strcmp(type, "socket") != 0) {
> +	     type != NET_CLIENT_SOCKET)) {
>               qerror_report(QERR_INVALID_PARAMETER_VALUE, "type",
>                             "a netdev backend type");
>               return -1;
> @@ -1103,35 +1136,26 @@ int net_client_init(Monitor *mon, QemuOpts *opts, int is_netdev)
>           name = qemu_opt_get(opts, "name");
>       }
>
> -    for (i = 0; net_client_types[i].type != NULL; i++) {
> -        if (!strcmp(net_client_types[i].type, type)) {
> -            VLANState *vlan = NULL;
> -
> -            if (qemu_opts_validate(opts,&net_client_types[i].desc[0]) == -1) {
> -                return -1;
> -            }
> -
> -            /* Do not add to a vlan if it's a -netdev or a nic with a
> -             * netdev= parameter. */
> -            if (!(is_netdev ||
> -                  (strcmp(type, "nic") == 0&&  qemu_opt_get(opts, "netdev")))) {
> -                vlan = qemu_find_vlan(qemu_opt_get_number(opts, "vlan", 0), 1);
> -            }
> +    if (qemu_opts_validate(opts,&net_client_types[type].desc[0]) == -1) {
> +	return -1;
> +    }
>
> -            if (net_client_types[i].init) {
> -                if (net_client_types[i].init(opts, mon, name, vlan)<  0) {
> -                    /* TODO push error reporting into init() methods */
> -                    qerror_report(QERR_DEVICE_INIT_FAILED, type);
> -                    return -1;
> -                }
> -            }
> -            return 0;
> -        }
> +    /* Do not add to a vlan if it's a -netdev or a nic with a
> +     * netdev= parameter. */
> +    if (!(is_netdev ||
> +	  (type == NET_CLIENT_NIC&&  qemu_opt_get(opts, "netdev")))) {
> +	vlan = qemu_find_vlan(qemu_opt_get_number(opts, "vlan", 0), 1);
>       }
>
> -    qerror_report(QERR_INVALID_PARAMETER_VALUE, "type",
> -                  "a network client type");
> -    return -1;
> +    if (net_client_types[type].init) {
> +	if (net_client_types[type].init(opts, mon, name, vlan)<  0) {
> +	    /* TODO push error reporting into init() methods */
> +	    qerror_report(QERR_DEVICE_INIT_FAILED,
> +			  qemu_net_type_to_string(type));
> +	    return -1;
> +	}
> +    }
> +    return 0;
>   }
>
>   static int net_host_check_device(const char *device)
>