[15/16] convert net_init_bridge() to NetClientOptions

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 net/tap.c |   23 ++++++++++++-----------
 1 files changed, 12 insertions(+), 11 deletions(-)

Il 22/05/2012 12:45, Laszlo Ersek ha scritto:
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  net/tap.c |   23 ++++++++++++-----------
>  1 files changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/net/tap.c b/net/tap.c
> index 7501eba..fdaab2b 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -512,21 +512,22 @@ static int net_bridge_run_helper(const char *helper, const char *bridge)
>      return -1;
>  }
>  
> -int net_init_bridge(QemuOpts *opts, const NetClientOptions *new_opts,
> +int net_init_bridge(QemuOpts *old_opts, const NetClientOptions *opts,
>                      const char *name, VLANState *vlan)
>  {
> +    const NetdevBridgeOptions *bridge;
> +    const char *helper, *br;
> +
>      TAPState *s;
>      int fd, vnet_hdr;
>  
> -    if (!qemu_opt_get(opts, "br")) {
> -        qemu_opt_set(opts, "br", DEFAULT_BRIDGE_INTERFACE);
> -    }
> -    if (!qemu_opt_get(opts, "helper")) {
> -        qemu_opt_set(opts, "helper", DEFAULT_BRIDGE_HELPER);
> -    }
> +    assert(opts->kind == NET_CLIENT_OPTIONS_KIND_BRIDGE);
> +    bridge = opts->bridge;
> +
> +    helper = bridge->has_helper ? bridge->helper : DEFAULT_BRIDGE_HELPER;
> +    br     = bridge->has_br     ? bridge->br     : DEFAULT_BRIDGE_INTERFACE;

Don't hate me for this, but why not do the same for strdup calls?

    foo = bar->has_foo ? g_strdup(bar->foo) : NULL;

earlier in the series?

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

On 06/05/12 23:05, Paolo Bonzini wrote:
> Il 22/05/2012 12:45, Laszlo Ersek ha scritto:
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>  net/tap.c |   23 ++++++++++++-----------
>>  1 files changed, 12 insertions(+), 11 deletions(-)
>>
>> diff --git a/net/tap.c b/net/tap.c
>> index 7501eba..fdaab2b 100644
>> --- a/net/tap.c
>> +++ b/net/tap.c
>> @@ -512,21 +512,22 @@ static int net_bridge_run_helper(const char *helper, const char *bridge)
>>      return -1;
>>  }
>>  
>> -int net_init_bridge(QemuOpts *opts, const NetClientOptions *new_opts,
>> +int net_init_bridge(QemuOpts *old_opts, const NetClientOptions *opts,
>>                      const char *name, VLANState *vlan)
>>  {
>> +    const NetdevBridgeOptions *bridge;
>> +    const char *helper, *br;
>> +
>>      TAPState *s;
>>      int fd, vnet_hdr;
>>  
>> -    if (!qemu_opt_get(opts, "br")) {
>> -        qemu_opt_set(opts, "br", DEFAULT_BRIDGE_INTERFACE);
>> -    }
>> -    if (!qemu_opt_get(opts, "helper")) {
>> -        qemu_opt_set(opts, "helper", DEFAULT_BRIDGE_HELPER);
>> -    }
>> +    assert(opts->kind == NET_CLIENT_OPTIONS_KIND_BRIDGE);
>> +    bridge = opts->bridge;
>> +
>> +    helper = bridge->has_helper ? bridge->helper : DEFAULT_BRIDGE_HELPER;
>> +    br     = bridge->has_br     ? bridge->br     : DEFAULT_BRIDGE_INTERFACE;
> 
> Don't hate me for this, but why not do the same for strdup calls?
> 
>     foo = bar->has_foo ? g_strdup(bar->foo) : NULL;
> 
> earlier in the series?

(I think you mean net_init_nic() in [PATCH 09/16] "convert
net_init_nic() to NetClientOptions".)

I didn't deliberately avoid the conditional operator there. The
net_init_nic() structure seemed OK; it sets all such pointers to
all-bits-zero in a batch (memset()) and only changes those whose
corresponding optarg (QemuOpt) is set. It seemed natural and didn't
summon ?: to my mind.

net_init_bridge() OTOH sets the defaults too on a case-by-case basis,
and it was screaming after ?:.

... No hate whatsoever :)

Laszlo

Il 06/06/2012 14:16, Laszlo Ersek ha scritto:
> The net_init_nic() structure seemed OK; it sets all such pointers to 
> all-bits-zero in a batch (memset()) and only changes those whose 
> corresponding optarg (QemuOpt) is set.

Makes sense.

Paolo