Re: [PATCH v5 02/10] plugins/udevng: Add support for PCIe MBIM modems * Parses through the sysfs tree and detects MBIM control nodes, net and AT nodes

Hi Muhammad!

On 11/27/25 12:49, Muhammad Asif wrote:
> ---
>   plugins/udevng.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 56 insertions(+)
> 
> diff --git a/plugins/udevng.c b/plugins/udevng.c
> index 18954d44..bae0be76 100644
> --- a/plugins/udevng.c
> +++ b/plugins/udevng.c
> @@ -1186,7 +1186,13 @@ static gboolean setup_mbim(struct modem_info *modem)
>   {
>   	const char *ctl = NULL, *net = NULL, *atcmd = NULL;
>   	GSList *list;
> +	const char *sub_subsystem = NULL, *type = NULL;
> +	char path[512], wwan_path[1024], sub_path[2048];
>   	char descriptors[PATH_MAX];
> +	struct udev *new_udev;
> +	struct udev_device *wwan_device, *sub_device;
> +	struct dirent *dir = NULL, *subdir = NULL;
> +	DIR *d = NULL, *sd = NULL;
>   
>   	DBG("%s [%s:%s]", modem->syspath, modem->vendor, modem->model);
>   
> @@ -1206,6 +1212,56 @@ static gboolean setup_mbim(struct modem_info *modem)
>   		else if (g_strcmp0(subsystem, "tty") == 0) {
>   			if (g_strcmp0(info->number, "02") == 0)
>   				atcmd = info->devnode;
> +		} else if (g_strcmp0(subsystem, "pci") == 0) {
> +			sprintf(path, "%s/wwan", udev_device_get_syspath(info->udev_device));

This about doubles the size of the function. I would suggest creating a 
new function (with a comment!) for traversing the subdirectories.

My understanding of this function is that if your modem has a "pci" 
directory, you're looking at pci/*/* where subsystem == wwan, and type 
== MBIM or AT. But a comment would help example that.



> +
> +			d = opendir(path);
> +			if (!d)
> +				return FALSE;
> +
> +			new_udev = udev_new();

This can fail, and you should probably check for NULL here.


> +
> +			while ((dir = readdir(d))) {
> +				if (g_strcmp0(dir->d_name, ".") == 0 ||
> +					g_strcmp0(dir->d_name, "..") == 0)
> +					continue;
> +

Glib provides wrappers for walking a directory:
<https://docs.gtk.org/glib/file-utils.html>
I suggest using g_dir_open() and friends instead of readdir(), etc. For 
example, g_dir_read_name skips over '.' and '..' already. You can see 
usage of it in src/plugin.c.

In addition, you've got some buffers above that may or may not be okay 
depending on what's in sysfs; I would assume you wouldn't normally hit 
PATH_MAX and overflow the buffer, but it'd be safer to just allocate 
memory using g_build_filename(), g_strconcat(), or similar.


> +				sprintf(wwan_path, "%s/%s", path, dir->d_name);
 > +> +				wwan_device = udev_device_new_from_syspath(new_udev,
> +									wwan_path);

This can also fail, and NULL should be checked for. Same issue below for 
sub_device.


> +				net = udev_device_get_sysname(wwan_device);
> +
> +				/* Parse all the subdirectories now */
> +				sd = opendir(wwan_path);
> +				while ((subdir = readdir(sd))) {
> +					if (subdir->d_type != DT_DIR)
> +						continue;
> +

If you decide to use GDir, here you'd do something like:
sub_path = g_build_filename(wwan_path, subdir, NULL);
if (g_file_test(sub_path, G_FILE_TEST_IS_DIR)) {
   g_free(sub_path);
   continue;
}


> +					if (g_strcmp0(subdir->d_name, ".") == 0 ||
> +						g_strcmp0(subdir->d_name, "..") == 0)
> +						continue;
> +
> +					sprintf(sub_path, "%s/%s", wwan_path, subdir->d_name);
> +
> +					sub_device = udev_device_new_from_syspath(new_udev,
> +										sub_path);
> +					sub_subsystem = udev_device_get_subsystem(sub_device);
> +
> +					if (g_strcmp0(sub_subsystem, "wwan") == 0) {
> +						type = udev_device_get_sysattr_value(sub_device, "type");
> +
> +						if (g_strcmp0(type, "MBIM") == 0)
> +							ctl = udev_device_get_devnode(sub_device);
> +						else if (g_strcmp0(type, "AT") == 0)
> +							atcmd = udev_device_get_devnode(sub_device);
> +					}

sub_device should be udev_device_unref'd here, otherwise it's leaked.

> +				}
> +				closedir(sd);

wwan_device should be udev_device_unref'd here, as well.


> +				break;
> +			}
 > +			closedir(d);> +			udev_unref(new_udev);
 >   		}>   	}
>