Re: [PATCH 3/3] Don't use unportable macros

[Doug Evans <dje@google.com>]

On Tue, Mar 19, 2013 at 7:35 PM, Yao Qi <yao@codesourcery.com> wrote:
> On 03/20/2013 12:31 AM, Doug Evans wrote:
>>>     /* Create DIRNAME.  */
>>> -  if (mkdir (dirname, S_IRWXU | S_IRGRP | S_IXGRP | S_IROTH | S_IXOTH)
>>> -      && errno != EEXIST)
>>> +  if (mkdir (dirname, S_IRUSR | S_IWUSR | S_IXUSR) && errno != EEXIST)
>>>       error (_("Unable to open directory '%s' for saving trace data (%s)"),
>>>             dirname, safe_strerror (errno));
>>
>> It feels like one would want to use mode 0755 by default on *nix.
>> Why not store the mode in a local, and have a sequence of #ifdef's
>> like remote-fileio.c?
>
> OK.  Done.  The directory mode now is 755.

Thanks.
I would have done it differently, e.g.,

#ifdef S_IRGRP
  hmode |= S_IRGRP;
#endif
...

but this way is ok.

> On 03/20/2013 01:19 AM, Eli Zaretskii wrote:
>>
>> This is why I suggested an alternative and much smaller and more
>> elegant solution.
>
> Also define a macro 'mkdir' in this patch, as Eli suggested before.  Is
> it OK?

fwiw, I don't mind all the crap gnulib dragged in.  Kinda depressing,
but whatever.
I just wanted to make sure it was correct.

I also don't mind Eli's suggestion.
One can argue the details belong elsewhere (if other parts of gdb want
mkdir it doesn't make sense to cut-n-paste that to every place -
unlikely, but still ...).

The "right" answer is probably to go with gnulib, but I'm happy to
defer judgement on this.

> --
> Yao (齐尧)
>
> gdb:
>
> 2013-03-20  Yao Qi  <yao@codesourcery.com>
>
>         * ctf.c: Include "gdb_stat.h".
>         [USE_WIN32API]: New macro 'mkdir'.
>         (ctf_start): Use permission bits macros if they are defined.
> ---
>  gdb/ctf.c |   20 ++++++++++++++++++--
>  1 files changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/gdb/ctf.c b/gdb/ctf.c
> index 455bfde..f058dc9 100644
> --- a/gdb/ctf.c
> +++ b/gdb/ctf.c
> @@ -24,6 +24,7 @@
>  #include "tracepoint.h"
>  #include "regcache.h"
>  #include "exec.h"
> +#include "gdb_stat.h"
>
>  #include <ctype.h>
>
> @@ -294,6 +295,10 @@ ctf_target_save (struct trace_file_writer *self,
>    return 0;
>  }
>
> +#ifdef USE_WIN32API
> +#define mkdir(pathname, mode) _mkdir (pathname)
> +#endif
> +
>  /* This is the implementation of trace_file_write_ops method
>     start.  It creates the directory DIRNAME, metadata and datastream
>     in the directory.  */
> @@ -306,10 +311,21 @@ ctf_start (struct trace_file_writer *self, const char *dirname)
>    struct ctf_trace_file_writer *writer
>      = (struct ctf_trace_file_writer *) self;
>    int i;
> +  mode_t hmode = S_IRUSR | S_IWUSR | S_IXUSR
> +#ifdef S_IRGRP
> +    | S_IRGRP
> +#endif
> +#ifdef S_IXGRP
> +    | S_IXGRP
> +#endif
> +    | S_IROTH /* Defined in common/gdb_stat.h if not defined.  */
> +#ifdef S_IXOTH
> +    | S_IXOTH
> +#endif
> +    ;
>
>    /* Create DIRNAME.  */
> -  if (mkdir (dirname, S_IRWXU | S_IRGRP | S_IXGRP | S_IROTH | S_IXOTH)
> -      && errno != EEXIST)
> +  if (mkdir (dirname, hmode) && errno != EEXIST)
>      error (_("Unable to open directory '%s' for saving trace data (%s)"),
>            dirname, safe_strerror (errno));
>
> --
> 1.7.7.6