Portable mktemp calls in the tests

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

Portable mktemp calls in the tests

Samba - samba-technical mailing list
Here is another piece of puzzle of making samba autobuild work out of the
box on FreeBSD.

There are plenty of places in the tests where 'mktemp` is called with
'--tempdir' or '-p' parameter, which are not recognized on FreeBSD(and,
possibly other Unix-like platforms).

This call can be safely replaced with the sequence:

$(TMPDIR="$TEST_VAR_DIR" mktemp -d)

Also, 'sha1sum' on FreeBSD is called just 'sha1', as well as there are
'md5' and 'sha256' commands instead of *sum variants.

So, second patch allows to set environment variable that would override
the name of the sha1sum command.

With regards,
Timur Bakeyev

0001-Made-calls-to-mktemp-compatiable-with-FreeBSD.patch (15K) Download Attachment
0002-Allow-to-set-sha1sum-command-name-through-the-enviro.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Portable mktemp calls in the tests

Samba - samba-technical mailing list
Hi Timur,

On Fri, 22 Dec 2017 02:49:48 +0100, "Timur I. Bakeyev via
samba-technical" <[hidden email]> wrote:

> Here is another piece of puzzle of making samba autobuild work out of the
> box on FreeBSD.
>
> There are plenty of places in the tests where 'mktemp` is called with
> '--tempdir' or '-p' parameter, which are not recognized on FreeBSD(and,
> possibly other Unix-like platforms).
>
> This call can be safely replaced with the sequence:
>
> $(TMPDIR="$TEST_VAR_DIR" mktemp -d)

First the long-winded rant about poor documentation...  :-)

I have a *minor* concern because neither the Linux nor FreeBSD manpages
for mktemp(1) clearly document the general use of TMPDIR.  Use of TMPDIR
is documented in the context of the -t option (and, for Linux, the -p
option).

The FreeBSD manpage and Linux info page each have an example that I
would regard as implying general use of TMPDIR to override the use
of /tmp.

That said, it clearly works on most Unix-related platforms.  Testing
under Linux shows that mktemp uses TMPDIR in general cases and a glance
at the source code confirms this.

Given that I don't know how to write texinfo documentation, I've opened
a bug (https://debbugs.gnu.org/cgi/bugreport.cgi?bug=29807) against the
GNU coreutils package to try to get this documented.  I would encourage
you to do the same for FreeBSD's mktemp.

All that said, even though the documentation of mktemp is poor, I think
this is an improvement and would be happy to give it a positive
review.

However, can you please remove the following change (and those related
to it) from the mktemp commit?

-cat >$tmpeditor <<-'EOF'
+cat >${tmpeditor}.sh <<-'EOF'

If you really meant to do that then can you please make that change
in a separate commit?  :-)

Thanks...

peace & happiness,
martin

Reply | Threaded
Open this post in threaded view
|

Re: Portable mktemp calls in the tests

Samba - samba-technical mailing list
Hi, Martin!

Thanks for the positive feedback!

On Fri, Dec 22, 2017 at 5:05 AM, Martin Schwenke <[hidden email]> wrote:

> Given that I don't know how to write texinfo documentation, I've opened
> a bug (https://debbugs.gnu.org/cgi/bugreport.cgi?bug=29807) against the
> GNU coreutils package to try to get this documented.  I would encourage
> you to do the same for FreeBSD's mktemp.
>

Can you suggest what would be an adequate explanation/example for this use
case?

All that said, even though the documentation of mktemp is poor, I think

> this is an improvement and would be happy to give it a positive
> review.
>
> However, can you please remove the following change (and those related
> to it) from the mktemp commit?
>
> -cat >$tmpeditor <<-'EOF'
> +cat >${tmpeditor}.sh <<-'EOF'
>
> If you really meant to do that then can you please make that change
> in a separate commit?  :-)
>

That's the direct followup of the:

-tmpeditor=$(mktemp --suffix .sh -p $STpath/bin samba-tool-editor-XXXXXXXX)
+tmpeditor=$(TMPDIR=$STpath/bin mktemp samba-tool-editor-XXXXXXXX)

As '--suffix .sh' is also non-portable extension I've implemented similar
functionality
by adding .sh extension to the resulting file. I see, it may have security
implications
as we don't verify that resulting file doesn't exist.

I can remove whole block, as seems, self-test doesn't really use this code
route.

Or, do you have better suggestion for this code part?

With regards,
Timur Bakeyev.
Reply | Threaded
Open this post in threaded view
|

Re: Portable mktemp calls in the tests

