RE: [PATCH 0/6] Remove usage of list iterator past the loop body
From: Dan Carpenter > Sent: 07 March 2022 15:01 > > Updating this API is risky because some places rely on the old behavior > and not all of them have been updated. Here are some additional places > you might want to change. I really can't help thinking that trying to merge this patch is actually impossible. It affects far too many different parts of the tree. Since (I believe) this is a doubly linked list with forwards and backwards pointers that point to a 'node' (not that there is a nice comment to that effect in the header - and there are lots of ways to do linked lists) the 'head' pretty much has to be a 'node'. I'd write the following new defines (but I might be using the old names here): list_first(head, field) First item, NULL if empty. list_last(head, field) Last item NULL if empty. list_next(head, item, field) Item after 'item', NULL if last. list_prev(head, item. field) Item before 'item', NULL if first. You get (something like): #define list_first(head, field) \ head->next == ? NULL : list_item(head->next, field) (probably needs typeof(item) from somewhere). The iterator loop is then just: #define loop_iterate(item, head, field) \ for (item = list_first(head, field); item; \ item = list_next(head, item, field) I'm not sure, but making the 'head' be a structure that contains a single member that is a 'node' might help type checking. Then all the code that uses the current defines can slowly be moved over (probably a couple of releases) before the existing defines are deleted. That should simplify all the open-coded search loops that are just as likely to be buggy (possibly more so). David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Re: [PATCH 0/6] Remove usage of list iterator past the loop body
Updating this API is risky because some places rely on the old behavior and not all of them have been updated. Here are some additional places you might want to change. drivers/usb/host/uhci-q.c:466 link_async() warn: iterator used outside loop: 'pqh' drivers/infiniband/core/mad.c:968 ib_get_rmpp_segment() warn: iterator used outside loop: 'mad_send_wr->cur_seg' drivers/opp/debugfs.c:208 opp_migrate_dentry() warn: iterator used outside loop: 'new_dev' drivers/staging/greybus/audio_codec.c:602 gbcodec_mute_stream() warn: iterator used outside loop: 'module' drivers/staging/media/atomisp/pci/atomisp_acc.c:508 atomisp_acc_load_extensions() warn: iterator used outside loop: 'acc_fw' drivers/perf/thunderx2_pmu.c:814 tx2_uncore_pmu_init_dev() warn: iterator used outside loop: 'rentry' drivers/gpu/drm/nouveau/nvkm/engine/device/ctrl.c:111 nvkm_control_mthd_pstate_attr() warn: iterator used outside loop: 'pstate' drivers/gpu/drm/panfrost/panfrost_mmu.c:203 panfrost_mmu_as_get() warn: iterator used outside loop: 'lru_mmu' drivers/media/usb/uvc/uvc_v4l2.c:885 uvc_ioctl_enum_input() warn: iterator used outside loop: 'iterm' drivers/media/usb/uvc/uvc_v4l2.c:896 uvc_ioctl_enum_input() warn: iterator used outside loop: 'iterm' drivers/scsi/dc395x.c:3596 device_alloc() warn: iterator used outside loop: 'p' drivers/net/ethernet/mellanox/mlx4/alloc.c:379 __mlx4_alloc_from_zone() warn: iterator used outside loop: 'curr_node' fs/ocfs2/dlm/dlmdebug.c:573 lockres_seq_start() warn: iterator used outside loop: 'res' This patchset fixes 3 bugs. Initially when it's merged it's probably going to introduce some bugs because there are likely other places which rely on the old behavior. In an ideal world, with the new API the compiler would warn about uninitialized variables, but unfortunately that warning is disabled by default so we still have to rely on kbuild/Clang/Smatch to find the bugs. But hopefully the new API encourages people to write clearer code so it prevents bugs in the long run. regards, dan carpenter
