[PATCH 2/3] base-manager: handle NULL physical device more gracefully

[Ben Chan]

On Fri, Feb 24, 2017 at 12:03 PM, Aleksander Morgado <
aleksander at aleksander.es> wrote:

> On Fri, Feb 24, 2017 at 8:14 PM, Ben Chan <benchan at chromium.org> wrote:
> >> > find_device_by_physdev_uid() expects a non-NULL UID of the physical
> >> > device. However, mm_kernel_device_get_physdev_uid() could potentially
> >> > return NULL on MMKernelDeviceUdev if find_physical_gudevdevice() in
> >> > mm-kernel-device-udev.c fails to find the physical device. When a NULL
> >> > physical device UID is passed to find_device_by_physdev_uid() and used
> >> > in g_hash_table_lookup(), it leads to a crash in g_str_hash(), which
> is
> >> > a bit obscure.
> >> >
> >> > This patch updates find_device_by_physdev_uid() to handle the case
> when
> >> > mm_kernel_device_get_physdev_uid() fails to return a valid UID, and
> also
> >> > moves the assertion on non-NULL UID to find_device_by_physdev_uid() in
> >> > order to ensure find_device_by_physdev_uid() is properly used by
> future
> >> > callers.
> >>
> >> I'm not sure I agree with this patch. We do want to enforce non-NULL
> >> UIDs, as that UID (it previously was the "physdev sysfs path" but
> >> doesn't have to now) is used to group ports of the same device
> >> together. If the device exposes a single port and has no parent device
> >> per se, the sysfs path of the port itself may be used as UID, as
> >> there's no grouping to be done.
> >>
> >> What we should make sure is that mm_kernel_device_get_physdev_uid()
> >> never returns NULL, and so the g_assert() is correctly in place.
> >>
> >> What do you think about these comments?
> >>
> >
> > If that's the expectation, the g_assert() should be placed inside
> > mm_kernel_device_get_physdev_uid () instead of the call sites of
> > mm_kernel_device_get_physdev_uid (), which is easier to omit.
> >
>
> I agree, yes.
>
> > I can update the patch to move the g_assert () and modify the code to
> > provide a fallback value of physdev uid to ensure it's never NULL. WDYT?
>
> Yes, that is a good idea.
>
> Something like this:
>
>  * If user-provided UID via "self->priv->properties", return that one.
>  * Otherwise, if user-provided UID via "ID_MM_PHYSDEV_UID", return that
> one.
>

I believe that's what MMKernelDeviceGeneric currently does. However,
MMKernelDeviceUdev tries to get ID_MM_PHYSDEV_UID on self->priv->physdev
instead of self->priv->device. That's first discrepancy I noticed. Is that
expected?


>  * Otherwise, if physdev is valid, return physdev sysfs path. This
> will be the most common case.
>  * Otherwise, if self->priv->device is set, return port sysfs path
> (kernel_device_get_sysfs_path()).
>  * Otherwise, the very unlikely case, just return port name.
>
> This would be the same order applied in the "generic" MMKernelDevice
> backend, but here we do require sysfs path of the port as a mandatory
> parameter and therefore the last very unlikely case above wouldn't
> apply.
>
> What do you think?
>
> --
> Aleksander
> https://aleksander.es
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/modemmanager-devel/attachments/20170224/c61243a8/attachment.html>