<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Fri, Feb 24, 2017 at 12:03 PM, Aleksander Morgado <<a href="mailto:aleksander@aleksander.es" target="_blank">aleksander@aleksander.es</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Fri, Feb 24, 2017 at 8:14 PM, Ben Chan <<a href="mailto:benchan@chromium.org">benchan@chromium.org</a>> 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. > </div></div>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. </blockquote><div> </div><div>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?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> * 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 <a href="https://aleksander.es" rel="noreferrer" target="_blank">https://aleksander.es</a> </blockquote></div> </div></div>