Re: svn commit: r335856 - in head/sys: netinet sys

2018-07-10 Thread Steven Hartland
Sorry guys I didn't spot it was just a revert as it was tagged on to the 
end of the description, I would have expected that to be in the subject.


What do others think, is there an recommend style for revert commit 
messages?


    Regards
    Steve

On 02/07/2018 17:30, Rodney W. Grimes wrote:

[ Charset UTF-8 unsupported, converting... ]

On Mon, Jul 2, 2018 at 10:44 AM Steven Hartland <
steven.hartl...@multiplay.co.uk> wrote:

You have M_WAITOK and a null check in this change

And, that's the same as the way it was before his commits. So, he did
exactly what he said he was doing and reverted his commits. I don't think
it is good practice to mix reverts with other changes.

It is a very bad practive to mix a revert with anything.


Since you've noticed this, I think you should feel free to make the change.
Jonathan


___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"


Re: svn commit: r335856 - in head/sys: netinet sys

2018-07-02 Thread Rodney W. Grimes
[ Charset UTF-8 unsupported, converting... ]
> On Mon, Jul 2, 2018 at 10:44 AM Steven Hartland <
> steven.hartl...@multiplay.co.uk> wrote:
> >
> > You have M_WAITOK and a null check in this change
> 
> And, that's the same as the way it was before his commits. So, he did
> exactly what he said he was doing and reverted his commits. I don't think
> it is good practice to mix reverts with other changes.

It is a very bad practive to mix a revert with anything.

> Since you've noticed this, I think you should feel free to make the change.
> Jonathan

-- 
Rod Grimes rgri...@freebsd.org
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"


Re: svn commit: r335856 - in head/sys: netinet sys

2018-07-02 Thread Cy Schubert
In message 
, "Jonathan T. Looney" writes:
> On Mon, Jul 2, 2018 at 10:44 AM Steven Hartland <
> steven.hartl...@multiplay.co.uk> wrote:
> >
> > You have M_WAITOK and a null check in this change
>
> And, that's the same as the way it was before his commits. So, he did
> exactly what he said he was doing and reverted his commits. I don't think
> it is good practice to mix reverts with other changes.

Yes, mixing reverts with other changes or batching changes together in 
one commit confuses history. This is my main criticism of Linux commit 
logs, IMO the worst example of commit log content. There they itemize a 
shopping list of changes or simply say, pull fixes for X from 
so-and-so, the patch passes our tests.

Five years from now or even a year from now, will anyone remember?

>
> Since you've noticed this, I think you should feel free to make the change.
>
> Jonathan


-- 
Cheers,
Cy Schubert 
FreeBSD UNIX: Web:  http://www.FreeBSD.org

The need of the many outweighs the greed of the few.


___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"


Re: svn commit: r335856 - in head/sys: netinet sys

2018-07-02 Thread Jonathan T. Looney
On Mon, Jul 2, 2018 at 10:44 AM Steven Hartland <
steven.hartl...@multiplay.co.uk> wrote:
>
> You have M_WAITOK and a null check in this change

And, that's the same as the way it was before his commits. So, he did
exactly what he said he was doing and reverted his commits. I don't think
it is good practice to mix reverts with other changes.

Since you've noticed this, I think you should feel free to make the change.

Jonathan
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"


Re: svn commit: r335856 - in head/sys: netinet sys

2018-07-02 Thread Steven Hartland
You have M_WAITOK and a null check in this change

On Mon, 2 Jul 2018 at 06:20, Matt Macy  wrote:

