Re: [PATCH 1/2] Fix Use hashtable to record udev path

[Denis Kenzior <denkenz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4137 bytes --]

Hi Zhenhua,

> Sometimes, Udev device 'remove' event could not report correct parent
> node of current udev_device. Current code replies on the devpath
> attached on the parent node to find modem and then remove it.
> 
> This fix is to change the way to store the devpath info into a
> hashtable. So that we search hashtable to get devpath and remove the
> modem.
> ---
>  plugins/udev.c |   59
>  +++++++++++++++++++++++++++++++++++++------------------ 1 files changed,
>  40 insertions(+), 19 deletions(-)
> 
> diff --git a/plugins/udev.c b/plugins/udev.c
> index 964ac65..6850bf9 100644
> --- a/plugins/udev.c
> +++ b/plugins/udev.c
> @@ -36,6 +36,7 @@
>  #include <ofono/log.h>
> 
>  static GSList *modem_list = NULL;
> +static GHashTable *devpath_list = NULL;
> 
>  static struct ofono_modem *find_modem(const char *devpath)
>  {
> @@ -258,7 +259,7 @@ static void add_modem(struct udev_device *udev_device)
>  {
>  	struct ofono_modem *modem;
>  	struct udev_device *parent;
> -	const char *devpath, *driver;
> +	const char *devpath, *curpath, *driver;
> 
>  	parent = udev_device_get_parent(udev_device);
>  	if (parent == NULL)
> @@ -294,6 +295,12 @@ static void add_modem(struct udev_device *udev_device)
>  		modem_list = g_slist_prepend(modem_list, modem);
>  	}
> 
> +	curpath = udev_device_get_devpath(udev_device);
> +	if (curpath == NULL)
> +		return;
> +
> +	g_hash_table_insert(devpath_list, g_strdup(curpath), g_strdup(devpath));
> +
>  	if (g_strcmp0(driver, "mbm") == 0)
>  		add_mbm(modem, udev_device);
>  	else if (g_strcmp0(driver, "hso") == 0)
> @@ -306,30 +313,28 @@ static void add_modem(struct udev_device
>  *udev_device) add_novatel(modem, udev_device);
>  }
> 
> +static gboolean devpath_remove(gpointer key, gpointer value, gpointer
>  user_data) +{
> +	const char *path = value;
> +	const char *devpath = user_data;
> +
> +	if (!g_strcmp0(path, devpath))
> +		return TRUE;

How about a simple return g_str_equals here?

> +
> +	return FALSE;
> +}
> +
>  static void remove_modem(struct udev_device *udev_device)
>  {
>  	struct ofono_modem *modem;
> -	struct udev_device *parent;
> -	const char *devpath, *driver = NULL;
> +	const char *curpath = udev_device_get_devpath(udev_device);
> +	char *devpath, *remove;
> 
> -	parent = udev_device_get_parent(udev_device);
> -	if (parent == NULL)
> +	if (curpath == NULL)
>  		return;
> 
> -	driver = get_driver(parent);
> -	if (driver == NULL) {
> -		parent = udev_device_get_parent(parent);
> -		driver = get_driver(parent);
> -		if (driver == NULL) {
> -			parent = udev_device_get_parent(parent);
> -			driver = get_driver(parent);
> -			if (driver == NULL)
> -				return;
> -		}
> -	}
> -
> -	devpath = udev_device_get_devpath(parent);
> -	if (devpath == NULL)
> +	devpath = g_hash_table_lookup(devpath_list, curpath);
> +	if (!devpath)
>  		return;
> 
>  	modem = find_modem(devpath);
> @@ -339,6 +344,12 @@ static void remove_modem(struct udev_device
>  *udev_device) modem_list = g_slist_remove(modem_list, modem);
> 
>  	ofono_modem_remove(modem);
> +
> +	remove = g_strdup(devpath);
> +
> +	g_hash_table_foreach_remove(devpath_list, devpath_remove, remove);
> +
> +	g_free(remove);
>  }
> 
>  static void enumerate_devices(struct udev *context)
> @@ -443,6 +454,13 @@ static void udev_start(void)
> 
>  static int udev_init(void)
>  {
> +	devpath_list = g_hash_table_new_full(g_str_hash, g_str_equal,
> +						g_free, g_free);
> +	if (!devpath_list) {
> +		ofono_error("Failed to create udev path list");
> +		return -ENOMEM;
> +	}
> +

You now need to take care of freeing the devpath_list on any further error 
conditions that might occur, otherwise you leak memory.

>  	udev_ctx = udev_new();
>  	if (udev_ctx == NULL) {
>  		ofono_error("Failed to create udev context");
> @@ -483,6 +501,9 @@ static void udev_exit(void)
>  	g_slist_free(modem_list);
>  	modem_list = NULL;
> 
> +	g_hash_table_destroy(devpath_list);
> +	devpath_list = NULL;
> +
>  	if (udev_ctx == NULL)
>  		return;
> 

Regards,
-Denis