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

[Ben Chan]

On Fri, Feb 24, 2017 at 10:55 AM, Aleksander Morgado <
aleksander at aleksander.es> wrote:

> On Thu, Feb 23, 2017 at 8:45 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 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?


> > ---
> >  src/mm-base-manager.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/mm-base-manager.c b/src/mm-base-manager.c
> > index 66096130..d5049b12 100644
> > --- a/src/mm-base-manager.c
> > +++ b/src/mm-base-manager.c
> > @@ -127,6 +127,8 @@ static MMDevice *
> >  find_device_by_physdev_uid (MMBaseManager *self,
> >                              const gchar   *physdev_uid)
> >  {
> > +    g_assert (physdev_uid != NULL);
> > +
> >      return g_hash_table_lookup (self->priv->devices, physdev_uid);
> >  }
> >
> > @@ -134,7 +136,12 @@ static MMDevice *
> >  find_device_by_kernel_device (MMBaseManager  *manager,
> >                                MMKernelDevice *kernel_device)
> >  {
> > -    return find_device_by_physdev_uid (manager,
> mm_kernel_device_get_physdev_uid (kernel_device));
> > +    const gchar *physdev_uid;
> > +
> > +    physdev_uid = mm_kernel_device_get_physdev_uid (kernel_device);
> > +    g_return_val_if_fail (physdev_uid != NULL, NULL);
> > +
> > +    return find_device_by_physdev_uid (manager, physdev_uid);
> >  }
> >
> >  /***********************************************************
> ******************/
> > @@ -277,7 +284,6 @@ device_added (MMBaseManager  *manager,
> >      /* Get the port's physical device's uid. All ports of the same
> physical
> >       * device will share the same uid. */
> >      physdev_uid = mm_kernel_device_get_physdev_uid (port);
> > -    g_assert (physdev_uid);
> >
> >      /* See if we already created an object to handle ports in this
> device */
> >      device = find_device_by_physdev_uid (manager, physdev_uid);
> > --
> > 2.11.0.483.g087da7b7c-goog
> >
>
>
>
> --
> Aleksander
> https://aleksander.es
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/modemmanager-devel/attachments/20170224/1ef3e80f/attachment-0001.html>