Looks good, thanks. Please see comments below:
Dan Smith wrote:
> This moves the INET4 address checkpoint and restart routines into
> net/ipv4/devinet.c and introduces a registration method to present
> the checkpoint code with the handler functions.
>
> This makes it easier to add additional address types, and also
> makes the cases where inet4 is absent, inet6 is a module, etc much
> easier. It also elminates the need for a couple of helper functions.
>
> Signed-off-by: Dan Smith <danms@xxxxxxxxxx>
> ---
> include/linux/checkpoint.h | 18 +++++-
> include/linux/checkpoint_hdr.h | 1 +
> net/checkpoint_dev.c | 152 ++++++++++++++++++++++++----------------
> net/ipv4/devinet.c | 75 ++++++++++++++++++++
> 4 files changed, 184 insertions(+), 62 deletions(-)
>
> diff --git a/include/linux/checkpoint.h b/include/linux/checkpoint.h
> index 96693e2..5fdbd01 100644
> --- a/include/linux/checkpoint.h
> +++ b/include/linux/checkpoint.h
> @@ -132,7 +132,8 @@ extern void *restore_netdev(struct ckpt_ctx *ctx);
>
> extern int ckpt_netdev_in_init_netns(struct ckpt_ctx *ctx,
> struct net_device *dev);
> -extern int ckpt_netdev_inet_addrs(struct in_device *indev,
> +extern int ckpt_netdev_inet_addrs(struct ckpt_ctx *ctx,
> + struct net_device *dev,
> struct ckpt_netdev_addr *list[]);
> extern int ckpt_netdev_hwaddr(struct net_device *dev,
> struct ckpt_hdr_netdev *h);
> @@ -513,6 +514,21 @@ extern void _ckpt_msg_complete(struct ckpt_ctx *ctx);
> _do_ckpt_msg(ctx, err, "[E @ %s:%d]" fmt, __func__, __LINE__, ##args); \
> } while (0)
>
> +struct ckpt_netdev_addr_handler {
> + int type;
> + struct module *owner;
> + int (*checkpoint_addr)(struct ckpt_ctx *ctx,
> + struct net_device *dev,
> + int index, int max,
> + struct ckpt_netdev_addr *addrs);
> + int (*restore_addr)(struct ckpt_ctx *ctx,
> + struct net_device *dev,
> + struct net *net,
> + struct ckpt_netdev_addr *addr);
> +};
> +extern int ckpt_netdev_addr_register(struct ckpt_netdev_addr_handler *);
> +extern int ckpt_netdev_addr_unregister(struct ckpt_netdev_addr_handler *);
In another context, Matt pointed out that the naming convention
should start with "register", so something like:
register_checkpoint_netdev_addr()
unregister_checkpoint_netdev_addr()
> +
> #endif /* CONFIG_CHECKPOINT */
> #endif /* __KERNEL__ */
>
> diff --git a/include/linux/checkpoint_hdr.h b/include/linux/checkpoint_hdr.h
> index 36386ad..13bf62c 100644
> --- a/include/linux/checkpoint_hdr.h
> +++ b/include/linux/checkpoint_hdr.h
> @@ -804,6 +804,7 @@ struct ckpt_hdr_netdev {
>
> enum ckpt_netdev_addr_types {
> CKPT_NETDEV_ADDR_IPV4,
> + CKPT_NETDEV_ADDR_MAX
> };
>
> struct ckpt_netdev_addr {
> diff --git a/net/checkpoint_dev.c b/net/checkpoint_dev.c
> index 5a4a95b..4ef06e3 100644
> --- a/net/checkpoint_dev.c
> +++ b/net/checkpoint_dev.c
> @@ -12,11 +12,11 @@
> #include <linux/sched.h>
> #include <linux/if.h>
> #include <linux/if_arp.h>
> -#include <linux/inetdevice.h>
> #include <linux/veth.h>
> #include <linux/checkpoint.h>
> #include <linux/checkpoint_hdr.h>
> #include <linux/deferqueue.h>
> +#include <linux/module.h>
>
> #include <net/net_namespace.h>
> #include <net/sch_generic.h>
> @@ -34,17 +34,64 @@ struct mvl_newlink {
>
> typedef int (*new_link_fn)(struct sk_buff *, void *);
>
> -static int __kern_devinet_ioctl(struct net *net, unsigned int cmd, void *arg)
> +static struct ckpt_netdev_addr_handler *addr_handlers[CKPT_NETDEV_ADDR_MAX];
> +
> +static char *addr_modules[] = {
> + "ipv4", /* CKPT_NETDEV_ADDR_IPV4 */
> + "ipv6", /* CKPT_NETDEV_ADDR_IPV6 */
> +};
> +
> +int ckpt_netdev_addr_register(struct ckpt_netdev_addr_handler *h)
> {
> - mm_segment_t fs;
> - int ret;
> + if (h->type >= CKPT_NETDEV_ADDR_MAX)
> + return -EINVAL;
h->type is 'int' and can be negative :(
Either test for it, or better - change to unsigned int (above).
>
> - fs = get_fs();
> - set_fs(KERNEL_DS);
> - ret = devinet_ioctl(net, cmd, arg);
> - set_fs(fs);
> + if (addr_handlers[h->type] != NULL)
> + return -EEXIST;
Does this test-and-set need locking ?
>
> - return ret;
> + ckpt_debug("Registered addr type %s\n", addr_modules[h->type]);
> + addr_handlers[h->type] = h;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(ckpt_netdev_addr_register);
> +
> +int ckpt_netdev_addr_unregister(struct ckpt_netdev_addr_handler *h)
> +{
> + if (h->type >= CKPT_NETDEV_ADDR_MAX)
> + return -EINVAL;
> +
> + if (addr_handlers[h->type] == NULL)
> + return -ESRCH;
> +
> + ckpt_debug("Unregistered addr type %s\n", addr_modules[h->type]);
> + addr_handlers[h->type] = NULL;
Ditto ?
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(ckpt_netdev_addr_unregister);
> +
> +static struct ckpt_netdev_addr_handler *get_addr_handler(int type)
> +{
> + struct ckpt_netdev_addr_handler *h;
> +
> + if (type >= CKPT_NETDEV_ADDR_MAX)
> + return ERR_PTR(-EINVAL);
type is 'int' - same comment as above.
> +
> + h = addr_handlers[type];
Ditto for locking ? (try_module_get below should be inside the
lock, of course).
> +
> + if (h == NULL)
> + return NULL;
> +
> + if (try_module_get(h->owner))
> + return h;
> + else
> + return ERR_PTR(-EBUSY);
Maybe some ckpt_err() here ? I can feel the frustration of trying
to figure out where _this_ came from !
> +}
> +
> +static void put_addr_handler(struct ckpt_netdev_addr_handler *h)
> +{
> + module_put(h->owner);
> }
[...]
The rest looks ok :)
Oren.
_______________________________________________
Containers mailing list
Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/containers
