Re: [Qemu-devel] [PATCH V4 2/7] CAN bus support to connect bust to Linux host SocketCAN interface.

[Philippe Mathieu-Daudé]

Cc'ing Paolo and Marc-André, the "Character device backends" maintainers.

On 01/14/2018 05:14 PM, address@hidden wrote:
> From: Pavel Pisa <address@hidden>
> 
> Connection to the real host CAN bus network through
> SocketCAN network interface is available only for Linux
> host system. Mechanism is generic, support for another
> CAN API and operating systems can be implemented in future.
> 
> Signed-off-by: Pavel Pisa <address@hidden>
> ---
>  hw/can/Makefile.objs   |   4 +
>  hw/can/can_socketcan.c | 314 
> +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 318 insertions(+)
>  create mode 100644 hw/can/can_socketcan.c
> 
> diff --git a/hw/can/Makefile.objs b/hw/can/Makefile.objs
> index 1028d7c455..f999085f7a 100644
> --- a/hw/can/Makefile.objs
> +++ b/hw/can/Makefile.objs
> @@ -2,5 +2,9 @@
>  
>  ifeq ($(CONFIG_CAN_CORE),y)
>  common-obj-y += can_core.o
> +ifeq ($(CONFIG_LINUX),y)
> +common-obj-y += can_socketcan.o
> +else
>  common-obj-y += can_host_stub.o

I'd just throw this stub in stubs/can_host.c

>  endif
> +endif
> diff --git a/hw/can/can_socketcan.c b/hw/can/can_socketcan.c
> new file mode 100644
> index 0000000000..f6df747c5a
> --- /dev/null
> +++ b/hw/can/can_socketcan.c

I think this is not the correct place to put this file, this is not a
modeled hardware, but a chardev backend.

This would be chardev/can-socketcan.c.

