|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [UNIKRAFT PATCH v3 1/2] lib/uknetdev: Add ipaddr, netmask & gwaddr as arg
Hey Sharan,
Thanks a lot for your work, this is a very useful feature. I have some comments
to this patch.
I put then inline.
Thanks,
Simon
On 08.07.20, 17:06, "Minios-devel on behalf of Sharan Santhanam"
<minios-devel-bounces@xxxxxxxxxxxxxxxxxxxx on behalf of
Sharan.Santhanam@xxxxxxxxx> wrote:
The patch adds ip address, netmask and the gateway address as library
argument. The parameters are list of argument delimited by a space.
The library user can forward the argument using the following command
line:
net.ipv4_addr, net.ipv4_subnet_mask, net.ipv4_gw_addr
Signed-off-by: Sharan Santhanam <sharan.santhanam@xxxxxxxxx>
---
lib/uknetdev/Config.uk | 1 +
lib/uknetdev/Makefile.uk | 1 +
lib/uknetdev/include/uk/netdev_core.h | 9 ++++
lib/uknetdev/netdev.c | 78 +++++++++++++++++++++++++++
4 files changed, 89 insertions(+)
diff --git a/lib/uknetdev/Config.uk b/lib/uknetdev/Config.uk
index 618e8e67..186dd462 100644
--- a/lib/uknetdev/Config.uk
+++ b/lib/uknetdev/Config.uk
@@ -4,6 +4,7 @@ menuconfig LIBUKNETDEV
select LIBNOLIBC if !HAVE_LIBC
select LIBUKDEBUG
select LIBUKALLOC
+ imply LIBUKLIBPARAM
if LIBUKNETDEV
config LIBUKNETDEV_MAXNBQUEUES
diff --git a/lib/uknetdev/Makefile.uk b/lib/uknetdev/Makefile.uk
index ca08b254..abdcd4bc 100644
--- a/lib/uknetdev/Makefile.uk
+++ b/lib/uknetdev/Makefile.uk
@@ -1,4 +1,5 @@
$(eval $(call addlib_s,libuknetdev,$(CONFIG_LIBUKNETDEV)))
+$(eval $(call addlib_paramprefix,libuknetdev,net))
CINCLUDES-$(CONFIG_LIBUKNETDEV) += -I$(LIBUKNETDEV_BASE)/include
CXXINCLUDES-$(CONFIG_LIBUKNETDEV) += -I$(LIBUKNETDEV_BASE)/include
diff --git a/lib/uknetdev/include/uk/netdev_core.h
b/lib/uknetdev/include/uk/netdev_core.h
index dba719fc..d2e8da05 100644
--- a/lib/uknetdev/include/uk/netdev_core.h
+++ b/lib/uknetdev/include/uk/netdev_core.h
@@ -373,6 +373,12 @@ struct uk_netdev_data {
const char *drv_name;
};
+struct uk_netdev_config {
+ const char *ipv4_addr;
+ const char *ipv4_net_mask;
+ const char *ipv4_gw_addr;
+};
+
I would call it `struct uk_netdev_einfo` or similar because you are going to
use it to overwrite values from our existing `einfo` interface.
/**
* NETDEV
* A structure used to interact with a network device.
@@ -400,6 +406,9 @@ struct uk_netdev {
struct uk_netdev_tx_queue *_tx_queue[CONFIG_LIBUKNETDEV_MAXNBQUEUES];
UK_TAILQ_ENTRY(struct uk_netdev) _list;
+
+ /** Netdevice address configuration */
+ struct uk_netdev_config *_config;
I would call it `*_einfo` or `*_einfo_overwrite` to make it more clear what
this is.
};
#ifdef __cplusplus
diff --git a/lib/uknetdev/netdev.c b/lib/uknetdev/netdev.c
index 151e0897..e0ee4427 100644
--- a/lib/uknetdev/netdev.c
+++ b/lib/uknetdev/netdev.c
@@ -38,10 +38,62 @@
#include <string.h>
#include <uk/netdev.h>
#include <uk/print.h>
+#include <uk/libparam.h>
struct uk_netdev_list uk_netdev_list =
UK_TAILQ_HEAD_INITIALIZER(uk_netdev_list);
static uint16_t netdev_count;
+/**
+ * TODO: Define Network argument format when multiple driver device need to
+ * coexist. For example like:
+ * Driver Name:IP Address:Net Mask
+ */
+static char *ipv4_addr;
+static char *ipv4_subnet_mask;
+static char *ipv4_gw_addr;
+
+UK_LIB_PARAM_STR(ipv4_addr);
+UK_LIB_PARAM_STR(ipv4_subnet_mask);
+UK_LIB_PARAM_STR(ipv4_gw_addr);
+
+static const char *_parse_ipv4_addr(void)
+{
+ /** Remember the reference to the ip address for successive calls*/
+ static char *ip_addr;
+
+ if (ip_addr)
+ return strtok_r(NULL, " ", &ip_addr);
+ else if (ipv4_addr)
+ return strtok_r(ipv4_addr, " ", &ip_addr);
+
+ return NULL;
+}
+
+static const char *_parse_ipv4_net_mask(void)
+{
+ /** Remember the reference to the netmask for successive calls*/
+ static char *net_mask;
+
+ if (net_mask)
+ return strtok_r(NULL, " ", &net_mask);
+ else if (ipv4_subnet_mask)
+ return strtok_r(ipv4_subnet_mask, " ", &net_mask);
+
+ return NULL;
+}
+
+static const char *_parse_ipv4_gw_addr(void)
+{
+ /** Remember the reference to the gateway address for successive calls*/
+ static char *gw;
+
+ if (gw)
+ return strtok_r(NULL, " ", &gw);
+ else if (ipv4_gw_addr)
+ return strtok_r(ipv4_gw_addr, " ", &gw);
+
+ return NULL;
+}
static struct uk_netdev_data *_alloc_data(struct uk_alloc *a,
uint16_t netdev_id,
@@ -64,6 +116,25 @@ static struct uk_netdev_data *_alloc_data(struct
uk_alloc *a,
return data;
}
+static struct uk_netdev_config *_alloc_config(struct uk_alloc *a)
+{
+ struct uk_netdev_config *_config = NULL;
+
+ if (ipv4_addr || ipv4_subnet_mask || ipv4_gw_addr) {
+ _config = uk_zalloc(a, sizeof(*_config));
+ if (!_config) {
+ uk_pr_warn("Failed to allocate memory for netdev
config\n");
+ return NULL;
I don't think it is a good idea to ignore an allocation failure. I am concerned
that it leads to unexpected behavior. You could return an ERRPTR value:
<uk/errptr.h>.
+ }
+
+ _config->ipv4_addr = _parse_ipv4_addr();
+ _config->ipv4_net_mask = _parse_ipv4_net_mask();
+ _config->ipv4_gw_addr = _parse_ipv4_gw_addr();
This looks like caching values to me. You probably want to do this within `
uk_netdev_einfo_get()` when accessing a field the first time? The reason is
that you could then also cache the driver response when no overwrite exists.
+ }
+
+ return _config;
+}
+
int uk_netdev_drv_register(struct uk_netdev *dev, struct uk_alloc *a,
const char *drv_name)
{
@@ -91,6 +162,13 @@ int uk_netdev_drv_register(struct uk_netdev *dev,
struct uk_alloc *a,
if (!dev->_data)
return -ENOMEM;
+ /**
+ * Since the _config is meant to be optional, the boot process should
+ * still continue if the allocation of config fails instead of reporting
+ * error.
+ */
+ dev->_config = _alloc_config(a);
+
UK_TAILQ_INSERT_TAIL(&uk_netdev_list, dev, _list);
uk_pr_info("Registered netdev%"PRIu16": %p (%s)\n",
netdev_count, dev, drv_name);
--
2.20.1
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |