> -----Original Message-----
> From: Burakov, Anatoly
> Sent: Thursday, January 25, 2018 12:00 PM
> To: Tan, Jianfeng <jianfeng....@intel.com>; dev@dpdk.org
> Cc: Richardson, Bruce <bruce.richard...@intel.com>; Ananyev, Konstantin
> <konstantin.anan...@intel.com>; tho...@monjalon.net
> Subject: Re: [PATCH v3 2/3] eal: add synchronous multi-process communication
>
> On the overall patch,
>
> Reviewed-by: Anatoly Burakov <anatoly.bura...@intel.com>
>
> For request(), returning number of replies received actually makes
> sense, because now we get use the value to read our replies, if we were
> a primary process sending messages to secondary processes.
Yes, I also think it is good to return number of sends.
Then caller can compare number of sended requests with number of
received replies and decide should it be considered a failure or no.
> > + return 0;
> > + }
> > +
> > + if (send_msg(dst, req, MP_REQ) != 1) {
> > + RTE_LOG(ERR, EAL, "Fail to send request %s:%s\n",
> > + dst, req->name);
> > + return 0;
> > + }
> > +
> > + pthread_mutex_lock(&sync_requests.lock);
> > + do {
> > + pthread_cond_timedwait(&sync_req.cond, &sync_requests.lock, ts);
> > + /* Check spurious wakeups */
> > + if (sync_req.reply_received == 1)
> > + break;
> > + /* Check if time is out */
> > + if (gettimeofday(&now, NULL) < 0)
> > + break;
> > + if (now.tv_sec < ts->tv_sec)
> > + break;
> > + else if (now.tv_sec == ts->tv_sec &&
> > + now.tv_usec * 1000 < ts->tv_nsec)
> > + break;
> > + } while (1);
> > + /* We got the lock now */
> > + TAILQ_REMOVE(&sync_requests.requests, &sync_req, next);
> > + pthread_mutex_unlock(&sync_requests.lock);
> > +
> > + if (sync_req.reply_received == 0) {
> > + RTE_LOG(ERR, EAL, "Fail to recv reply for request %s:%s\n",
> > + dst, req->name);
> > + return 1;
>
> Why are we returning 1 here? There was no reply, so no reply structure
> was allocated. This looks like a potential buffer overflow on trying to
> read replies if one of them wasn't delivered.
As I understand - because we receive a number of sended requests.
Number of received replies will be available in reply->nb_msgs.
Same below.
Konstantin
>
> > + }
> > +
> > + tmp = realloc(reply->msgs, sizeof(msg) * (reply->nb_msgs + 1));
> > + if (!tmp) {
> > + RTE_LOG(ERR, EAL, "Fail to alloc reply for request %s:%s\n",
> > + dst, req->name);
> > + return 1;
> > + }
>
> Same here - we couldn't allocate a reply, so it won't get to the user.
> Why return 1 here?
>
> > + memcpy(&tmp[reply->nb_msgs], &msg, sizeof(msg));
> > + reply->msgs = tmp;
> > + reply->nb_msgs++;
> > + return 1;
> > +}
> > +
> > +int
> > +rte_eal_mp_request(struct rte_mp_msg *req,
> > + struct rte_mp_reply *reply,
> > + const struct timespec *ts)
> > +{
> > + DIR *mp_dir;
> > + struct dirent *ent;
> > + int nb_snds = 0;
> > + struct timeval now;
> > + struct timespec end;
> > +
>
> <snip>
>
> > /**
> > * @warning
> > @@ -262,6 +268,56 @@ void rte_eal_mp_action_unregister(const char *name);
> > int rte_eal_mp_sendmsg(struct rte_mp_msg *msg);
> >
> > /**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice
> > + *
> > + * Send a request to the peer process and expect a reply.
> > + *
> > + * This function sends a request message to the peer process, and will
> > + * block until receiving reply message from the peer process.
> > + *
> > + * @note The caller is responsible to free reply->replies.
> > + *
> > + * @param req
> > + * The req argument contains the customized request message.
> > + *
> > + * @param reply
> > + * The reply argument will be for storing all the replied messages;
> > + * the caller is responsible for free reply->replies.
> > + *
> > + * @param ts
> > + * The ts argument specifies how long we can wait for the peer(s) to
> > reply.
> > + *
> > + * @return
> > + * - (<0) on invalid parameters;
> > + * - (>=0) as the number of messages being sent successfully.
> > + */
> > +int rte_eal_mp_request(struct rte_mp_msg *req,
> > + struct rte_mp_reply *reply, const struct timespec *ts);
>
> See above: it would be much more useful to return number of replies
> received, rather than number of messages sent, as that's the number we
> are most interested in. Otherwise, if we e.g. sent 5 messages but
> received 1 reply, you're essentially not telling the user how far can he
> index the reply pointer.
>
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice
> > + *
> > + * Send a reply to the peer process.
> > + *
> > + * This function will send a reply message in response to a request message
> > + * received previously.
> > + *
> > + * @param msg
> > + * The msg argument contains the customized message.
> > + *
> > + * @param peer
> > + * The peer argument is the pointer to the peer socket path.
> > + *
> > + * @return
> > + * - (1) on success;
> > + * - (0) on failure;
> > + * - (<0) on invalid parameters.
> > + */
> > +int rte_eal_mp_reply(struct rte_mp_msg *msg, const char *peer);
>
> I don't think there's much point in making distinction between invalid
> parameters and failure.
>
> > +
> > +/**
> > * Usage function typedef used by the application usage function.
> > *
> > * Use this function typedef to define and call
> > rte_set_application_usage_hook()
> > diff --git a/lib/librte_eal/rte_eal_version.map
> > b/lib/librte_eal/rte_eal_version.map
> > index adeadfb..3015bc6 100644
> > --- a/lib/librte_eal/rte_eal_version.map
> > +++ b/lib/librte_eal/rte_eal_version.map
> > @@ -220,6 +220,9 @@ EXPERIMENTAL {
> > rte_eal_mp_action_register;
> > rte_eal_mp_action_unregister;
> > rte_eal_mp_sendmsg;
> > + rte_eal_mp_request;
> > + rte_eal_mp_reply;
> > + rte_eal_mp_sendmsg;
>
> You're adding rte_eal_mp_sendmsg twice.
>
> > rte_service_attr_get;
> > rte_service_attr_reset_all;
> > rte_service_component_register;
> >
>
>
> --
> Thanks,
> Anatoly
