Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consolidate network devices #64

Merged
merged 2 commits into from
Dec 2, 2024
Merged

Consolidate network devices #64

merged 2 commits into from
Dec 2, 2024

Conversation

chiangkd
Copy link
Collaborator

This PR refactors the existing TAP-based network device implementation without affecting the current functionality. It separates network client handling to support multiple backends, allowing the use of the -netdev flag to specify the client type, defaulting to TAP if not provided. Support for other backends is yet to be implemented.

Additionally, packet transfer functions are now designed to be specific to each network client, making the system more modular and extensible for future development.

Verification of existing functionality (in privileged mode):

  1. Open TAP device and run simulator
sudo make check
  1. Configure the IP address for tap0 and enable it on the host side:
sudo ip addr add 192.168.10.1/24 dev tap0
sudo ip link set tap0 up
  1. Enable eth0 and set its IP address:
ip l set eth0 up
ip a add 192.168.10.2/24 dev eth0

net_client.h Outdated Show resolved Hide resolved
net_client.h Outdated Show resolved Hide resolved
Copy link
Collaborator

@jserv jserv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enhance CI pipeline to automate the network specific tests.

See vwifi for reference.

netdev.c Outdated Show resolved Hide resolved
netdev.c Outdated Show resolved Hide resolved
virtio-net.c Outdated Show resolved Hide resolved
netdev.h Outdated Show resolved Hide resolved
NET_DEV_DRIVER_MAX,
} net_dev_driver_t;

typedef struct {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you hide the implementation details as possible?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I use an X-Macro to define supported network devices:

#define SUPPORTED_DEVICES   \
        _(tap)              \
        _(user)

Previously, NET_DEV_DRIVER_MAX was used to denote the total number of network device drivers implemented. To simplify the design, this constant has been removed. Instead, the find_net_dev_idx function dynamically determines the range of the lookup table based on its content.

The final NULL ensures the array is properly terminated, allowing for safe iteration.

static const char *netdev_driver_lookup[] = {
#define _(dev) [NET_DEV_DRIVER_##dev] = #dev,
    SUPPORTED_DEVICES
#undef _
        NULL,
};

...

static int find_net_dev_idx(const char *net_type, const char **netlookup)
{
    if (!net_type)
        return -1;
    int i = 0;
    while (netlookup[i]) {
        if (!strcmp(net_type, netlookup[i])) {
            return i;
        }
        i++;
    }
    return -1;
}

@jserv jserv changed the title Consolidate and extend network client support Consolidate network devices Nov 23, 2024
netdev.c Outdated Show resolved Hide resolved
netdev.c Outdated Show resolved Hide resolved
netdev.c Outdated Show resolved Hide resolved
netdev.h Outdated
Comment on lines 28 to 31
union {
net_tap_options tap;
net_user_options user;
} op;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think of the use of opaque pointer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I try to encapsulate the implementation details of netdev_t as follow:

typedef struct {
    char *name;
    net_dev_driver_t type;
    void *device_op;
} netdev_t;

The device_op pointer is allocated and defined during the netdev_init process, which is called by virtio_net_init. Also read/write operations in virtio-net depend on the underlying peer device specified by netdev_t.

static ssize_t handle_write(netdev_t *netdev,
                            virtio_net_queue_t *queue,
                            struct iovec *iovs_cursor,
                            size_t niovs)
{
    ssize_t plen = 0;
#define _(dev) NET_DEV_DRIVER_##dev
    switch (netdev->type) {
    case _(tap):
        net_tap_options_t *tap = (net_tap_options_t *) netdev->device_op;
        plen = writev(tap->tap_fd, iovs_cursor, niovs);
        if (plen < 0 && (errno == EWOULDBLOCK || errno == EAGAIN)) {
            queue->fd_ready = false;
            return -1;
        }
        if (plen < 0) {
            plen = 0;
            fprintf(stderr, "[VNET] could not write packet: %s\n",
                    strerror(errno));
        }
        break;
        ...
    }
#undef _
    return plen;
}

netdev.h Outdated Show resolved Hide resolved
netdev.h Outdated Show resolved Hide resolved
netdev.c Outdated Show resolved Hide resolved
Separate network device handling to support various network backends,
such as Slirp, instead of hard-coding the TAP device. The '-netdev'
flag can now be used to specify the network device type, defaulting
to TAP if unspecified. Additionally, packet transfer functions are
now tailored to each network device, enabling more modular and
extensible network device development.
To support future development, tests are added for different network
devices. ICMP packet transmission is used to verify link functionality
between virtio-net and other network devices, such as TAP.
@jserv jserv merged commit 333254e into sysprog21:master Dec 2, 2024
3 checks passed
@jserv
Copy link
Collaborator

jserv commented Dec 2, 2024

Thank @chiangkd for contributing!

@chiangkd chiangkd deleted the dev branch December 3, 2024 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants