On Thu, Sep 28, 2017 at 01:50:20PM +0000, Tan, Jianfeng wrote:
> > > +int
> > > +rte_eal_primary_secondary_add_action(const char *action_name,
> > > +                              rte_eal_primary_secondary_t action)
> > > +{
> > > + struct action_entry *entry = malloc(sizeof(struct action_entry));
> > > +
> > > + if (entry == NULL)
> > > +         return -ENOMEM;
> > > +
> > > + strncpy(entry->action_name, action_name,
> > MAX_ACTION_NAME_LEN);
> > > + entry->action = action;
> > > + TAILQ_INSERT_TAIL(&action_entry_list, entry, next);
> > 
> > Since you intended to support "one primary process and multiple secondary
> > process", here we need a lock to protect the list.
> 
> Only one thread of each process (either primary or secondary) does the 
> register. So I wonder we don't have to add lock? Of course, no harm to add a 
> lock.

Right, I was wrong. I was thinking secondary processes could start at the
same time, that we need a lock here. But as you said, the list is process
specific: each process has it's own copy. No locked is needed then.

> > 
> > Another wonder is do we really need that, I mean 1:N model?
> 
> I'm open to suggestions. IMO, not much extra code for 1:N model than 1:1 
> model. So not necessary to restrict that.

Fair enough. I was just wondering; I have no objection though.

> > > +/** Path of primary/secondary communication unix socket file. */
> > > +#define PRIMARY_SECONDARY_UNIX_PATH_FMT "%s/.%s_unix"
> > > +static inline const char *
> > > +eal_primary_secondary_unix_path(void)
> > > +{
> > > + static char buffer[PATH_MAX]; /* static so auto-zeroed */
> > > + const char *directory = default_config_dir;
> > > + const char *home_dir = getenv("HOME");
> > 
> > It's not a good practice to generate such file at HOME dir. User would
> > be surprised to find it at HOME dir. In the worst case, user might delete
> > it.
> 
> This way is the legacy way in DPDK, for example the config path. So I think 
> we should fix that in another patch.

Yes, I think so.

        --yliu
> 
> > 
> > The more common way is to put it to tmp dir, like "/tmp".
> 
> Thanks,
> Jianfeng

Reply via email to