[dpdk-dev,v3,5/5] eal: don't hardcode socket filter value in IPC

Message ID 2f9a4da56e7f31f141db8cb2bb34d3c7fef97691.1519740527.git.anatoly.burakov@intel.com (mailing list archive)
State Superseded, archived
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail apply patch file failure

Commit Message

Anatoly Burakov Feb. 27, 2018, 2:35 p.m. UTC
  Currently, filter value is hardcoded and disconnected from actual
value returned by eal_mp_socket_path(). Fix this to generate filter
value by deriving it from eal_mp_socket_path() instead.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---

Notes:
    v3: no changes
    
    v2: no changes

 lib/librte_eal/common/eal_common_proc.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)
  

Comments

Jianfeng Tan Feb. 28, 2018, 1:52 a.m. UTC | #1
Hi Anatoly,

> -----Original Message-----
> From: Burakov, Anatoly
> Sent: Tuesday, February 27, 2018 10:36 PM
> To: dev@dpdk.org
> Cc: Tan, Jianfeng
> Subject: [PATCH v3 5/5] eal: don't hardcode socket filter value in IPC
> 
> Currently, filter value is hardcoded and disconnected from actual
> value returned by eal_mp_socket_path().

I can understand the hardcode is not good. But why it's not consistent with the actual value returned from eal_mp_socket_path()?

> Fix this to generate filter
> value by deriving it from eal_mp_socket_path() instead.
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>

Anyway, I think your way looks good to me, so

Acked-by: Jianfeng Tan <jianfeng.tan@intel.com>

Thanks,
Jianfeng

> ---
> 
> Notes:
>     v3: no changes
> 
>     v2: no changes
> 
>  lib/librte_eal/common/eal_common_proc.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/librte_eal/common/eal_common_proc.c
> b/lib/librte_eal/common/eal_common_proc.c
> index 7856a7b..4cf3aa6 100644
> --- a/lib/librte_eal/common/eal_common_proc.c
> +++ b/lib/librte_eal/common/eal_common_proc.c
> @@ -506,16 +506,17 @@ int
>  rte_mp_channel_init(void)
>  {
>  	char thread_name[RTE_MAX_THREAD_NAME_LEN];
> -	char *path;
> +	char path[PATH_MAX];
>  	int dir_fd;
>  	pthread_t tid;
> 
> -	snprintf(mp_filter, PATH_MAX, ".%s_unix_*",
> -		 internal_config.hugefile_prefix);
> +	/* create filter path */
> +	create_socket_path("*", path, sizeof(path));
> +	snprintf(mp_filter, sizeof(mp_filter), "%s", basename(path));
> 
> -	path = strdup(eal_mp_socket_path());
> -	snprintf(mp_dir_path, PATH_MAX, "%s", dirname(path));
> -	free(path);
> +	/* path may have been modified, so recreate it */
> +	create_socket_path("*", path, sizeof(path));
> +	snprintf(mp_dir_path, sizeof(mp_dir_path), "%s", dirname(path));
> 
>  	/* lock the directory */
>  	dir_fd = open(mp_dir_path, O_RDONLY);
> --
> 2.7.4
  
Anatoly Burakov Feb. 28, 2018, 10:21 a.m. UTC | #2
On 28-Feb-18 1:52 AM, Tan, Jianfeng wrote:
> Hi Anatoly,
> 
>> -----Original Message-----
>> From: Burakov, Anatoly
>> Sent: Tuesday, February 27, 2018 10:36 PM
>> To: dev@dpdk.org
>> Cc: Tan, Jianfeng
>> Subject: [PATCH v3 5/5] eal: don't hardcode socket filter value in IPC
>>
>> Currently, filter value is hardcoded and disconnected from actual
>> value returned by eal_mp_socket_path().
> 
> I can understand the hardcode is not good. But why it's not consistent with the actual value returned from eal_mp_socket_path()?

It is consistent. It's just that it's disconnected from the value 
returned by eal_mp_socket_path(). Meaning, if you change how 
eal_mp_socket_path() works, you'll also have to update the filter (or 
you may forget to do it and have a bug). This patch makes it so that 
mp_filter value is automatically updated, should you change internal 
workings of eal_mp_socket_path().

> 
>> Fix this to generate filter
>> value by deriving it from eal_mp_socket_path() instead.
>>
>> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> 
> Anyway, I think your way looks good to me, so
> 
> Acked-by: Jianfeng Tan <jianfeng.tan@intel.com>
> 
> Thanks,
> Jianfeng
>
  
Jianfeng Tan Feb. 28, 2018, 3:01 p.m. UTC | #3
On 2/28/2018 6:21 PM, Burakov, Anatoly wrote:
> On 28-Feb-18 1:52 AM, Tan, Jianfeng wrote:
>> Hi Anatoly,
>>
>>> -----Original Message-----
>>> From: Burakov, Anatoly
>>> Sent: Tuesday, February 27, 2018 10:36 PM
>>> To: dev@dpdk.org
>>> Cc: Tan, Jianfeng
>>> Subject: [PATCH v3 5/5] eal: don't hardcode socket filter value in IPC
>>>
>>> Currently, filter value is hardcoded and disconnected from actual
>>> value returned by eal_mp_socket_path().
>>
>> I can understand the hardcode is not good. But why it's not 
>> consistent with the actual value returned from eal_mp_socket_path()?
>
> It is consistent. It's just that it's disconnected from the value 
> returned by eal_mp_socket_path(). Meaning, if you change how 
> eal_mp_socket_path() works, you'll also have to update the filter (or 
> you may forget to do it and have a bug). This patch makes it so that 
> mp_filter value is automatically updated, should you change internal 
> workings of eal_mp_socket_path().

Yeah, that makes sense. Thank you for fixing it.

Thanks,
Jianfeng
  

Patch

diff --git a/lib/librte_eal/common/eal_common_proc.c b/lib/librte_eal/common/eal_common_proc.c
index 7856a7b..4cf3aa6 100644
--- a/lib/librte_eal/common/eal_common_proc.c
+++ b/lib/librte_eal/common/eal_common_proc.c
@@ -506,16 +506,17 @@  int
 rte_mp_channel_init(void)
 {
 	char thread_name[RTE_MAX_THREAD_NAME_LEN];
-	char *path;
+	char path[PATH_MAX];
 	int dir_fd;
 	pthread_t tid;
 
-	snprintf(mp_filter, PATH_MAX, ".%s_unix_*",
-		 internal_config.hugefile_prefix);
+	/* create filter path */
+	create_socket_path("*", path, sizeof(path));
+	snprintf(mp_filter, sizeof(mp_filter), "%s", basename(path));
 
-	path = strdup(eal_mp_socket_path());
-	snprintf(mp_dir_path, PATH_MAX, "%s", dirname(path));
-	free(path);
+	/* path may have been modified, so recreate it */
+	create_socket_path("*", path, sizeof(path));
+	snprintf(mp_dir_path, sizeof(mp_dir_path), "%s", dirname(path));
 
 	/* lock the directory */
 	dir_fd = open(mp_dir_path, O_RDONLY);