Re: [PATCH] xc: deal with xen/evtchn and xen/gntdev device names

[Jeremy Fitzhardinge <jeremy@goop.org>]

On 06/02/2010 02:29 AM, Ian Campbell wrote:
> On Tue, 2010-06-01 at 17:35 +0100, Jeremy Fitzhardinge wrote:
>   
>> On 06/01/2010 12:56 AM, Ian Campbell wrote:
>>     
>>> Does any of this logic really belong in libxc in the first place? 
>>>   
>>>       
>> No.  It's a relic of a simpler time.
>>
>>     
>>> Can we not just rip it all out and make it the distro/platform's
>>> responsibility to ensure these devices exist and are correct? Perhaps
>>> that might involve shipping some default/example udev rules instead.
>>>   
>>>       
>> I only kept it because I didn't want to break any existing installs, but
>> updating the kernel's name is going to do that anyway.  Unfortunately
>> the current libxc code is broken in the worst possible way - it will
>> unlink an existing correct device and then fail to replace it - so even
>> if the system has set it up correctly it will still screw things up.
>>
>> I think we're just going to have to have a flag day and fix kernel,
>> libxc and udev (assuming it needs it) all at once...
>>     
> I don't think we need a flag day. It seems like we already ship a udev
> rule (in tools/hotplug/Linux/xen-backend.rules) which correctly
> created /dev/xen/evtchn with the current kernel and which is apparently
> unnecessary (but harmless) with the proposed kernel change.
>   

My main concern is that an old libxc will screw anyone with new kernel
and udev.

> I removed all the mknod stuff from my tools and things kept working as
> expected.
>
> So lets try the following. I think once it has settled down in unstable
> we could consider it for inclusion in the stable branch.
>
> --- 
> tools: assume that special Xen devices have been created by the platform
>
> Remove all the magic surrounding the special Xen devices in Linux
> specific code whereby we attempt to figure out what the correct
> major:minor number is and check the the existing device has these
> numbers etc. In 2010 we really should be able to trust that the platform
> has created the devices correctly or provide correct configuration
> settings such that they are without resorting to tearing down the
> platform configured state and rebuilding it.
>
> tools/hotplug/Linux/xen-backend.rules already contains the necessary
> udev rules to create /dev/xen/evtchn and friends in the correct place.
>
>   

Looks fine to me.

    J

> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>
> diff -r 2f765c9825b2 -r 2962c9c4d26f tools/blktap/drivers/blktapctrl_linux.c
> --- a/tools/blktap/drivers/blktapctrl_linux.c	Tue Jun 01 10:40:06 2010 +0100
> +++ b/tools/blktap/drivers/blktapctrl_linux.c	Wed Jun 02 10:01:09 2010 +0100
> @@ -79,31 +79,11 @@
>  
>  int blktap_interface_open(void)
>  {
> -	char *devname;
> -	int ret;
>  	int ctlfd;
>  
> -	/* Attach to blktap0 */
> -	if (asprintf(&devname,"%s/%s0", BLKTAP_DEV_DIR, BLKTAP_DEV_NAME) == -1)
> -		goto open_failed;
> -
> -	ret = xc_find_device_number("blktap0");
> -	if (ret < 0) {
> -		DPRINTF("couldn't find device number for 'blktap0'\n");
> -		goto open_failed;
> -	}
> -
> -	blktap_major = major(ret);
> -	make_blktap_dev(devname,blktap_major, 0);
> -
> -	ctlfd = open(devname, O_RDWR);
> -	if (ctlfd == -1) {
> +	ctlfd = open(BLKTAP_DEV_DIR "/" BLKTAP_DEV_NAME "0", O_RDWR);
> +	if (ctlfd == -1)
>  		DPRINTF("blktap0 open failed\n");
> -		goto open_failed;
> -	}
>  
>  	return ctlfd;
> -
> -open_failed:
> -	return -1;
>  }
> diff -r 2f765c9825b2 -r 2962c9c4d26f tools/libxc/xc_linux.c
> --- a/tools/libxc/xc_linux.c	Tue Jun 01 10:40:06 2010 +0100
> +++ b/tools/libxc/xc_linux.c	Wed Jun 02 10:01:09 2010 +0100
> @@ -316,126 +316,11 @@
>                        (unsigned long)hypercall);
>  }
>  
> -#define MTAB "/proc/mounts"
> -#define MAX_PATH 255
> -#define _STR(x) #x
> -#define STR(x) _STR(x)
> -
> -static int find_sysfsdir(char *sysfsdir)
> -{
> -    FILE *fp;
> -    char type[MAX_PATH + 1];
> -
> -    if ( (fp = fopen(MTAB, "r")) == NULL )
> -        return -1;
> -
> -    while ( fscanf(fp, "%*s %"STR(MAX_PATH)"s %"STR(MAX_PATH)"s %*s %*d %*d\n",
> -                   sysfsdir, type) == 2 )
> -        if ( strncmp(type, "sysfs", 5) == 0 )
> -            break;
> -
> -    fclose(fp);
> -
> -    return ((strncmp(type, "sysfs", 5) == 0) ? 0 : -1);
> -}
> -
> -int xc_find_device_number(const char *name)
> -{
> -    FILE *fp;
> -    int i, major, minor;
> -    char sysfsdir[MAX_PATH + 1];
> -    static char *classlist[] = { "xen", "misc" };
> -
> -    for ( i = 0; i < (sizeof(classlist) / sizeof(classlist[0])); i++ )
> -    {
> -        if ( find_sysfsdir(sysfsdir) < 0 )
> -            goto not_found;
> -
> -        /* <base>/class/<classname>/<devname>/dev */
> -        strncat(sysfsdir, "/class/", MAX_PATH);
> -        strncat(sysfsdir, classlist[i], MAX_PATH);
> -        strncat(sysfsdir, "/", MAX_PATH);
> -        strncat(sysfsdir, name, MAX_PATH);
> -        strncat(sysfsdir, "/dev", MAX_PATH);
> -
> -        if ( (fp = fopen(sysfsdir, "r")) != NULL )
> -            goto found;
> -    }
> -
> - not_found:
> -    errno = -ENOENT;
> -    return -1;
> -
> - found:
> -    if ( fscanf(fp, "%d:%d", &major, &minor) != 2 )
> -    {
> -        fclose(fp);
> -        goto not_found;
> -    }
> -
> -    fclose(fp);
> -
> -    return makedev(major, minor);
> -}
> -
> -#define DEVXEN "/dev/xen"
> -
> -static int make_dev_xen(void)
> -{
> -    if ( mkdir(DEVXEN, 0755) != 0 )
> -    {
> -        struct stat st;
> -        if ( (stat(DEVXEN, &st) != 0) || !S_ISDIR(st.st_mode) )
> -            return -1;
> -    }
> -
> -    return 0;
> -}
> -
> -static int xendev_open(const char *dev)
> -{
> -    int fd, devnum;
> -    struct stat st;
> -    char *devname, *devpath;
> -
> -    devname = devpath = NULL;
> -    fd = -1;
> -
> -    if ( asprintf(&devname, "xen!%s", dev) == 0 )
> -        goto fail;
> -
> -    if ( asprintf(&devpath, "%s/%s", DEVXEN, dev) == 0 )
> -        goto fail;
> -
> -    devnum = xc_find_device_number(dev);
> -    if ( devnum == -1 )
> -        devnum = xc_find_device_number(devname);
> -
> -    /*
> -     * If we know what the correct device is and the path doesn't exist or 
> -     * isn't a device, then remove it so we can create the device.
> -     */
> -    if ( (devnum != -1) &&
> -         ((stat(devpath, &st) != 0) || !S_ISCHR(st.st_mode)) )
> -    {
> -        unlink(devpath);
> -        if ( (make_dev_xen() == -1) ||
> -             (mknod(devpath, S_IFCHR|0600, devnum) != 0) )
> -            goto fail;
> -    }
> -
> -    fd = open(devpath, O_RDWR);
> -
> - fail:
> -    free(devname);
> -    free(devpath);
> -
> -    return fd;
> -}
> +#define DEVXEN "/dev/xen/"
>  
>  int xc_evtchn_open(void)
>  {
> -    return xendev_open("evtchn");
> +    return open(DEVXEN "evtchn", O_RDWR);
>  }
>  
>  int xc_evtchn_close(int xce_handle)
> @@ -551,7 +436,7 @@
>  
>  int xc_gnttab_open(xc_interface *xch)
>  {
> -    return xendev_open("gntdev");
> +    return open(DEVXEN "gntdev", O_RDWR);
>  }
>  
>  int xc_gnttab_close(xc_interface *xch, int xcg_handle)
>
>
>