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

Doug Evans
2013-03-20T15:03:29Z

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