> Author: mmacy
> Date: Mon Jul  2 05:19:44 2018
> New Revision: 335856
> URL: https://svnweb.freebsd.org/changeset/base/335856
>
> Log:
>   inpcb: don't gratuitously defer frees
>
>   Don't defer frees in sysctl handlers. It isn't necessary
>   and it just confuses things.
>   revert: r333911, r334104, and r334125
>
>   Requested by: jtl
>
> Modified:
>   head/sys/netinet/ip_divert.c
>   head/sys/netinet/raw_ip.c
>   head/sys/netinet/tcp_subr.c
>   head/sys/netinet/udp_usrreq.c
>   head/sys/sys/malloc.h
>
> Modified: head/sys/netinet/ip_divert.c
>
> ==
> --- head/sys/netinet/ip_divert.cMon Jul  2 01:30:33 2018
> (r335855)
> +++ head/sys/netinet/ip_divert.cMon Jul  2 05:19:44 2018
> (r335856)
> @@ -552,7 +552,6 @@ div_detach(struct socket *so)
> KASSERT(inp != NULL, ("div_detach: inp == NULL"));
> INP_INFO_WLOCK(_divcbinfo);
> INP_WLOCK(inp);
> -   /* XXX defer destruction to epoch_call */
> in_pcbdetach(inp);
> in_pcbfree(inp);
> INP_INFO_WUNLOCK(_divcbinfo);
> @@ -632,7 +631,6 @@ static int
>  div_pcblist(SYSCTL_HANDLER_ARGS)
>  {
> int error, i, n;
> -   struct in_pcblist *il;
> struct inpcb *inp, **inp_list;
> inp_gen_t gencnt;
> struct xinpgen xig;
> @@ -672,8 +670,9 @@ div_pcblist(SYSCTL_HANDLER_ARGS)
> if (error)
> return error;
>
> -   il = malloc(sizeof(struct in_pcblist) + n * sizeof(struct inpcb
> *), M_TEMP, M_WAITOK|M_ZERO_INVARIANTS);
> -   inp_list = il->il_inp_list;
> +   inp_list = malloc(n * sizeof *inp_list, M_TEMP, M_WAITOK);
> +   if (inp_list == NULL)
> +   return ENOMEM;
>
> INP_INFO_RLOCK(_divcbinfo);
> for (inp = CK_LIST_FIRST(V_divcbinfo.ipi_listhead), i = 0; inp &&
> i < n;
> @@ -702,9 +701,14 @@ div_pcblist(SYSCTL_HANDLER_ARGS)
> } else
> INP_RUNLOCK(inp);
> }
> -   il->il_count = n;
> -   il->il_pcbinfo = _divcbinfo;
> -   epoch_call(net_epoch_preempt, >il_epoch_ctx,
> in_pcblist_rele_rlocked);
> +   INP_INFO_WLOCK(_divcbinfo);
> +   for (i = 0; i < n; i++) {
> +   inp = inp_list[i];
> +   INP_RLOCK(inp);
> +   if (!in_pcbrele_rlocked(inp))
> +   INP_RUNLOCK(inp);
> +   }
> +   INP_INFO_WUNLOCK(_divcbinfo);
>
> if (!error) {
> /*
> @@ -721,6 +725,7 @@ div_pcblist(SYSCTL_HANDLER_ARGS)
> INP_INFO_RUNLOCK(_divcbinfo);
> error = SYSCTL_OUT(req, , sizeof xig);
> }
> +   free(inp_list, M_TEMP);
> return error;
>  }
>
> @@ -800,7 +805,6 @@ div_modevent(module_t mod, int type, void *unused)
> break;
> }
> ip_divert_ptr = NULL;
> -   /* XXX defer to epoch_call ? */
> err = pf_proto_unregister(PF_INET, IPPROTO_DIVERT,
> SOCK_RAW);
> INP_INFO_WUNLOCK(_divcbinfo);
>  #ifndef VIMAGE
>
> Modified: head/sys/netinet/raw_ip.c
>
> ==
> --- head/sys/netinet/raw_ip.c   Mon Jul  2 01:30:33 2018(r335855)
> +++ head/sys/netinet/raw_ip.c   Mon Jul  2 05:19:44 2018(r335856)
> @@ -863,7 +863,6 @@ rip_detach(struct socket *so)
> ip_rsvp_force_done(so);
> if (so == V_ip_rsvpd)
> ip_rsvp_done();
> -   /* XXX defer to epoch_call */
> in_pcbdetach(inp);
> in_pcbfree(inp);
> INP_INFO_WUNLOCK(_ripcbinfo);
> @@ -1033,7 +1032,6 @@ static int
>  rip_pcblist(SYSCTL_HANDLER_ARGS)
>  {
> int error, i, n;
> -   struct in_pcblist *il;
> struct inpcb *inp, **inp_list;
> inp_gen_t gencnt;
> struct xinpgen xig;
> @@ -1068,8 +1066,9 @@ rip_pcblist(SYSCTL_HANDLER_ARGS)
> if (error)
> return (error);
>
> -   il = malloc(sizeof(struct in_pcblist) + n * sizeof(struct inpcb
> *), M_TEMP, M_WAITOK|M_ZERO_INVARIANTS);
> -   inp_list = il->il_inp_list;
> +   inp_list = malloc(n * sizeof *inp_list, M_TEMP, M_WAITOK);
> +   if (inp_list == NULL)
> +   return (ENOMEM);
>
> INP_INFO_RLOCK(_ripcbinfo);
> for (inp = CK_LIST_FIRST(V_ripcbinfo.ipi_listhead), i = 0; inp &&
> i < n;
> @@ -1098,9 +1097,14 @@ rip_pcblist(SYSCTL_HANDLER_ARGS)
> } else
> INP_RUNLOCK(inp);
> }
> -   il->il_count = n;
> -   il->il_pcbinfo = _ripcbinfo;
> -   epoch_call(net_epoch_preempt, >il_epoch_ctx,
> in_pcblist_rele_rlocked);
> +   INP_INFO_WLOCK(_ripcbinfo);
> +   for (i = 0; i < n; i++) {
> +   inp = inp_list[i];
> +   INP_RLOCK(inp);
> +   if