Samba - samba-technical mailing list
On Fri, 22 Dec 2017 05:43:31 +0100, "Timur I. Bakeyev"
<[hidden email]> wrote:

> Thanks for the positive feedback!
>
> On Fri, Dec 22, 2017 at 5:05 AM, Martin Schwenke <[hidden email]> wrote:
>
> > Given that I don't know how to write texinfo documentation, I've opened
> > a bug (https://debbugs.gnu.org/cgi/bugreport.cgi?bug=29807) against the
> > GNU coreutils package to try to get this documented.  I would encourage
> > you to do the same for FreeBSD's mktemp.

> Can you suggest what would be an adequate explanation/example for this use
> case?

Actually, I've re-read the documentation (for both GNU coreutils and
FreeBSD) and they both mention what happens if no arguments are
passed.  You just need to work out the implications involving
TMPDIR.

So, no documentation problem apart of lack of readability.

> All that said, even though the documentation of mktemp is poor, I think
> > this is an improvement and would be happy to give it a positive
> > review.
> >
> > However, can you please remove the following change (and those related
> > to it) from the mktemp commit?
> >
> > -cat >$tmpeditor <<-'EOF'
> > +cat >${tmpeditor}.sh <<-'EOF'
> >
> > If you really meant to do that then can you please make that change
> > in a separate commit?  :-)
> >  
>
> That's the direct followup of the:
>
> -tmpeditor=$(mktemp --suffix .sh -p $STpath/bin samba-tool-editor-XXXXXXXX)
> +tmpeditor=$(TMPDIR=$STpath/bin mktemp samba-tool-editor-XXXXXXXX)
>
> As '--suffix .sh' is also non-portable extension I've implemented similar
> functionality
> by adding .sh extension to the resulting file. I see, it may have security
> implications
> as we don't verify that resulting file doesn't exist.
>
> I can remove whole block, as seems, self-test doesn't really use this code
> route.
>
> Or, do you have better suggestion for this code part?

Oh, sorry!  I missed the --suffix part.  Instead of changing all of the
lines that use this variable I would suggest simply adding an extra
line (after the changed line) that says:

  tmpeditor="${tmpeditor}.sh"

:-)

Note there's another problem!  Where a template is used (and,
therefore, non-option arguments are present) TMPDIR is *not* used (at
least in GNU coreutils).  Therefore, the above will create the file in
the current directory!  The obvious solution is to do something like:

  tmpeditor=$(mktemp "$STpath/bin/samba-tool-editor-XXXXXXXX")
  tmpeditor="${tempeditor}.sh"

This is also true for this change:

    export CTDB_PUBLIC_ADDRESSES=$(TMPDIR="$EVENTSCRIPTS_TESTS_VAR_DIR" \
                                        mktemp \
                                       "public-addresses-XXXXXXXX")
It will need to be:

    export CTDB_PUBLIC_ADDRESSES=$(mktemp \
                   "${EVENTSCRIPTS_TESTS_VAR_DIR}/public-addresses-XXXXXXXX")

There might other places that behave unexpectedly because a template is
used.  :-(

peace & happiness,
martin

Reply | Threaded
Open this post in threaded view
|

Re: Portable mktemp calls in the tests

Samba - samba-technical mailing list
On Fri, Dec 22, 2017 at 5:55 AM, Martin Schwenke <[hidden email]> wrote:

> On Fri, 22 Dec 2017 05:43:31 +0100, "Timur I. Bakeyev"
> <[hidden email]> wrote:
>
> > Thanks for the positive feedback!
> >
> > On Fri, Dec 22, 2017 at 5:05 AM, Martin Schwenke <[hidden email]>
> wrote:
>
> Oh, sorry!  I missed the --suffix part.  Instead of changing all of the
> lines that use this variable I would suggest simply adding an extra
> line (after the changed line) that says:
>
>   tmpeditor="${tmpeditor}.sh"
>

Sounds smarter way of doing it :)

Note there's another problem!  Where a template is used (and,
> therefore, non-option arguments are present) TMPDIR is *not* used (at
> least in GNU coreutils).  Therefore, the above will create the file in
> the current directory!  The obvious solution is to do something like:
>
>   tmpeditor=$(mktemp "$STpath/bin/samba-tool-editor-XXXXXXXX")
>   tmpeditor="${tempeditor}.sh"
>

I've tested it, but seems to forgot to properly add it everywhere...

Seems, construction like:

TMPDIR=/var/tmp mktemp -t blah-xxxxx

Note '-t' flag ^

