From owner-svn-src-head@freebsd.org Sun Feb 5 02:16:15 2017 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 2C270CD0FCE for ; Sun, 5 Feb 2017 02:16:15 +0000 (UTC) (envelope-from steven.hartland@multiplay.co.uk) Received: from mail-wm0-x236.google.com (mail-wm0-x236.google.com [IPv6:2a00:1450:400c:c09::236]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id AD88F1AEE for ; Sun, 5 Feb 2017 02:16:14 +0000 (UTC) (envelope-from steven.hartland@multiplay.co.uk) Received: by mail-wm0-x236.google.com with SMTP id b65so83348494wmf.0 for ; Sat, 04 Feb 2017 18:16:14 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=multiplay-co-uk.20150623.gappssmtp.com; s=20150623; h=subject:to:references:from:message-id:date:user-agent:mime-version :in-reply-to; bh=MSo5tbz1h+ykk1j1jOVNSQ9DPwphzTOrlroxw+e6RGY=; b=RZdR4TrL8qFQWdQnFAAnwh8jFcueSxsZn46ryM7kBprWMlpts0xafbr7AvOG3c1At0 Le6u6xr7VtS4+UJ8hK8EQFvWLWdMZZH4iS1+/+1Ao/ovF70EISfoqMyaywqMlOBNeeVZ InRxMQ3XqAsu0gi7I7KkVnur+2M8VFhbAAg8xGKsqIZcHFSv+Ehoa+OefhNvjapoeSYP urE2ETrnumxQ+ourHCF3/JoGpKxrnOniuMMdE4I5TAgpWOAC4s4RS29XBxa38Y8mrEkK dgBb3WI8EArU7uHWR0E+NTJ0rPNsl6i3IWctvuDRetCpJfCNbnLaaidtcI6jv0T/+Gry MU5g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to; bh=MSo5tbz1h+ykk1j1jOVNSQ9DPwphzTOrlroxw+e6RGY=; b=q/9InBqB5PJW5XSUTCDb086PVVGGL0zducL/bhzMgC1PQDPyqnOuuvqaRpjkcvwRAS aieu2K4BZ8IyZj+o+PRmQCwqrWQsxoQ5dqXQUvz/5Z7eoT8R7qjXiAejIRzIaesglLMm DXIdW7g502fduzb2chI2jPgcfb7CQdChZoT0+/MC9jwJpLyjoHzSaBS41kHo7KOR61J7 sJCNNuGAUav/Fp6kJ1zvjJ3dT9VZ5Dbmm4Sk4RW4AnWqQ/jxkx1F/1a1S0C7PmaxKGjP jipdc4p5GMIEnRPuJq58Ywa/gy8dvlvIXEhiwB5AjJDM93fH3PQXKhiDFwHhw/S8YkHI +fuQ== X-Gm-Message-State: AMke39kcEkSjx06ntFdU79u8mhwMneOIpW4k3wAkLteL63gJ6ieumJmtHo0DK6GHQzeIZ7Tf X-Received: by 10.28.127.13 with SMTP id a13mr3239591wmd.96.1486260972606; Sat, 04 Feb 2017 18:16:12 -0800 (PST) Received: from [10.10.1.58] ([185.97.61.26]) by smtp.gmail.com with ESMTPSA id 198sm5253474wmn.11.2017.02.04.18.16.11 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sat, 04 Feb 2017 18:16:11 -0800 (PST) Subject: Re: svn commit: r313260 - head/sys/kern To: Mateusz Guzik , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org References: <201702050140.v151eRXX090326@repo.freebsd.org> From: Steven Hartland Message-ID: <42c790bb-df12-2a50-6181-24bac5c72d34@multiplay.co.uk> Date: Sun, 5 Feb 2017 02:16:12 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: <201702050140.v151eRXX090326@repo.freebsd.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-Content-Filtered-By: Mailman/MimeDel 2.1.23 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 05 Feb 2017 02:16:15 -0000 Hi Mateusz could you improve on the commit message as it currently describes what is changed, which can be obtained from the diff, but not why? I hope on one feels like I'm trying to teach them to suck eggs, as I know everyone here has a wealth of experience, but I strongly believe commit messages are a very important way of improving the overall quality of the code base by sharing with others the reason for changes, which they can then learn from. I know I for one love picking up new nuggets of knowledge from others in this way. Also I believe this is area the project as a whole can improve on, so I don't mean to single out anyone here. Anyway I hope people find this useful: When I write a commit message I try to stick to the following rules which I believe helps to bring clarity for others about my actions. 1. First line is a brief summary of the out come of the change e.g. Fixed compiler warnings in nvmecontrol on 32bit platforms 2. Follow up paragraphs expand on #1 if needed including details about not just what but why the change was made e.g. Use ssize_t instead of uint32_t to prevent warnings about a comparison with different signs. Due to the promotion rules, this would only happen on 32-bit platforms. 3. When writing #2 include details that would not be obvious to non-experts in the particular area. #2 and #3 are really important to sharing knowledge that others may not know, its quite relevant to this commit msg, as while it may be obvious to you and others familiar with the atomic ops, to the rest of us we're just wondering why make this change? N.B. The example is based on Warner's recent commit purely as an example, which had a good why, just missing the brief summary. While on this subject are there any official guidelines to writing commit messages, if no should we create some? On 05/02/2017 01:40, Mateusz Guzik wrote: > Author: mjg > Date: Sun Feb 5 01:40:27 2017 > New Revision: 313260 > URL: https://svnweb.freebsd.org/changeset/base/313260 > > Log: > fd: switch fget_unlocked to atomic_fcmpset > > Modified: > head/sys/kern/kern_descrip.c > > Modified: head/sys/kern/kern_descrip.c > ============================================================================== > --- head/sys/kern/kern_descrip.c Sun Feb 5 01:20:39 2017 (r313259) > +++ head/sys/kern/kern_descrip.c Sun Feb 5 01:40:27 2017 (r313260) > @@ -2569,8 +2569,8 @@ fget_unlocked(struct filedesc *fdp, int > if (error != 0) > return (error); > #endif > - retry: > count = fp->f_count; > + retry: > if (count == 0) { > /* > * Force a reload. Other thread could reallocate the > @@ -2584,7 +2584,7 @@ fget_unlocked(struct filedesc *fdp, int > * Use an acquire barrier to force re-reading of fdt so it is > * refreshed for verification. > */ > - if (atomic_cmpset_acq_int(&fp->f_count, count, count + 1) == 0) > + if (atomic_fcmpset_acq_int(&fp->f_count, &count, count + 1) == 0) > goto retry; > fdt = fdp->fd_files; > #ifdef CAPABILITIES >