Linux Virtualization - Mar 2023 - [PATCH 3/3] virtio_ring: Use const to annotate read-only pointer params

On 2023-03-08 a.m.9:28, Michael S. Tsirkin wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Wed, Mar 08, 2023 at 09:19:38AM -0500, Feng Liu wrote:
>>
>>
>> On 2023-03-08 a.m.9:16, Michael S. Tsirkin wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On Wed, Mar 08, 2023 at 09:07:49AM -0500, Feng Liu wrote:
>>>>
>>>>
>>>> On 2023-03-08 a.m.12:58, Jason Wang wrote:
>>>>> External email: Use caution opening links or attachments
>>>>>
>>>>>
>>>>> On Tue, Mar 7, 2023 at 11:57?AM Feng Liu <feliu at
nvidia.com> wrote:
>>>>>>
>>>>>> Add const to make the read-only pointer parameters
clear, similar to
>>>>>> many existing functions.
>>>>>>
>>>>>> Signed-off-by: Feng Liu <feliu at nvidia.com>
>>>>>> Reviewed-by: Jiri Pirko <jiri at nvidia.com>
>>>>>> Reviewed-by: Parav Pandit <parav at nvidia.com>
>>>>>> Reviewed-by: Gavin Li <gavinl at nvidia.com>
>>>>>> Reviewed-by: Bodong Wang <bodong at nvidia.com>
>>>>>> ---
>>>>>>     drivers/virtio/virtio_ring.c | 25
++++++++++++-------------
>>>>>>     include/linux/virtio.h       | 12 ++++++------
>>>>>>     2 files changed, 18 insertions(+), 19 deletions(-)
>>>>>>
>>>>>
>>>>> [...]
>>>>>
>>>>>>
>>>>>> -/*
>>>>>> - * This should prevent the device from being used,
allowing drivers to
>>>>>> +/ This should prevent the device from being used,
allowing drivers to
>>>>>>      * recover.  You may need to grab appropriate locks
to flush.
>>>>>>      */
>>>>>
>>>>> Any reason for this change?
>>>>>
>>>> Hi, Jason
>>>> The original comment of the code had a syntax problem and
couldn't compile,
>>>> I fixed it here
>>>
>>> This is how it looked before your patch:
>>>
>>> /*
>>>    * This should prevent the device from being used, allowing
drivers to
>>>    * recover.  You may need to grab appropriate locks to flush.
>>>    */
>>>
>>> I see no problem here.
>>>
>> Yes, you are right. I made a mistake here, I will fix it
> 
> Nice but the bigger problem is not the mistake - it is the posting of
> untested code. It might be an ok thing to do - as long as you make it
> super abundantrly clear that this is what it is, and explain why
> you are posting it now and not after testing.
> 
In fact, I compiled and tested locally. I just looked it up and it might 
be that I was missing a ?git add? action which caused the problem. 
Before I post the patch in future, I will find a clean kernel source and 
apply my patch for testing instead of on the branch where the code is 
modified, so as to avoid this kind of problem from happening again. 
Very sorry for this problem, I will be careful and pay attention to it later
>>>
>>>>> Thanks
>>>>>
>>>
>

On Wed, Mar 08, 2023 at 09:40:03AM -0500, Feng Liu
wrote:
> In fact, I compiled and tested locally. I just looked it up and it might be
> that I was missing a ?git add? action which caused the problem. Before I
> post the patch in future, I will find a clean kernel source and apply my
> patch for testing instead of on the branch where the code is modified, so
as
> to avoid this kind of problem from happening again. Very sorry for this
> problem, I will be careful and pay attention to it later
I have a pre-push hook since it was happening to me a lot with pushes:


#!/bin/sh

# An example hook script to verify what is about to be pushed.  Called by
"git
# push" after it has checked the remote status, but before anything has
been
# pushed.  If this script exits with a non-zero status nothing will be pushed.
#
# This hook is called with the following parameters:
#
# $1 -- Name of the remote to which the push is being done
# $2 -- URL to which the push is being done
#
# If pushing without using a named remote those arguments will be equal.
#
# Information about the commits which are being pushed is supplied as lines to
# the standard input in the form:
#
#   <local ref> <local sha1> <remote ref> <remote sha1>
#
# This sample shows how to prevent push of commits where the log message starts
# with "WIP" (work in progress).

remote="$1"
url="$2"

echo "Pre push hook for remote $url"

#if
#	echo $url |grep ^root at virtlab > /dev/null
#then
#	echo "Lab push no need to check"
#	exit 0
#fi
#
#if
#	echo $url |grep ^/ > /dev/null
#then
#	echo "Local push no need to check"
#	exit 0
#fi

checked=0
HEAD=`git rev-list -1 HEAD`
IFS=' '
while read local_ref local_sha remote_ref remote_sha
do
    if [ $checked = 0 ]
    then
	if [ "$local_sha" = $HEAD ]
	then
		echo "Pushing HEAD to remote. Checking that tree is clean."
		if
			git diff-index --quiet HEAD
		then
			echo -n # No differences
		else
			echo "DIFF in HEAD. Not pushed, stash or -no-verify!"
			exit 1
		fi
		checked=1
	fi
    fi
done

exit 0



Consider sticking this in a post commit hook maybe?

-- 
MST