Works both for FreeBSD and Linux(at least, Debian) as intended. Creation of
directories
with extra '-d' also works.

So, let me re-send updated patch.

With best regards,
Timur Bakeyev.
Reply | Threaded
Open this post in threaded view
|

Re: Portable mktemp calls in the tests

Samba - samba-technical mailing list
I'm walking home so can't check right now, but...

Is -t deprecated in Linux? My memory of the source code tells me it is but
I'm not sure how clear the documentation is.

Isn't it amazing how the simplest changes are really hard when you start
taking about portability? :-\

peace & happiness,
martin



On 22 December 2017 16:47:00 "Timur I. Bakeyev" <[hidden email]> wrote:

> On Fri, Dec 22, 2017 at 5:55 AM, Martin Schwenke <[hidden email]> wrote:
>
>> On Fri, 22 Dec 2017 05:43:31 +0100, "Timur I. Bakeyev"
>> <[hidden email]> wrote:
>>
>> > Thanks for the positive feedback!
>> >
>> > On Fri, Dec 22, 2017 at 5:05 AM, Martin Schwenke <[hidden email]>
>> wrote:
>>
>> Oh, sorry!  I missed the --suffix part.  Instead of changing all of the
>> lines that use this variable I would suggest simply adding an extra
>> line (after the changed line) that says:
>>
>>   tmpeditor="${tmpeditor}.sh"
>>
>
> Sounds smarter way of doing it :)
>
> Note there's another problem!  Where a template is used (and,
>> therefore, non-option arguments are present) TMPDIR is *not* used (at
>> least in GNU coreutils).  Therefore, the above will create the file in
>> the current directory!  The obvious solution is to do something like:
>>
>>   tmpeditor=$(mktemp "$STpath/bin/samba-tool-editor-XXXXXXXX")
>>   tmpeditor="${tempeditor}.sh"
>>
>
> I've tested it, but seems to forgot to properly add it everywhere...
>
> Seems, construction like:
>
> TMPDIR=/var/tmp mktemp -t blah-xxxxx
>
> Note '-t' flag ^
>
> Works both for FreeBSD and Linux(at least, Debian) as intended. Creation of
> directories
> with extra '-d' also works.
>
> So, let me re-send updated patch.
>
> With best regards,
> Timur Bakeyev.
Reply | Threaded
Open this post in threaded view
|

Re: Portable mktemp calls in the tests

Samba - samba-technical mailing list
On Fri, Dec 22, 2017 at 6:55 AM, Martin Schwenke <[hidden email]> wrote:

> I'm walking home so can't check right now, but...
>
> Is -t deprecated in Linux? My memory of the source code tells me it is but
> I'm not sure how clear the documentation is.
>

Yeh, it's documented as deprecated... But still works :)

 -t     interpret TEMPLATE as a single file name component, relative to a
directory: $TMPDIR, if set; else the directory specified via -p; else /tmp
[deprecated]

Isn't it amazing how the simplest changes are really hard when you start
> taking about portability? :-\
>

I guess on a long run we'll need some kind of shell compatibility library
that would take care of all those matters(if that's possible at all).

With regards,
Timur Bakeyev.
Reply | Threaded
Open this post in threaded view
|

Re: Portable mktemp calls in the tests

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
Hi Timur,

On Fri, 22 Dec 2017 06:46:24 +0100, "Timur I. Bakeyev"
<[hidden email]> wrote:

> On Fri, Dec 22, 2017 at 5:55 AM, Martin Schwenke <[hidden email]> wrote:
>
> > On Fri, 22 Dec 2017 05:43:31 +0100, "Timur I. Bakeyev"
> > <[hidden email]> wrote:
> >  
> > > Thanks for the positive feedback!
> > >
> > > On Fri, Dec 22, 2017 at 5:05 AM, Martin Schwenke <[hidden email]>  
> > wrote:
> >
> > Oh, sorry!  I missed the --suffix part.  Instead of changing all of the
> > lines that use this variable I would suggest simply adding an extra
> > line (after the changed line) that says:
> >
> >   tmpeditor="${tmpeditor}.sh"
> >  
>
> Sounds smarter way of doing it :)
>
> Note there's another problem!  Where a template is used (and,
> > therefore, non-option arguments are present) TMPDIR is *not* used (at
> > least in GNU coreutils).  Therefore, the above will create the file in
> > the current directory!  The obvious solution is to do something like:
> >
> >   tmpeditor=$(mktemp "$STpath/bin/samba-tool-editor-XXXXXXXX")
> >   tmpeditor="${tempeditor}.sh"
> >  
>
> I've tested it, but seems to forgot to properly add it everywhere...
>
> Seems, construction like:
>
> TMPDIR=/var/tmp mktemp -t blah-xxxxx
>
> Note '-t' flag ^
>
> Works both for FreeBSD and Linux(at least, Debian) as intended. Creation of
> directories
> with extra '-d' also works.
>
> So, let me re-send updated patch.

