Re: [dpdk-dev] [PATCH 2/2] eal/windows: fix build by supporting trace

[Jerin Jacob <jerinjacobk@gmail.com>]

On Sun, Apr 26, 2020 at 8:53 AM Dmitry Kozlyuk <dmitry.kozliuk@gmail.com> wrote:
>
> Add EAL private functions to support trace storage:
>
> * eal_persistent_data_path()
> * eal_dir_create()
>
> Replace clock_gettime(CLOCK_REALTIME) with C11 timespec_get().
> Implementation is provided for MinGW-w64 that misses this function.
>
> Provide minimum viable implementations of malloc and timer functions
> used by tracing.
>
> Fixes: 185b7dc1d467 ("trace: save bootup timestamp")
> Fixes: 321dd5f8fa62 ("trace: add internal init and fini interface")
> Reported-by: Pallavi Kadam <pallavi.kadam@intel.com>
> Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> ---

In general, the patch looks good to me. Could you split the patch as two

1) New eal_permanent_data_path()  and eal_dir_create() API definition,
and its implementation with unix and the common code changes to adapt new APIs.
2) Windows implementation of eal_permanent_data_path() and other
missing pieces for
trace implementation.

Some comments below,

>
>  void eal_free_no_trace(void *addr);
>
> +/**
> + * Get absolute path to the directory where permanent data can be stored.
> + *
> + * @return
> + *  Statically allocated string on success, NULL on failure.
> + */
> +const char *
> +eal_permanent_data_path(void);

Do windows have PATH_MAX kind of macro? I think, it is better API
consumer allocates
the memory of size PATH_MAX and implementation fills it, instead of,
the static scheme.

> +
> +/**
> + * Create a directory accessible to the current user only.
> + *
> + * This function does not create intermediate directories,
> + * thus only the last path component may be nonexistent.
> + *
> + * This function succeeds if path already exists and is a directory.
> + *
> + * Platform-independent code should use forward slash as path separator.
> + *
> + * @param path
> + *  Path to be created.
> + * @return
> + *  0 on success, (-1) on failure and rte_errno is set.
> + */
> +int eal_dir_create(const char *path);
>

Looks good to me.