Michael S. Tsirkin
2023-Mar-08 14:28 UTC
[PATCH 3/3] virtio_ring: Use const to annotate read-only pointer params
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 itNice 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.> > > > > > Thanks > > > > > >
Feng Liu
2023-Mar-08 14:40 UTC
[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 >>>>> >>> >