I've looked at both the GNU and FreeBSD sources for mktemp:

  https://svnweb.freebsd.org/base/release/11.1.0/usr.bin/mktemp/mktemp.c?view=markup

  https://git.savannah.gnu.org/cgit/coreutils.git/tree/src/mktemp.c?h=v8.29&id=27b2b19aa8d8b30b8cb4198b2f4b54568e10a35e

and I have also run some tests with both versions.

It is possible to have portability without using the deprecated -t
option.

* When a template is being used then put the directory inline:

    mktemp /var/tmp/blah-xxxxx

* When a template is not being used then use TMPDIR:

    TMPDIR=/var/tmp mktemp

Both of these appear to work with and without the -d option.

I really don't think we should be using -t, since it deprecated in the
GNU version.

I am wondering if your testing is having problems due to the typo here:

>   tmpeditor=$(mktemp "$STpath/bin/samba-tool-editor-XXXXXXXX")
>   tmpeditor="${tempeditor}.sh"

or if the "tempeditor" typo is only in your email.  :-)

peace & happiness,
martin

Reply | Threaded
Open this post in threaded view
|

Re: Portable mktemp calls in the tests

Samba - samba-technical mailing list
On Tue, 2 Jan 2018 12:54:18 +1100
Martin Schwenke via samba-technical <[hidden email]>
wrote:

> Hi Timur,
>
> On Fri, 22 Dec 2017 06:46:24 +0100, "Timur I. Bakeyev"
> <[hidden email]> wrote:
>
> > On Fri, Dec 22, 2017 at 5:55 AM, Martin Schwenke
> > <[hidden email]> wrote:
> >
> > > On Fri, 22 Dec 2017 05:43:31 +0100, "Timur I. Bakeyev"
> > > <[hidden email]> wrote:
> > >  
> > > > Thanks for the positive feedback!
> > > >
> > > > On Fri, Dec 22, 2017 at 5:05 AM, Martin Schwenke
> > > > <[hidden email]>  
> > > wrote:
> > >
> > > Oh, sorry!  I missed the --suffix part.  Instead of changing all
> > > of the lines that use this variable I would suggest simply adding
> > > an extra line (after the changed line) that says:
> > >
> > >   tmpeditor="${tmpeditor}.sh"
> > >  
> >
> > Sounds smarter way of doing it :)
> >
> > Note there's another problem!  Where a template is used (and,
> > > therefore, non-option arguments are present) TMPDIR is *not* used
> > > (at least in GNU coreutils).  Therefore, the above will create
> > > the file in the current directory!  The obvious solution is to do
> > > something like:
> > >
> > >   tmpeditor=$(mktemp "$STpath/bin/samba-tool-editor-XXXXXXXX")
> > >   tmpeditor="${tempeditor}.sh"
> > >  
> >
> > I've tested it, but seems to forgot to properly add it everywhere...
> >
> > Seems, construction like:
> >
> > TMPDIR=/var/tmp mktemp -t blah-xxxxx
> >
> > Note '-t' flag ^
> >
> > Works both for FreeBSD and Linux(at least, Debian) as intended.
> > Creation of directories
> > with extra '-d' also works.
> >
> > So, let me re-send updated patch.
>
> I've looked at both the GNU and FreeBSD sources for mktemp:
>
>   https://svnweb.freebsd.org/base/release/11.1.0/usr.bin/mktemp/mktemp.c?view=markup
>
>   https://git.savannah.gnu.org/cgit/coreutils.git/tree/src/mktemp.c?h=v8.29&id=27b2b19aa8d8b30b8cb4198b2f4b54568e10a35e
>
> and I have also run some tests with both versions.
>
> It is possible to have portability without using the deprecated -t
> option.
>
> * When a template is being used then put the directory inline:
>
>     mktemp /var/tmp/blah-xxxxx
>
> * When a template is not being used then use TMPDIR:
>
>     TMPDIR=/var/tmp mktemp
>
> Both of these appear to work with and without the -d option.
>
> I really don't think we should be using -t, since it deprecated in the
> GNU version.
>
> I am wondering if your testing is having problems due to the typo
> here:
>
> >   tmpeditor=$(mktemp "$STpath/bin/samba-tool-editor-XXXXXXXX")
> >   tmpeditor="${tempeditor}.sh"
>
> or if the "tempeditor" typo is only in your email.  :-)
>
> peace & happiness,
> martin
>

