QLIST_REMOVE() and friends leave dangling linked list pointers in the node that
was removed.  This makes it impossible to decide whether a node is currently in
a list or not.  It also makes debugging harder.

Based-on: 20200222085030.1760640-1-stefanha@redhat.com
          ("[PULL 00/31] Block patches")

Stefan Hajnoczi (2):
  qemu/queue.h: clear linked list pointers on remove
  aio-posix: remove confusing QLIST_SAFE_REMOVE()

 include/qemu/queue.h | 19 +++++++++++++++----
 util/aio-posix.c     |  2 +-
 2 files changed, 16 insertions(+), 5 deletions(-)

-- 
2.24.1

Patchew URL: https://patchew.org/QEMU/20200224103406.1894923-1-stefanha@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [PATCH 0/2] qemu/queue.h: clear linked list pointers on remove
Message-id: 20200224103406.1894923-1-stefanha@redhat.com
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
56b6529 aio-posix: remove confusing QLIST_SAFE_REMOVE()
f913b24 qemu/queue.h: clear linked list pointers on remove

=== OUTPUT BEGIN ===
1/2 Checking commit f913b2430ad3 (qemu/queue.h: clear linked list pointers on remove)
ERROR: do not use assignment in if condition
#65: FILE: include/qemu/queue.h:314:
+    if (((head)->sqh_first = elm->field.sqe_next) == NULL)              \

total: 1 errors, 0 warnings, 59 lines checked

Patch 1/2 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

2/2 Checking commit 56b6529b1894 (aio-posix: remove confusing QLIST_SAFE_REMOVE())
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200224103406.1894923-1-stefanha@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

On Mon, Feb 24, 2020 at 02:55:33AM -0800, no-reply@patchew.org wrote:
> === OUTPUT BEGIN ===
> 1/2 Checking commit f913b2430ad3 (qemu/queue.h: clear linked list pointers on remove)
> ERROR: do not use assignment in if condition
> #65: FILE: include/qemu/queue.h:314:
> +    if (((head)->sqh_first = elm->field.sqe_next) == NULL)              \
> 
> total: 1 errors, 0 warnings, 59 lines checked

The same pattern is used elsewhere in this file.  This code comes from
BSD and doesn't comply with QEMU's coding style.

Stefan

On 2/24/20 12:39 PM, Stefan Hajnoczi wrote:
> On Mon, Feb 24, 2020 at 02:55:33AM -0800, no-reply@patchew.org wrote:
>> === OUTPUT BEGIN ===
>> 1/2 Checking commit f913b2430ad3 (qemu/queue.h: clear linked list pointers on remove)
>> ERROR: do not use assignment in if condition
>> #65: FILE: include/qemu/queue.h:314:
>> +    if (((head)->sqh_first = elm->field.sqe_next) == NULL)              \
>>
>> total: 1 errors, 0 warnings, 59 lines checked
> 
> The same pattern is used elsewhere in this file.  This code comes from
> BSD and doesn't comply with QEMU's coding style.

Checkpatch is right, assigning out of the if statement makes the review 
easier, and we can avoid the 'elm' null deref:

#define QSIMPLEQ_REMOVE_HEAD(head, field) do { \
-    if (((head)->sqh_first = (head)->sqh_first->field.sqe_next) == NULL)\
+    typeof((head)->sqh_first) elm = (head)->sqh_first; \
+    (head)->sqh_first = elm->field.sqe_next; \
+    if (elm == NULL) { \
          (head)->sqh_last = &(head)->sqh_first; \
+    } else { \
+        elm->field.sqe_next = NULL; \
+    } \
  } while (/*CONSTCOND*/0)

On Mon, Feb 24, 2020 at 12:54:37PM +0100, Philippe Mathieu-Daudé wrote:
> On 2/24/20 12:39 PM, Stefan Hajnoczi wrote:
> > On Mon, Feb 24, 2020 at 02:55:33AM -0800, no-reply@patchew.org wrote:
> > > === OUTPUT BEGIN ===
> > > 1/2 Checking commit f913b2430ad3 (qemu/queue.h: clear linked list pointers on remove)
> > > ERROR: do not use assignment in if condition
> > > #65: FILE: include/qemu/queue.h:314:
> > > +    if (((head)->sqh_first = elm->field.sqe_next) == NULL)              \
> > > 
> > > total: 1 errors, 0 warnings, 59 lines checked
> > 
> > The same pattern is used elsewhere in this file.  This code comes from
> > BSD and doesn't comply with QEMU's coding style.
> 
> Checkpatch is right, assigning out of the if statement makes the review
> easier, and we can avoid the 'elm' null deref:

