On Sun, Mar 01, 2020 at 10:55:02PM -0800, Bjorn Andersson wrote:
> On Sun 01 Mar 21:50 PST 2020, Manivannan Sadhasivam wrote:
>
> > Hi Bjorn,
> >
> > Thanks for the fix. I have tested this and it works perfectly!
> >
> > On Sun, Mar 01, 2020 at 07:25:26PM -0800, Bjorn Andersson wrote:
> > > Lost in the translation from the user space implementation was the
> > > detail that HELLO mesages must be exchanged between each node pair. As
> > > such the incoming HELLO must be replied to.
> > >
> >
> > Err. I thought the say_hello() part in ctrl_cmd_hello() was redundant, so
> > removed it :P
> >
> > Sorry for that.
> >
>
> No worries.
>
> > > Similar to the previous implementation no effort is made to prevent two
> > > Linux boxes from continuously sending HELLO messages back and forth,
> > > this is left to a follow up patch.
> > >
> > > say_hello() is moved, to facilitate the new call site.
> > >
> > > Fixes: 0c2204a4ad71 ("net: qrtr: Migrate nameservice to kernel from userspace")
> > > Signed-off-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
> > > ---
> > > net/qrtr/ns.c | 54 ++++++++++++++++++++++++++++-----------------------
> > > 1 file changed, 30 insertions(+), 24 deletions(-)
> > >
> > > diff --git a/net/qrtr/ns.c b/net/qrtr/ns.c
> > > index 7bfde01f4e8a..e3f11052b5f6 100644
> > > --- a/net/qrtr/ns.c
> > > +++ b/net/qrtr/ns.c
> > > @@ -286,9 +286,38 @@ static int server_del(struct qrtr_node *node, unsigned int port)
> > > return 0;
> > > }
> > >
> > > +static int say_hello(struct sockaddr_qrtr *dest)
> > > +{
> > > + struct qrtr_ctrl_pkt pkt;
> > > + struct msghdr msg = { };
> > > + struct kvec iv;
> > > + int ret;
> > > +
> > > + iv.iov_base = &pkt;
> > > + iv.iov_len = sizeof(pkt);
> > > +
> > > + memset(&pkt, 0, sizeof(pkt));
> > > + pkt.cmd = cpu_to_le32(QRTR_TYPE_HELLO);
> > > +
> > > + msg.msg_name = (struct sockaddr *)dest;
> > > + msg.msg_namelen = sizeof(*dest);
> > > +
> > > + ret = kernel_sendmsg(qrtr_ns.sock, &msg, &iv, 1, sizeof(pkt));
> > > + if (ret < 0)
> > > + pr_err("failed to send hello msg\n");
> > > +
> > > + return ret;
> > > +}
> > > +
> > > /* Announce the list of servers registered on the local node */
> > > static int ctrl_cmd_hello(struct sockaddr_qrtr *sq)
> > > {
> > > + int ret;
> > > +
> > > + ret = say_hello(sq); > > + if (ret < 0)
> > > + return ret;
> > > +
> > > return announce_servers(sq);
> > > }
> > >
> > > @@ -566,29 +595,6 @@ static void ctrl_cmd_del_lookup(struct sockaddr_qrtr *from,
> > > }
> > > }
> > >
> > > -static int say_hello(void)
> > > -{
> > > - struct qrtr_ctrl_pkt pkt;
> > > - struct msghdr msg = { };
> > > - struct kvec iv;
> > > - int ret;
> > > -
> > > - iv.iov_base = &pkt;
> > > - iv.iov_len = sizeof(pkt);
> > > -
> > > - memset(&pkt, 0, sizeof(pkt));
> > > - pkt.cmd = cpu_to_le32(QRTR_TYPE_HELLO);
> > > -
> > > - msg.msg_name = (struct sockaddr *)&qrtr_ns.bcast_sq;
> > > - msg.msg_namelen = sizeof(qrtr_ns.bcast_sq);
> > > -
> > > - ret = kernel_sendmsg(qrtr_ns.sock, &msg, &iv, 1, sizeof(pkt));
> > > - if (ret < 0)
> > > - pr_err("failed to send hello msg\n");
> > > -
> > > - return ret;
> > > -}
> > > -
> > > static void qrtr_ns_worker(struct work_struct *work)
> > > {
> > > const struct qrtr_ctrl_pkt *pkt;
> > > @@ -725,7 +731,7 @@ void qrtr_ns_init(struct work_struct *work)
> > > if (!qrtr_ns.workqueue)
> > > goto err_sock;
> > >
> > > - ret = say_hello();
> > > + ret = say_hello(&qrtr_ns.bcast_sq);
> >
> > Why do you want to pass a global variable here? Why can't it be used directly
> > in say_hello() as done before?
> >
>
> Because I changed the prototype of say_hello() so that we pass the
> destination address; here that's the broadcast address, in
> ctrl_cmd_hello() it's the specific sender of the incoming hello that we
> want to respond to.
>
Ah, yes. I missed that. Sounds good to me.
Thanks,
Mani
> Regards,
> Bjorn
>
> > Other than that,
> >
> > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> > Tested-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> >
> > Thanks,
> > Mani
> >
> > > if (ret < 0)
> > > goto err_wq;
> > >
> > > --
> > > 2.24.0
> > >
