On Mon, 24 Aug 2020 20:07:48 +0300
Vitaliy Makkoveev wrote:
> I pointed some comments inline.
Thanks,
>> +case PIPEXASESSION:
>> +{
>> +struct pipex_session_req *req =
>> +(struct pipex_session_req *)data;
>> +if ((error = pipex_init_session(, req)) != 0)
>> +break;
>> +error = pipex_link_session(session, >sc_if, sc);
>> +break;
>> +}
>
> If pipex_link_session() fails `session' will be leaked.
Yes, it's a good catch.
>> +case PIPEXDSESSION:
>> +{
>> +struct pipex_session_close_req *req =
>> +(struct pipex_session_close_req *)data;
>> +session = pipex_lookup_by_session_id(req->pcr_protocol,
>> +req->pcr_session_id);
>> +if (session == NULL || session->ifindex != sc->sc_if.if_index) {
>
> Can you compare with `session->ownersc' instead of `ifindex' like other
> code does? For consistency with other code.
Yes, it's better.
> What about to introduce pppac_{add,del}_session() and move related code
> into them?
Also I agreed.
> Also I see no such reason to kill pipex_{add,destroy}_session() because
> they play with `pipex_rd_head{4,6}' and you don't need newly introduced
> `session->is_pppx' which you use only once for that reason.
pipex_{add,destroy}_session() should be killed since they are only for pppac.
I think such functions should have "pppac_" prefix and placed in if_pppx.c.
Also I'd like to move pipex_rd_head{4,6} things to pppac_{add,del}_session with
a next step. Yes, we might be able to kill is_pppx. But I'd like to discuss
that as a next step as well.
I'd like to commit this for this moment, and continue further discussion.
ok?
Index: sys/net/if_pppx.c
===
RCS file: /cvs/src/sys/net/if_pppx.c,v
retrieving revision 1.101
diff -u -p -r1.101 if_pppx.c
--- sys/net/if_pppx.c 14 Aug 2020 11:05:38 - 1.101
+++ sys/net/if_pppx.c 26 Aug 2020 06:25:34 -
@@ -163,7 +163,6 @@ struct pppx_if {
struct ifnetpxi_if;
struct pppx_dev *pxi_dev; /* [I] */
struct pipex_session*pxi_session; /* [I] */
- struct pipex_iface_context pxi_ifcontext; /* [N] */
};
static inline int
@@ -181,12 +180,6 @@ intpppx_add_session(struct pppx_dev *,
struct pipex_session_req *);
intpppx_del_session(struct pppx_dev *,
struct pipex_session_close_req *);
-intpppx_config_session(struct pppx_dev *,
- struct pipex_session_config_req *);
-intpppx_get_stat(struct pppx_dev *,
- struct pipex_session_stat_req *);
-intpppx_get_closed(struct pppx_dev *,
- struct pipex_session_list_req *);
intpppx_set_session_descr(struct pppx_dev *,
struct pipex_session_descr_req *);
@@ -424,17 +417,6 @@ pppxioctl(dev_t dev, u_long cmd, caddr_t
NET_LOCK();
switch (cmd) {
- case PIPEXSMODE:
- /*
-* npppd always enables on open, and only disables before
-* closing. we cheat and let open and close do that, so lie
-* to npppd.
-*/
- break;
- case PIPEXGMODE:
- *(int *)addr = 1;
- break;
-
case PIPEXASESSION:
error = pppx_add_session(pxd,
(struct pipex_session_req *)addr);
@@ -445,21 +427,6 @@ pppxioctl(dev_t dev, u_long cmd, caddr_t
(struct pipex_session_close_req *)addr);
break;
- case PIPEXCSESSION:
- error = pppx_config_session(pxd,
- (struct pipex_session_config_req *)addr);
- break;
-
- case PIPEXGSTAT:
- error = pppx_get_stat(pxd,
- (struct pipex_session_stat_req *)addr);
- break;
-
- case PIPEXGCLOSED:
- error = pppx_get_closed(pxd,
- (struct pipex_session_list_req *)addr);
- break;
-
case PIPEXSIFDESCR:
error = pppx_set_session_descr(pxd,
(struct pipex_session_descr_req *)addr);
@@ -472,7 +439,7 @@ pppxioctl(dev_t dev, u_long cmd, caddr_t
break;
default:
- error = ENOTTY;
+ error = pipex_ioctl(pxd, cmd, addr);
break;
}
NET_UNLOCK();
@@ -741,11 +708,7 @@ pppx_add_session(struct pppx_dev *pxd, s
if_addrhooks_run(ifp);
}
- /* fake a pipex interface context */
- pxi->pxi_ifcontext.ifindex = ifp->if_index;
- pxi->pxi_ifcontext.pipexmode = PIPEX_ENABLED;
-
- error = pipex_link_session(session, >pxi_ifcontext);
+ error = pipex_link_session(session,
