| Message ID | 20200224103406.1894923-1-stefanha@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | qemu/queue.h: clear linked list pointers on remove | expand |
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
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(-)