From: Blaisorblade <bla...@ya...> - 2007-02-21 23:54:17
|
I started uml_daemon as root by mistake, so /tmp/uml.ctl even if /dev/net/tun was world-readable. Ok, I did it. This is the result (after trying many times to do 'ifconfig eth0 up' inside UML): [42949710.290000] Choosing a random ethernet address for device eth0 [42949710.290000] Netdevice 0 (3a:9d:6f:12:83:b2) : daemon backend (uml_switch version 3) - unix:/tmp/uml.ctl [42949710.290000] daemon_open : control connect failed, errno = 13 (13 = EPERM) [42949720.360000] Registering fd 0 twice [42949720.360000] Irqs : 2, 5 [42949720.360000] Ids : 0x0000000061a795f8, 0x0000000061d181c0 [42949720.360000] uml_net_open: failed to get irq(-12) (-12 is -ENOMEM, not -13 + 1, but is caused by 'registering XXX twice'). [42949720.360000] Registering fd 0 twice [42949720.360000] Irqs : 2, 5 [42949720.360000] Ids : 0x0000000061a795f8, 0x0000000061d181c0 [42949720.360000] uml_net_open: failed to get irq(-12) Looking into the code, I saw that: 1) daemon_user_init is confusing because it saved any error into pri->fd, instead of returning it to the caller. Also, it would make sense to move this into daemon_open. 2) connect_to_switch uses the same errno twice - it shouldn't, and we're hitting this. um_interrupt is called with fd => 0 because errno is cleared after printk (verified under debugging, I did not trace it fully to where it's changing but it's sufficient for a patch anyway). -- Inform me of my mistakes, so I can add them to my list! Paolo Giarrusso, aka Blaisorblade http://www.user-mode-linux.org/~blaisorblade Chiacchiera con i tuoi amici in tempo reale! http://it.yahoo.com/mail_it/foot/*http://it.messenger.yahoo.com |
From: Jeff D. <jd...@ad...> - 2007-02-22 17:49:59
|
On Thu, Feb 22, 2007 at 12:54:04AM +0100, Blaisorblade wrote: > Looking into the code, I saw that: > 1) daemon_user_init is confusing because it saved any error into pri->fd, > instead of returning it to the caller. Yup. > Also, it would make sense to move this into daemon_open. The current behavior is on purpose. The thinking is that device initialization is equivalent to plugging a box into a switch. The device isn't up (and the box might in fact be turned off). Bringing the interface up comes later, as a separate step. You may be right, and we can talk about it, but the current code is no accident. > 2) connect_to_switch uses the same errno twice - it shouldn't, and we're > hitting this. um_interrupt is called with fd => 0 because errno is cleared > after printk (verified under debugging, I did not trace it fully to where > it's changing but it's sufficient for a patch anyway). We've seen this before, and I thought I made a pass over all of UML and fixed everything. Sigh. Jeff -- Work email - jdike at linux dot intel dot com |
From: Jeff D. <jd...@ad...> - 2007-02-22 20:08:01
|
On Thu, Feb 22, 2007 at 12:54:04AM +0100, Blaisorblade wrote: > I started uml_daemon as root by mistake, so /tmp/uml.ctl even if /dev/net/tun > was world-readable. Ok, I did it. This is the result (after trying many times > to do 'ifconfig eth0 up' inside UML): I don't see your irq stuff here when I try that, but there were multiple problems, which I hope are fixed by the patch below: style violations - this made the patch much bigger, and needs to be split out the user init routines now return success or failure some major cleaning of the failure paths through eth_configure device was never freed sysfs_unregister was never called, adding the same interface again successfully would result in nasty error messages device stuck in the devices list regardless, so adding the same interface again would fail the return from platform_device_register was not checked I moved the register_netdev call until almost the end of the function Let me know what you think about this. And about your thought of merging the _init into _open, my excuse for keeping them separate deserves some scrutiny. Every transport besides the switch does all of its work in _open, and the _init routine just initializes data. So, I would say that if a transport has separate plug-it-in and bring-it-up operations, maybe they should be separated more than they are now. If the switch is the odd one, maybe we should merge _init into _open. Jeff -- Work email - jdike at linux dot intel dot com Index: linux-2.6.18-mm/arch/um/drivers/daemon_user.c =================================================================== --- linux-2.6.18-mm.orig/arch/um/drivers/daemon_user.c 2007-02-22 12:54:02.000000000 -0500 +++ linux-2.6.18-mm/arch/um/drivers/daemon_user.c 2007-02-22 13:44:09.000000000 -0500 @@ -39,7 +39,7 @@ static struct sockaddr_un *new_addr(void sun = um_kmalloc(sizeof(struct sockaddr_un)); if(sun == NULL){ printk("new_addr: allocation of sockaddr_un failed\n"); - return(NULL); + return NULL; } sun->sun_family = AF_UNIX; memcpy(sun->sun_path, name, len); @@ -122,7 +122,7 @@ static int connect_to_switch(struct daem return err; } -static void daemon_user_init(void *data, void *dev) +static int daemon_user_init(void *data, void *dev) { struct daemon_data *pri = data; struct timeval tv; @@ -145,13 +145,16 @@ static void daemon_user_init(void *data, if(pri->fd < 0){ kfree(pri->local_addr); pri->local_addr = NULL; + return pri->fd; } + + return 0; } static int daemon_open(void *data) { struct daemon_data *pri = data; - return(pri->fd); + return pri->fd; } static void daemon_remove(void *data) @@ -175,12 +178,12 @@ int daemon_user_write(int fd, void *buf, { struct sockaddr_un *data_addr = pri->data_addr; - return(net_sendto(fd, buf, len, data_addr, sizeof(*data_addr))); + return net_sendto(fd, buf, len, data_addr, sizeof(*data_addr)); } static int daemon_set_mtu(int mtu, void *data) { - return(mtu); + return mtu; } const struct net_user_info daemon_user_info = { @@ -193,14 +196,3 @@ const struct net_user_info daemon_user_i .delete_address = NULL, .max_packet = MAX_PACKET - ETH_HEADER_OTHER }; - -/* - * Overrides for Emacs so that we follow Linus's tabbing style. - * Emacs will notice this stuff at the end of the file and automatically - * adjust the settings for this buffer only. This must remain at the end - * of the file. - * --------------------------------------------------------------------------- - * Local variables: - * c-file-style: "linux" - * End: - */ Index: linux-2.6.18-mm/arch/um/drivers/mcast_user.c =================================================================== --- linux-2.6.18-mm.orig/arch/um/drivers/mcast_user.c 2006-12-29 12:20:14.000000000 -0500 +++ linux-2.6.18-mm/arch/um/drivers/mcast_user.c 2007-02-22 13:45:00.000000000 -0500 @@ -39,15 +39,16 @@ static struct sockaddr_in *new_addr(char sin->sin_family = AF_INET; sin->sin_addr.s_addr = in_aton(addr); sin->sin_port = htons(port); - return(sin); + return sin; } -static void mcast_user_init(void *data, void *dev) +static int mcast_user_init(void *data, void *dev) { struct mcast_data *pri = data; pri->mcast_addr = new_addr(pri->addr, pri->port); pri->dev = dev; + return 0; } static int mcast_open(void *data) @@ -145,12 +146,12 @@ int mcast_user_write(int fd, void *buf, { struct sockaddr_in *data_addr = pri->mcast_addr; - return(net_sendto(fd, buf, len, data_addr, sizeof(*data_addr))); + return net_sendto(fd, buf, len, data_addr, sizeof(*data_addr)); } static int mcast_set_mtu(int mtu, void *data) { - return(mtu); + return mtu; } const struct net_user_info mcast_user_info = { Index: linux-2.6.18-mm/arch/um/drivers/net_kern.c =================================================================== --- linux-2.6.18-mm.orig/arch/um/drivers/net_kern.c 2007-02-19 11:52:41.000000000 -0500 +++ linux-2.6.18-mm/arch/um/drivers/net_kern.c 2007-02-22 14:53:00.000000000 -0500 @@ -325,30 +325,31 @@ static struct platform_driver uml_net_dr }; static int driver_registered; -static int eth_configure(int n, void *init, char *mac, - struct transport *transport) +static void eth_configure(int n, void *init, char *mac, + struct transport *transport) { struct uml_net *device; struct net_device *dev; struct uml_net_private *lp; int save, err, size; - size = transport->private_size + sizeof(struct uml_net_private) + - sizeof(((struct uml_net_private *) 0)->user); + /* + * This overallocates by sizeof(int) since the last field in + * uml_net_private is used to access the transport-specific + * data, so that user field is the first 4 bytes of the + * transport data. + */ + size = transport->private_size + sizeof(struct uml_net_private); device = kzalloc(sizeof(*device), GFP_KERNEL); if (device == NULL) { printk(KERN_ERR "eth_configure failed to allocate uml_net\n"); - return(1); + return; } INIT_LIST_HEAD(&device->list); device->index = n; - spin_lock(&devices_lock); - list_add(&device->list, &devices); - spin_unlock(&devices_lock); - setup_etheraddr(mac, device->mac); printk(KERN_INFO "Netdevice %d ", n); @@ -360,7 +361,7 @@ static int eth_configure(int n, void *in dev = alloc_etherdev(size); if (dev == NULL) { printk(KERN_ERR "eth_configure: failed to allocate device\n"); - return 1; + goto out_free_device; } lp = dev->priv; @@ -376,7 +377,8 @@ static int eth_configure(int n, void *in } device->pdev.id = n; device->pdev.name = DRIVER_NAME; - platform_device_register(&device->pdev); + if(platform_device_register(&device->pdev)) + goto out_free_netdev; SET_NETDEV_DEV(dev,&device->pdev.dev); /* If this name ends up conflicting with an existing registered @@ -386,31 +388,11 @@ static int eth_configure(int n, void *in snprintf(dev->name, sizeof(dev->name), "eth%d", n); device->dev = dev; + /* These just fill in a data structure, so there's no failure + * to be worried about. + */ (*transport->kern->init)(dev, init); - dev->mtu = transport->user->max_packet; - dev->open = uml_net_open; - dev->hard_start_xmit = uml_net_start_xmit; - dev->stop = uml_net_close; - dev->get_stats = uml_net_get_stats; - dev->set_multicast_list = uml_net_set_multicast_list; - dev->tx_timeout = uml_net_tx_timeout; - dev->set_mac_address = uml_net_set_mac; - dev->change_mtu = uml_net_change_mtu; - dev->ethtool_ops = ¨_net_ethtool_ops; - dev->watchdog_timeo = (HZ >> 1); - dev->irq = UM_ETH_IRQ; - - rtnl_lock(); - err = register_netdevice(dev); - rtnl_unlock(); - if (err) { - device->dev = NULL; - /* XXX: should we call ->remove() here? */ - free_netdev(dev); - return 1; - } - /* lp.user is the first four bytes of the transport data, which * has already been initialized. This structure assignment will * overwrite that, so we make sure that .user gets overwritten with @@ -438,12 +420,45 @@ static int eth_configure(int n, void *in lp->tl.function = uml_net_user_timer_expire; memcpy(lp->mac, device->mac, sizeof(lp->mac)); - if (transport->user->init) - (*transport->user->init)(&lp->user, dev); + if ((transport->user->init != NULL) && + ((*transport->user->init)(&lp->user, dev) != 0)) + goto out_unregister; set_ether_mac(dev, device->mac); + dev->mtu = transport->user->max_packet; + dev->open = uml_net_open; + dev->hard_start_xmit = uml_net_start_xmit; + dev->stop = uml_net_close; + dev->get_stats = uml_net_get_stats; + dev->set_multicast_list = uml_net_set_multicast_list; + dev->tx_timeout = uml_net_tx_timeout; + dev->set_mac_address = uml_net_set_mac; + dev->change_mtu = uml_net_change_mtu; + dev->ethtool_ops = ¨_net_ethtool_ops; + dev->watchdog_timeo = (HZ >> 1); + dev->irq = UM_ETH_IRQ; - return 0; + rtnl_lock(); + err = register_netdevice(dev); + rtnl_unlock(); + if (err) + goto out_undo_user_init; + + spin_lock(&devices_lock); + list_add(&device->list, &devices); + spin_unlock(&devices_lock); + + return; + +out_undo_user_init: + if (transport->user->init != NULL) + (*transport->user->remove)(&lp->user); +out_unregister: + platform_device_unregister(&device->pdev); +out_free_netdev: + free_netdev(dev); +out_free_device: ; + kfree(device); } static struct uml_net *find_device(int n) Index: linux-2.6.18-mm/arch/um/drivers/pcap_user.c =================================================================== --- linux-2.6.18-mm.orig/arch/um/drivers/pcap_user.c 2007-02-20 16:12:28.000000000 -0500 +++ linux-2.6.18-mm/arch/um/drivers/pcap_user.c 2007-02-22 13:47:04.000000000 -0500 @@ -18,7 +18,7 @@ #define PCAP_FD(p) (*(int *)(p)) -static void pcap_user_init(void *data, void *dev) +static int pcap_user_init(void *data, void *dev) { struct pcap_data *pri = data; pcap_t *p; @@ -28,11 +28,12 @@ static void pcap_user_init(void *data, v if(p == NULL){ printk("pcap_user_init : pcap_open_live failed - '%s'\n", errors); - return; + return -EINVAL; } pri->dev = dev; pri->pcap = p; + return 0; } static int pcap_open(void *data) @@ -42,19 +43,19 @@ static int pcap_open(void *data) int err; if(pri->pcap == NULL) - return(-ENODEV); + return -ENODEV; if(pri->filter != NULL){ err = dev_netmask(pri->dev, &netmask); if(err < 0){ printk("pcap_open : dev_netmask failed\n"); - return(-EIO); + return -EIO; } pri->compiled = um_kmalloc(sizeof(struct bpf_program)); if(pri->compiled == NULL){ printk("pcap_open : kmalloc failed\n"); - return(-ENOMEM); + return -ENOMEM; } err = pcap_compile(pri->pcap, @@ -63,18 +64,18 @@ static int pcap_open(void *data) if(err < 0){ printk("pcap_open : pcap_compile failed - '%s'\n", pcap_geterr(pri->pcap)); - return(-EIO); + return -EIO; } err = pcap_setfilter(pri->pcap, pri->compiled); if(err < 0){ printk("pcap_open : pcap_setfilter failed - '%s'\n", pcap_geterr(pri->pcap)); - return(-EIO); + return -EIO; } } - return(PCAP_FD(pri->pcap)); + return PCAP_FD(pri->pcap); } static void pcap_remove(void *data) @@ -114,11 +115,11 @@ int pcap_user_read(int fd, void *buffer, n = pcap_dispatch(pri->pcap, 1, handler, (u_char *) &hdata); if(n < 0){ printk("pcap_dispatch failed - %s\n", pcap_geterr(pri->pcap)); - return(-EIO); + return -EIO; } else if(n == 0) - return(0); - return(hdata.len); + return 0; + return hdata.len; } const struct net_user_info pcap_user_info = { @@ -131,14 +132,3 @@ const struct net_user_info pcap_user_inf .delete_address = NULL, .max_packet = MAX_PACKET - ETH_HEADER_OTHER }; - -/* - * Overrides for Emacs so that we follow Linus's tabbing style. - * Emacs will notice this stuff at the end of the file and automatically - * adjust the settings for this buffer only. This must remain at the end - * of the file. - * --------------------------------------------------------------------------- - * Local variables: - * c-file-style: "linux" - * End: - */ Index: linux-2.6.18-mm/arch/um/drivers/slip_user.c =================================================================== --- linux-2.6.18-mm.orig/arch/um/drivers/slip_user.c 2007-02-20 16:12:28.000000000 -0500 +++ linux-2.6.18-mm/arch/um/drivers/slip_user.c 2007-02-22 13:49:28.000000000 -0500 @@ -17,11 +17,12 @@ #include "os.h" #include "um_malloc.h" -void slip_user_init(void *data, void *dev) +static int slip_user_init(void *data, void *dev) { struct slip_data *pri = data; pri->dev = dev; + return 0; } static int set_up_tty(int fd) Index: linux-2.6.18-mm/arch/um/drivers/slirp_user.c =================================================================== --- linux-2.6.18-mm.orig/arch/um/drivers/slirp_user.c 2007-02-20 16:12:28.000000000 -0500 +++ linux-2.6.18-mm/arch/um/drivers/slirp_user.c 2007-02-22 13:50:28.000000000 -0500 @@ -15,11 +15,12 @@ #include "slip_common.h" #include "os.h" -void slirp_user_init(void *data, void *dev) +static int slirp_user_init(void *data, void *dev) { struct slirp_data *pri = data; pri->dev = dev; + return 0; } struct slirp_pre_exec_data { Index: linux-2.6.18-mm/arch/um/include/net_user.h =================================================================== --- linux-2.6.18-mm.orig/arch/um/include/net_user.h 2006-12-29 12:20:14.000000000 -0500 +++ linux-2.6.18-mm/arch/um/include/net_user.h 2007-02-22 13:51:11.000000000 -0500 @@ -14,7 +14,7 @@ #define UML_NET_VERSION (4) struct net_user_info { - void (*init)(void *, void *); + int (*init)(void *, void *); int (*open)(void *); void (*close)(int, void *); void (*remove)(void *); Index: linux-2.6.18-mm/arch/um/os-Linux/drivers/ethertap_user.c =================================================================== --- linux-2.6.18-mm.orig/arch/um/os-Linux/drivers/ethertap_user.c 2007-02-20 16:12:28.000000000 -0500 +++ linux-2.6.18-mm/arch/um/os-Linux/drivers/ethertap_user.c 2007-02-22 13:56:37.000000000 -0500 @@ -24,11 +24,12 @@ #define MAX_PACKET ETH_MAX_PACKET -void etap_user_init(void *data, void *dev) +static int etap_user_init(void *data, void *dev) { struct ethertap_data *pri = data; pri->dev = dev; + return 0; } struct addr_change { @@ -121,7 +122,7 @@ static int etap_tramp(char *dev, char *g n = os_read_file(control_me, &c, sizeof(c)); if(n != sizeof(c)){ printk("etap_tramp : read of status failed, err = %d\n", -n); - return(-EINVAL); + return -EINVAL; } if(c != 1){ printk("etap_tramp : uml_net failed\n"); @@ -132,7 +133,7 @@ static int etap_tramp(char *dev, char *g else if(!WIFEXITED(status) || (WEXITSTATUS(status) != 1)) printk("uml_net didn't exit with status 1\n"); } - return(err); + return err; } static int etap_open(void *data) @@ -142,18 +143,19 @@ static int etap_open(void *data) int data_fds[2], control_fds[2], err, output_len; err = tap_open_common(pri->dev, pri->gate_addr); - if(err) return(err); + if(err) + return err; err = os_pipe(data_fds, 0, 0); if(err < 0){ printk("data os_pipe failed - err = %d\n", -err); - return(err); + return err; } err = os_pipe(control_fds, 1, 0); if(err < 0){ printk("control os_pipe failed - err = %d\n", -err); - return(err); + return err; } err = etap_tramp(pri->dev_name, pri->gate_addr, control_fds[0], @@ -171,13 +173,13 @@ static int etap_open(void *data) if(err < 0){ printk("etap_tramp failed - err = %d\n", -err); - return(err); + return err; } pri->data_fd = data_fds[0]; pri->control_fd = control_fds[0]; iter_addresses(pri->dev, etap_open_addr, &pri->control_fd); - return(data_fds[0]); + return data_fds[0]; } static void etap_close(int fd, void *data) @@ -195,7 +197,7 @@ static void etap_close(int fd, void *dat static int etap_set_mtu(int mtu, void *data) { - return(mtu); + return mtu; } static void etap_add_addr(unsigned char *addr, unsigned char *netmask, @@ -204,7 +206,8 @@ static void etap_add_addr(unsigned char struct ethertap_data *pri = data; tap_check_ips(pri->gate_addr, addr); - if(pri->control_fd == -1) return; + if(pri->control_fd == -1) + return; etap_open_addr(addr, netmask, &pri->control_fd); } @@ -213,7 +216,8 @@ static void etap_del_addr(unsigned char { struct ethertap_data *pri = data; - if(pri->control_fd == -1) return; + if(pri->control_fd == -1) + return; etap_close_addr(addr, netmask, &pri->control_fd); } @@ -227,14 +231,3 @@ const struct net_user_info ethertap_user .delete_address = etap_del_addr, .max_packet = MAX_PACKET - ETH_HEADER_ETHERTAP }; - -/* - * Overrides for Emacs so that we follow Linus's tabbing style. - * Emacs will notice this stuff at the end of the file and automatically - * adjust the settings for this buffer only. This must remain at the end - * of the file. - * --------------------------------------------------------------------------- - * Local variables: - * c-file-style: "linux" - * End: - */ Index: linux-2.6.18-mm/arch/um/os-Linux/drivers/tuntap_user.c =================================================================== --- linux-2.6.18-mm.orig/arch/um/os-Linux/drivers/tuntap_user.c 2007-02-20 16:12:28.000000000 -0500 +++ linux-2.6.18-mm/arch/um/os-Linux/drivers/tuntap_user.c 2007-02-22 13:55:28.000000000 -0500 @@ -24,11 +24,12 @@ #define MAX_PACKET ETH_MAX_PACKET -void tuntap_user_init(void *data, void *dev) +static int tuntap_user_init(void *data, void *dev) { struct tuntap_data *pri = data; pri->dev = dev; + return 0; } static void tuntap_add_addr(unsigned char *addr, unsigned char *netmask, @@ -37,7 +38,8 @@ static void tuntap_add_addr(unsigned cha struct tuntap_data *pri = data; tap_check_ips(pri->gate_addr, addr); - if((pri->fd == -1) || pri->fixed_config) return; + if((pri->fd == -1) || pri->fixed_config) + return; open_addr(addr, netmask, pri->dev_name); } @@ -46,7 +48,8 @@ static void tuntap_del_addr(unsigned cha { struct tuntap_data *pri = data; - if((pri->fd == -1) || pri->fixed_config) return; + if((pri->fd == -1) || pri->fixed_config) + return; close_addr(addr, netmask, pri->dev_name); } @@ -83,7 +86,8 @@ static int tuntap_open_tramp(char *gate, pid = run_helper(tuntap_pre_exec, &data, argv, NULL); - if(pid < 0) return(-pid); + if(pid < 0) + return -pid; os_close_file(remote); @@ -114,16 +118,16 @@ static int tuntap_open_tramp(char *gate, cmsg = CMSG_FIRSTHDR(&msg); if(cmsg == NULL){ printk("tuntap_open_tramp : didn't receive a message\n"); - return(-EINVAL); + return -EINVAL; } if((cmsg->cmsg_level != SOL_SOCKET) || (cmsg->cmsg_type != SCM_RIGHTS)){ printk("tuntap_open_tramp : didn't receive a descriptor\n"); - return(-EINVAL); + return -EINVAL; } *fd_out = ((int *) CMSG_DATA(cmsg))[0]; os_set_exec_close(*fd_out, 1); - return(0); + return 0; } static int tuntap_open(void *data) @@ -135,7 +139,7 @@ static int tuntap_open(void *data) err = tap_open_common(pri->dev, pri->gate_addr); if(err < 0) - return(err); + return err; if(pri->fixed_config){ pri->fd = os_open_file("/dev/net/tun", @@ -143,7 +147,7 @@ static int tuntap_open(void *data) if(pri->fd < 0){ printk("Failed to open /dev/net/tun, err = %d\n", -pri->fd); - return(pri->fd); + return pri->fd; } memset(&ifr, 0, sizeof(ifr)); ifr.ifr_flags = IFF_TAP | IFF_NO_PI; @@ -160,7 +164,7 @@ static int tuntap_open(void *data) if(err < 0){ printk("tuntap_open : os_pipe failed - err = %d\n", -err); - return(err); + return err; } buffer = get_output_buffer(&len); @@ -175,7 +179,7 @@ static int tuntap_open(void *data) printk("%s", output); free_output_buffer(buffer); printk("tuntap_open_tramp failed - err = %d\n", -err); - return(err); + return err; } pri->dev_name = uml_strdup(buffer); @@ -187,7 +191,7 @@ static int tuntap_open(void *data) iter_addresses(pri->dev, open_addr, pri->dev_name); } - return(pri->fd); + return pri->fd; } static void tuntap_close(int fd, void *data) @@ -202,7 +206,7 @@ static void tuntap_close(int fd, void *d static int tuntap_set_mtu(int mtu, void *data) { - return(mtu); + return mtu; } const struct net_user_info tuntap_user_info = { @@ -215,14 +219,3 @@ const struct net_user_info tuntap_user_i .delete_address = tuntap_del_addr, .max_packet = MAX_PACKET }; - -/* - * Overrides for Emacs so that we follow Linus's tabbing style. - * Emacs will notice this stuff at the end of the file and automatically - * adjust the settings for this buffer only. This must remain at the end - * of the file. - * --------------------------------------------------------------------------- - * Local variables: - * c-file-style: "linux" - * End: - */ |
From: Blaisorblade <bla...@ya...> - 2007-03-04 19:14:30
|
On Thursday 22 February 2007 20:59, Jeff Dike wrote: > On Thu, Feb 22, 2007 at 12:54:04AM +0100, Blaisorblade wrote: > > I started uml_daemon as root by mistake, so /tmp/uml.ctl even if > > /dev/net/tun was world-readable. Ok, I did it. This is the result (after > > trying many times to do 'ifconfig eth0 up' inside UML): > > I don't see your irq stuff here when I try that, but there were > multiple problems, which I hope are fixed by the patch below: > style violations - this made the patch much bigger, and needs > to be split out Indeed, sigh. However I'm now convinced that if this just causes delays in patch merging without review benefit, it's not a good thing. It should be easy to split this away, and I have the patch about errno and am going to merge it (I hope to give a flush to my patch queue tomorrow). However you still add new ones: + if(platform_device_register(&device->pdev)) + goto out_free_netdev; > the user init routines now return success or failure Good. > some major cleaning of the failure paths through eth_configure > device was never freed > sysfs_unregister was never called, adding the same > interface again successfully would result in nasty error messages > device stuck in the devices list regardless, so adding > the same interface again would fail > the return from platform_device_register was not > checked > I moved the register_netdev call until almost the end > of the function Have you checked if other calls require that register_netdev was already called? > Let me know what you think about this. > > And about your thought of merging the _init into _open, my excuse for > keeping them separate deserves some scrutiny. Every transport besides > the switch does all of its work in _open, and the _init routine just > initializes data. So, I would say that if a transport has separate > plug-it-in and bring-it-up operations, maybe they should be separated > more than they are now. If the switch is the odd one, maybe we should > merge _init into _open. Hmm, I'm really not sure about what to do about this. However I'd leave that for a future cleanup and just add a comment about this. And the reason for which init now returns a value is that pcap_init _can_ fail, so the switch transport is not the only odd one. Another note: + /* + * This overallocates by sizeof(int) since the last field in + * uml_net_private is used to access the transport-specific + * data, so that user field is the first 4 bytes of the + * transport data. + */ + size = transport->private_size + sizeof(struct uml_net_private); There is a simple and standard solution in Linux style: struct uml_net_private { - int user[1]; + char user[0]; }; With fgrep [0] include/linux/* I found quite a few examples (like in include/linux/ethtool.h) - even if it is not fully ISO C conforming it's gcc conforming. A proof-of-concept (i.e. untested and applying on vanilla) patch for this is attached, with name remember-zero-array-net.diff. Another note: before of this patch, you should please apply the attached two ones. *) The first (net-mac-check-cleanup.diff) checks the validity of assigned MAC address, but to print a meaningful error message requires adding a local buffer. *) The second (net_kern-eth_configure...) allows avoiding this local buffer by moving code around. I think I'm excessively paranoid about these two patches and about not yet merging them, so please give a look and merge them. -- Inform me of my mistakes, so I can add them to my list! Paolo Giarrusso, aka Blaisorblade http://www.user-mode-linux.org/~blaisorblade |
From: Jeff D. <jd...@ad...> - 2007-03-05 16:00:15
|
On Sun, Mar 04, 2007 at 08:13:39PM +0100, Blaisorblade wrote: > Have you checked if other calls require that register_netdev was already > called? Just did, nothing else depends on it. > Hmm, I'm really not sure about what to do about this. However I'd leave that > for a future cleanup and just add a comment about this. > And the reason for which init now returns a value is that pcap_init _can_ > fail, so the switch transport is not the only odd one. Yeah, that slipped my mind. > struct uml_net_private { > - int user[1]; > + char user[0]; > }; Neat, that used to be illegal. > Another note: before of this patch, you should please apply the attached two > ones. > > *) The first (net-mac-check-cleanup.diff) checks the validity of assigned MAC > address, but to print a meaningful error message requires adding a local > buffer. > *) The second (net_kern-eth_configure...) allows avoiding this local buffer by > moving code around. > > I think I'm excessively paranoid about these two patches and about not yet > merging them, so please give a look and merge them. I just dropped them in - they look OK at a first glance. Jeff -- Work email - jdike at linux dot intel dot com |
From: Blaisorblade <bla...@ya...> - 2007-03-05 18:28:05
|
On Monday 05 March 2007 16:51, Jeff Dike wrote: > On Sun, Mar 04, 2007 at 08:13:39PM +0100, Blaisorblade wrote: > > Have you checked if other calls require that register_netdev was already > > called? > > Just did, nothing else depends on it. Fine. > > Hmm, I'm really not sure about what to do about this. However I'd leave > > that for a future cleanup and just add a comment about this. > > And the reason for which init now returns a value is that pcap_init _can_ > > fail, so the switch transport is not the only odd one. > > Yeah, that slipped my mind. > > > struct uml_net_private { > > - int user[1]; > > + char user[0]; > > }; > > Neat, that used to be illegal. If you use -pedantic (-ansi -std=c89 doesn't suffice) you'll learn that ISO C prohibits that; however, ethtool.h is quite a good example I guess, so GCC 3.2 supports that; and the code is anyway simpler that way. -- Inform me of my mistakes, so I can add them to my list! Paolo Giarrusso, aka Blaisorblade http://www.user-mode-linux.org/~blaisorblade Chiacchiera con i tuoi amici in tempo reale! http://it.yahoo.com/mail_it/foot/*http://it.messenger.yahoo.com |