The rest of the file uses if ((a = b) == NULL), so making it
inconsistent in this one instance isn't very satisfying.

> #define QSIMPLEQ_REMOVE_HEAD(head, field) do { \
> -    if (((head)->sqh_first = (head)->sqh_first->field.sqe_next) == NULL)\
> +    typeof((head)->sqh_first) elm = (head)->sqh_first; \
> +    (head)->sqh_first = elm->field.sqe_next; \
> +    if (elm == NULL) { \

The previous line would have segfaulted if elm was NULL so this check
doesn't make sense.

This macro assumes there is at least one element in the list.

The point of the check is to fix up the sqh_last pointer in the head
when the final element is removed from the list.

>          (head)->sqh_last = &(head)->sqh_first; \
> +    } else { \
> +        elm->field.sqe_next = NULL; \
> +    } \
>  } while (/*CONSTCOND*/0)

On 2/25/20 10:05 AM, Stefan Hajnoczi wrote:
> On Mon, Feb 24, 2020 at 12:54:37PM +0100, Philippe Mathieu-Daudé wrote:
>> On 2/24/20 12:39 PM, Stefan Hajnoczi wrote:
>>> On Mon, Feb 24, 2020 at 02:55:33AM -0800, no-reply@patchew.org wrote:
>>>> === OUTPUT BEGIN ===
>>>> 1/2 Checking commit f913b2430ad3 (qemu/queue.h: clear linked list pointers on remove)
>>>> ERROR: do not use assignment in if condition
>>>> #65: FILE: include/qemu/queue.h:314:
>>>> +    if (((head)->sqh_first = elm->field.sqe_next) == NULL)              \
>>>>
>>>> total: 1 errors, 0 warnings, 59 lines checked
>>>
>>> The same pattern is used elsewhere in this file.  This code comes from
>>> BSD and doesn't comply with QEMU's coding style.
>>
>> Checkpatch is right, assigning out of the if statement makes the review
>> easier, and we can avoid the 'elm' null deref:
> 
> The rest of the file uses if ((a = b) == NULL), so making it
> inconsistent in this one instance isn't very satisfying.
> 
>> #define QSIMPLEQ_REMOVE_HEAD(head, field) do { \
>> -    if (((head)->sqh_first = (head)->sqh_first->field.sqe_next) == NULL)\
>> +    typeof((head)->sqh_first) elm = (head)->sqh_first; \
>> +    (head)->sqh_first = elm->field.sqe_next; \
>> +    if (elm == NULL) { \
> 
> The previous line would have segfaulted if elm was NULL so this check
> doesn't make sense.
> 
> This macro assumes there is at least one element in the list.

Ah good point, thanks.

> 
> The point of the check is to fix up the sqh_last pointer in the head
> when the final element is removed from the list.
> 
>>           (head)->sqh_last = &(head)->sqh_first; \
>> +    } else { \
>> +        elm->field.sqe_next = NULL; \
>> +    } \
>>   } while (/*CONSTCOND*/0)

On Mon, Feb 24, 2020 at 10:34:04AM +0000, Stefan Hajnoczi wrote:
> QLIST_REMOVE() and friends leave dangling linked list pointers in the node that
> was removed.  This makes it impossible to decide whether a node is currently in
> a list or not.  It also makes debugging harder.
> 
> Based-on: 20200222085030.1760640-1-stefanha@redhat.com
>           ("[PULL 00/31] Block patches")
> 
> Stefan Hajnoczi (2):
>   qemu/queue.h: clear linked list pointers on remove
>   aio-posix: remove confusing QLIST_SAFE_REMOVE()
> 
>  include/qemu/queue.h | 19 +++++++++++++++----
>  util/aio-posix.c     |  2 +-
>  2 files changed, 16 insertions(+), 5 deletions(-)
> 
> -- 
> 2.24.1
> 

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan