Re: [PATCH v20 5/7] net/vmnet: implement bridged mode (vmnet-bridged)

[Akihiko Odaki <akihiko.odaki@gmail.com>]

On 2022/03/16 8:07, Vladislav Yaroshchuk wrote:
> Signed-off-by: Vladislav Yaroshchuk <Vladislav.Yaroshchuk@jetbrains.com>
> ---
>   net/vmnet-bridged.m | 128 ++++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 123 insertions(+), 5 deletions(-)
> 
> diff --git a/net/vmnet-bridged.m b/net/vmnet-bridged.m
> index 91c1a2f2c7..5936c87718 100644
> --- a/net/vmnet-bridged.m
> +++ b/net/vmnet-bridged.m
> @@ -10,16 +10,134 @@
>   
>   #include "qemu/osdep.h"
>   #include "qapi/qapi-types-net.h"
> -#include "vmnet_int.h"
> -#include "clients.h"
> -#include "qemu/error-report.h"
>   #include "qapi/error.h"
> +#include "clients.h"
> +#include "vmnet_int.h"
>   
>   #include <vmnet/vmnet.h>
>   
> +
> +static bool validate_ifname(const char *ifname)
> +{
> +    xpc_object_t shared_if_list = vmnet_copy_shared_interface_list();
> +    bool match = false;
> +    if (!xpc_array_get_count(shared_if_list)) {
> +        goto done;
> +    }
> +
> +    match = !xpc_array_apply(
> +        shared_if_list,
> +        ^bool(size_t index, xpc_object_t value) {
> +            return strcmp(xpc_string_get_string_ptr(value), ifname) != 0;
> +        });
> +
> +done:
> +    xpc_release(shared_if_list);
> +    return match;
> +}
> +
> +
> +static bool get_valid_ifnames(char *output_buf)
> +{
> +    xpc_object_t shared_if_list = vmnet_copy_shared_interface_list();
> +    __block const char *ifname = NULL;
> +    __block int str_offset = 0;
> +    bool interfaces_available = true;
> +
> +    if (!xpc_array_get_count(shared_if_list)) {
> +        interfaces_available = false;
> +        goto done;
> +    }
> +
> +    xpc_array_apply(
> +        shared_if_list,
> +        ^bool(size_t index, xpc_object_t value) {
> +            /* build list of strings like "en0 en1 en2 " */
> +            ifname = xpc_string_get_string_ptr(value);
> +            strcpy(output_buf + str_offset, ifname);
> +            strcpy(output_buf + str_offset + strlen(ifname), " ");
> +            str_offset += strlen(ifname) + 1;
> +            return true;
> +        });
> +
> +done:
> +    xpc_release(shared_if_list);
> +    return interfaces_available;
> +}
> +
> +
> +static bool validate_options(const Netdev *netdev, Error **errp)
> +{
> +    const NetdevVmnetBridgedOptions *options = &(netdev->u.vmnet_bridged);
> +    char ifnames[1024];

There is no guarantee it fits in 1024 bytes. It was 256 bytes in an old 
version, but growing into some arbitrary size is not an appropriate fix. 
It should be dynamically allocated as it was done in an older version.

I'm sorry for missing things repeatedly. This should be *really* the 
last comment so please have a look at this.

P.S. I'm testing the current version and it is pleasantly working well. 
(I'm actually writing this email on QEMU with this series.)

Regards,
Akihiko Odaki

> +
> +    if (!validate_ifname(options->ifname)) {
> +        if (get_valid_ifnames(ifnames)) {
> +            error_setg(errp,
> +                       "unsupported ifname '%s', expected one of [ %s]",
> +                       options->ifname,
> +                       ifnames);
> +            return false;
> +        }
> +        error_setg(errp,
> +                   "unsupported ifname '%s', no supported "
> +                   "interfaces available",
> +                   options->ifname);
> +        return false;
> +    }
> +
> +#if !defined(MAC_OS_VERSION_11_0) || \
> +    MAC_OS_X_VERSION_MIN_REQUIRED < MAC_OS_VERSION_11_0
> +    if (options->has_isolated) {
> +        error_setg(errp,
> +                   "vmnet-bridged.isolated feature is "
> +                   "unavailable: outdated vmnet.framework API");
> +        return false;
> +    }
> +#endif
> +    return true;
> +}
> +
> +
> +static xpc_object_t build_if_desc(const Netdev *netdev)
> +{
> +    const NetdevVmnetBridgedOptions *options = &(netdev->u.vmnet_bridged);
> +    xpc_object_t if_desc = xpc_dictionary_create(NULL, NULL, 0);
> +
> +    xpc_dictionary_set_uint64(if_desc,
> +                              vmnet_operation_mode_key,
> +                              VMNET_BRIDGED_MODE
> +    );
> +
> +    xpc_dictionary_set_string(if_desc,
> +                              vmnet_shared_interface_name_key,
> +                              options->ifname);
> +
> +#if defined(MAC_OS_VERSION_11_0) && \
> +    MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_VERSION_11_0
> +    xpc_dictionary_set_bool(if_desc,
> +                            vmnet_enable_isolation_key,
> +                            options->isolated);
> +#endif
> +    return if_desc;
> +}
> +
> +
> +static NetClientInfo net_vmnet_bridged_info = {
> +    .type = NET_CLIENT_DRIVER_VMNET_BRIDGED,
> +    .size = sizeof(VmnetState),
> +    .receive = vmnet_receive_common,
> +    .cleanup = vmnet_cleanup_common,
> +};
> +
> +
>   int net_init_vmnet_bridged(const Netdev *netdev, const char *name,
>                              NetClientState *peer, Error **errp)
>   {
> -  error_setg(errp, "vmnet-bridged is not implemented yet");
> -  return -1;
> +    NetClientState *nc = qemu_new_net_client(&net_vmnet_bridged_info,
> +                                             peer, "vmnet-bridged", name);
> +    if (!validate_options(netdev, errp)) {
> +        return -1;
> +    }
> +    return vmnet_if_create(nc, build_if_desc(netdev), errp);
>   }