This is all my fault, I wrote the script in question.
I initially used a hardcoded '/tmp/editor.sh', but was advised that
this wasn't a good idea, I changed it to what is there now (without
really thinking it through or considering portability)

With hindsight and thought, it would have been better to use:

tmpeditor=$(mktemp /tmp/samba-tool-editor-XXXXXXXX)

or

tmpeditor=$(mktemp /var/tmp/samba-tool-editor-XXXXXXXX)

Adding the '.sh' suffix is a bit of a red herring, 'scriptname' will
work just as well as 'scriptname.sh'

Rowland

Reply | Threaded
Open this post in threaded view
|

Re: Portable mktemp calls in the tests

Samba - samba-technical mailing list
On Tue, 2 Jan 2018 09:56:48 +0000, Rowland Penny <[hidden email]>
wrote:

> On Tue, 2 Jan 2018 12:54:18 +1100
> Martin Schwenke via samba-technical <[hidden email]>
> wrote:
>
> > Hi Timur,
> >
> > On Fri, 22 Dec 2017 06:46:24 +0100, "Timur I. Bakeyev"
> > <[hidden email]> wrote:
> >  
> > > On Fri, Dec 22, 2017 at 5:55 AM, Martin Schwenke
> > > <[hidden email]> wrote:
> > >  
>  [...]  
>  [...]  
>  [...]  
> > >
> > > Sounds smarter way of doing it :)
> > >
> > > Note there's another problem!  Where a template is used (and,  
>  [...]  
> > >
> > > I've tested it, but seems to forgot to properly add it everywhere...
> > >
> > > Seems, construction like:
> > >
> > > TMPDIR=/var/tmp mktemp -t blah-xxxxx
> > >
> > > Note '-t' flag ^
> > >
> > > Works both for FreeBSD and Linux(at least, Debian) as intended.
> > > Creation of directories
> > > with extra '-d' also works.
> > >
> > > So, let me re-send updated patch.  
> >
> > I've looked at both the GNU and FreeBSD sources for mktemp:
> >
> >   https://svnweb.freebsd.org/base/release/11.1.0/usr.bin/mktemp/mktemp.c?view=markup
> >
> >   https://git.savannah.gnu.org/cgit/coreutils.git/tree/src/mktemp.c?h=v8.29&id=27b2b19aa8d8b30b8cb4198b2f4b54568e10a35e
> >
> > and I have also run some tests with both versions.
> >
> > It is possible to have portability without using the deprecated -t
> > option.
> >
> > * When a template is being used then put the directory inline:
> >
> >     mktemp /var/tmp/blah-xxxxx
> >
> > * When a template is not being used then use TMPDIR:
> >
> >     TMPDIR=/var/tmp mktemp
> >
> > Both of these appear to work with and without the -d option.
> >
> > I really don't think we should be using -t, since it deprecated in the
> > GNU version.
> >
> > I am wondering if your testing is having problems due to the typo
> > here:
> >  
> > >   tmpeditor=$(mktemp "$STpath/bin/samba-tool-editor-XXXXXXXX")
> > >   tmpeditor="${tempeditor}.sh"  
> >
> > or if the "tempeditor" typo is only in your email.  :-)
> >
> > peace & happiness,
> > martin
> >  
>
> This is all my fault, I wrote the script in question.
> I initially used a hardcoded '/tmp/editor.sh', but was advised that
> this wasn't a good idea, I changed it to what is there now (without
> really thinking it through or considering portability)
>
> With hindsight and thought, it would have been better to use:
>
> tmpeditor=$(mktemp /tmp/samba-tool-editor-XXXXXXXX)
>
> or
>
> tmpeditor=$(mktemp /var/tmp/samba-tool-editor-XXXXXXXX)
>
> Adding the '.sh' suffix is a bit of a red herring, 'scriptname' will
> work just as well as 'scriptname.sh'

I think that if we make portability changes to the scripts then we
shouldn't change behaviour in the same commit.  If we decide to drop
the ".sh" suffix then we should probably do that explicitly in a
separate commit, either before or afterwards.

That said, I don't think the ".sh" suffix is a problem.  It means the
temporary filename is self-documenting.

It is the wide range of non-portable options passed to mktemp that is
the problem... and I've certainly been using them...  :-)

peace & happiness,
martin