> @@ -0,0 +1,314 @@
> +/*
> + * CAN socketcan support to connect to the Linux host SocketCAN interfaces
> + *
> + * Copyright (c) 2013-2014 Jin Yang
> + * Copyright (c) 2014-2018 Pavel Pisa
> + *
> + * Initial development supported by Google GSoC 2013 from RTEMS project slot
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a 
> copy
> + * of this software and associated documentation files (the "Software"), to 
> deal
> + * in the Software without restriction, including without limitation the 
> rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
> FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "qemu/error-report.h"
> +#include "chardev/char.h"
> +#include "qemu/sockets.h"
> +#include "qemu/error-report.h"
> +#include "hw/hw.h"
> +#include "can/can_emu.h"
> +
> +#include <sys/ioctl.h>
> +#include <net/if.h>
> +#include <linux/can.h>
> +#include <linux/can/raw.h>
> +
> +#ifndef DEBUG_CAN
> +#define DEBUG_CAN 0
> +#endif /*DEBUG_CAN*/
> +
> +#define CAN_READ_BUF_LEN  5
> +typedef struct {
> +    CanBusClientState  bus_client;
> +    qemu_can_filter    *rfilter;
> +    int                rfilter_num;
> +    can_err_mask_t     err_mask;
> +
> +    qemu_can_frame     buf[CAN_READ_BUF_LEN];
> +    int                bufcnt;
> +    int                bufptr;
> +
> +    int                fd;
> +} CanBusSocketcanConnectState;
> +
> +static void can_bus_socketcan_display_msg(struct qemu_can_frame *msg)
> +{
> +    int i;
> +
> +    /* Check that QEMU and Linux kernel flags encoding matches */
> +    assert(QEMU_CAN_EFF_FLAG == CAN_EFF_FLAG);
> +    assert(QEMU_CAN_RTR_FLAG == CAN_RTR_FLAG);
> +    assert(QEMU_CAN_ERR_FLAG == CAN_ERR_FLAG);
> +
> +    assert(QEMU_CAN_INV_FILTER == CAN_INV_FILTER);
> +
> +    assert(offsetof(qemu_can_frame, data) == offsetof(struct can_frame, 
> data));
> +
> +    qemu_log_lock();
> +    qemu_log("[cansocketcan]: %03X [%01d] %s %s",
> +             msg->can_id & QEMU_CAN_EFF_MASK,
> +             msg->can_dlc,
> +             msg->can_id & QEMU_CAN_EFF_FLAG ? "EFF" : "SFF",
> +             msg->can_id & QEMU_CAN_RTR_FLAG ? "RTR" : "DAT");
> +
> +    for (i = 0; i < msg->can_dlc; i++) {
> +        qemu_log(" %02X", msg->data[i]);
> +    }
> +    qemu_log("\n");
> +    qemu_log_flush();
> +    qemu_log_unlock();
> +}
> +
> +static void can_bus_socketcan_read(void *opaque)
> +{
> +    CanBusSocketcanConnectState *c;
> +    c = (CanBusSocketcanConnectState *)opaque;
> +
> +
> +
> +    /* CAN_READ_BUF_LEN for multiple messages syscall is possible for future 
> */
> +    c->bufcnt = read(c->fd, c->buf, sizeof(qemu_can_frame));
> +    if (c->bufcnt < 0) {
> +        warn_report("CAN bus host read failed (%s)", strerror(errno));
> +        return;
> +    }
> +
> +    can_bus_client_send(&c->bus_client, c->buf, 1);
> +
> +    if (DEBUG_CAN) {
> +        can_bus_socketcan_display_msg(c->buf);
> +    }
> +}
> +
> +static int can_bus_socketcan_can_receive(CanBusClientState *client)
> +{
> +    CanBusSocketcanConnectState *c;
> +    c = container_of(client, CanBusSocketcanConnectState, bus_client);
> +
> +    if (c->fd < 0) {
> +        return -1;
> +    }
> +
> +    return 1;
> +}
> +
> +static ssize_t can_bus_socketcan_receive(CanBusClientState *client,
> +                            const qemu_can_frame *frames, size_t frames_cnt)
> +{
> +    CanBusSocketcanConnectState *c;
> +    c = container_of(client, CanBusSocketcanConnectState, bus_client);
> +    size_t len = sizeof(qemu_can_frame);
> +    int res;
> +
> +    if (c->fd < 0) {
> +        return -1;
> +    }
> +
> +    res = write(c->fd, frames, len);
> +
> +    if (!res) {
> +        warn_report("[cansocketcan]: write message to host returns zero");
> +        return -1;
> +    }
> +
> +    if (res != len) {
> +        if (res < 0) {
> +            warn_report("[cansocketcan]: write to host failed (%s)",
> +                        strerror(errno));
> +        } else {
> +            warn_report("[cansocketcan]: write to host truncated");
> +        }
> +        return -1;
> +    }
> +
> +    return 1;
> +}
> +
> +static void can_bus_socketcan_cleanup(CanBusClientState *client)
> +{
> +    CanBusSocketcanConnectState *c;
> +    c = container_of(client, CanBusSocketcanConnectState, bus_client);
> +
> +    if (c->fd >= 0) {
> +        qemu_set_fd_handler(c->fd, NULL, NULL, c);
> +        close(c->fd);
> +        c->fd = -1;
> +    }
> +
> +    c->rfilter_num = 0;
> +    if (c->rfilter != NULL) {
> +        g_free(c->rfilter);
> +    }
> +}
> +
> +static int can_bus_socketcan_set_filters(CanBusClientState *client,
> +                   const struct qemu_can_filter *filters, size_t filters_cnt)
> +{
> +    CanBusSocketcanConnectState *c;
> +    c = container_of(client, CanBusSocketcanConnectState, bus_client);
> +
> +    int i;
> +
> +    if (DEBUG_CAN) {
> +        qemu_log_lock();
> +        qemu_log("[cansocketcan]: filters set for channel\n");
> +        for (i = 0; i < filters_cnt; i++) {
> +            fprintf(stderr, "[%i]  id=0x%08x maks=0x%08x\n",
> +                   i, filters[i].can_id, filters[i].can_mask);
> +        }
> +        qemu_log("\n");
> +        qemu_log_flush();
> +        qemu_log_unlock();
> +    }
> +
> +    setsockopt(c->fd, SOL_CAN_RAW, CAN_RAW_FILTER,
> +               filters, filters_cnt * sizeof(qemu_can_filter));

Good than you don't do the filtering in userland :)

> +
> +    return 0;
> +}
> +
> +static
> +void can_bus_socketcan_update_read_handler(CanBusSocketcanConnectState *c)
> +{
> +    if (c->fd >= 0) {
> +        qemu_set_fd_handler(c->fd, can_bus_socketcan_read, NULL, c);
> +    }
> +}
> +
> +static CanBusClientInfo can_bus_socketcan_bus_client_info = {
> +    .can_receive = can_bus_socketcan_can_receive,
> +    .receive = can_bus_socketcan_receive,
> +    .cleanup = can_bus_socketcan_cleanup,
> +    .poll = NULL
> +};
> +
> +static CanBusSocketcanConnectState *
> +    can_bus_socketcan_connect_new(const char *host_dev_name)
> +{
> +    int s; /* can raw socket */
> +    CanBusSocketcanConnectState    *c;
> +    struct sockaddr_can addr;
> +    struct ifreq ifr;
> +
> +    c = g_malloc0(sizeof(CanBusSocketcanConnectState));
> +    if (c == NULL) {
> +        goto fail1;
> +    }
> +
> +    c->fd = -1;
> +
> +    /* open socket */
> +    s = socket(PF_CAN, SOCK_RAW, CAN_RAW);
> +    if (s < 0) {
> +        error_report("[cansocketcan]: CAN_RAW socket create failed  (%s)",
> +                        strerror(errno));
> +        goto fail;
> +    }
> +
> +    addr.can_family = AF_CAN;
> +    memset(&ifr.ifr_name, 0, sizeof(ifr.ifr_name));
> +    strcpy(ifr.ifr_name, host_dev_name);
> +    if (ioctl(s, SIOCGIFINDEX, &ifr) < 0) {
> +        error_report("[cansocketcan]: host interface %s not available (%s)",
> +                     host_dev_name, strerror(errno));
> +        goto fail;
> +    }
> +    addr.can_ifindex = ifr.ifr_ifindex;
> +
> +    c->err_mask = 0xffffffff; /* Receive error frame. */
> +    setsockopt(s, SOL_CAN_RAW, CAN_RAW_ERR_FILTER,
> +                   &c->err_mask, sizeof(c->err_mask));
> +
> +    c->rfilter_num = 1;
> +    c->rfilter = g_malloc0(c->rfilter_num * sizeof(struct qemu_can_filter));
> +    if (c->rfilter == NULL) {
> +        goto fail;
> +    }
> +
> +    /* Receive all data frame. If |= CAN_INV_FILTER no data. */
> +    c->rfilter[0].can_id = 0;
> +    c->rfilter[0].can_mask = 0;
> +    c->rfilter[0].can_mask &= ~CAN_ERR_FLAG;
> +

I'd extract this in a more explicit explicit function like
promisc_filter() and use

       can_bus_socketcan_set_filters(c, promisc_filter);

maybe clever would be to use NULL for promisc?

#define CANBUS_PROMISC_FILTER NULL /* why... */

       can_bus_socketcan_set_filters(c, CANBUS_PROMISC_FILTER);

can_bus_socketcan_set_filters( ..., *filters, rfilter_num )
{
    static const qemu_can_filter promisc_filter = {
        .can_id = 0,
        .can_mask = 0,
        .can_mask &= ~CAN_ERR_FLAG
    };

    if (!filters) {
        filters = &promisc_filter;
        rfilter_num = 1;
    }
    setsockopt(c->fd, SOL_CAN_RAW, CAN_RAW_FILTER,
               filters, rfilter_num * sizeof(struct qemu_can_filter));
    ...

> +    setsockopt(s, SOL_CAN_RAW, CAN_RAW_FILTER, c->rfilter,
> +               c->

don't forget to g_free(c->rfilter);

> +
> +    if (bind(s, (struct sockaddr *)&addr, sizeof(addr)) < 0) {
> +        error_report("[cansocketcan]: bind to host interface %s failed (%s)",
> +                     host_dev_name, strerror(errno));
> +        goto fail;
> +    }
> +
> +    c->fd = s;
> +
> +    c->bus_client.info = &can_bus_socketcan_bus_client_info;
> +
> +    can_bus_socketcan_update_read_handler(c);
> +
> +    return c;
> +
> +fail:
> +    can_bus_socketcan_cleanup(&c->bus_client);
> +    g_free(c);
> +fail1:
> +
> +    return NULL;
> +}
> +
> +static int can_bus_connect_to_host_socketcan(CanBusState *bus,
> +                                             const char *host_dev_name)
> +{
> +    CanBusSocketcanConnectState *c;
> +
> +    c = can_bus_socketcan_connect_new(host_dev_name);
> +    if (c == NULL) {
> +        error_report("CAN bus setup of host connect to \"%s\" failed",
> +                      host_dev_name);
> +        exit(1);
> +    }
> +
> +    if (can_bus_insert_client(bus, &c->bus_client) < 0) {
> +        error_report("CAN host device \"%s\" connect to bus \"%s\" failed",
> +                      host_dev_name, bus->name);
> +        exit(1);
> +    }
> +
> +    if (0) {
> +        /*
> +         * Not used there or as a CanBusSocketcanConnectState method
> +         * for now but included there for documentation purposes
> +         * and to suppress warning.

So this can be avoided using PROMISC = NULL as suggested.

> +         */
> +        can_bus_socketcan_set_filters(&c->bus_client, NULL, 0);
> +    }
> +
> +    return 0;
> +}
> +
> +int (*can_bus_connect_to_host_variant)(CanBusState *bus, const char *name) =
> +        can_bus_connect_to_host_socketcan;
>