Re: [PATCH 01/12] Unit tests for basenamecpy

From: Martin Wilck <mwilck@suse.com>
To: Benjamin Marzinski <bmarzins@redhat.com>, dm-devel@redhat.com
Cc: christophe.varoqui@free.fr
Subject: Re: [PATCH 01/12] Unit tests for basenamecpy
Date: Mon, 19 Mar 2018 11:05:44 +0100	[thread overview]
Message-ID: <1521453944.3798.9.camel@suse.com> (raw)
In-Reply-To: <1521452961.3798.6.camel@suse.com>

On Mon, 2018-03-19 at 10:49 +0100, Martin Wilck wrote:
> On Wed, 2018-03-14 at 12:46 -0500, Benjamin Marzinski wrote:
> > The current implementation of basenamecpy is broken, so some of
> > these
> > tests currently fail. Fixes to follow.
> > 
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> >  tests/Makefile |   2 +-
> >  tests/util.c   | 167
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 168 insertions(+), 1 deletion(-)
> >  create mode 100644 tests/util.c
> > 
> > +static void test_basenamecpy_good6(void **state)
> > +{
> > +        char dst[6];
> > +
> > +        assert_int_equal(basenamecpy("/xyzzy/plugh   ", dst,
> > sizeof(dst)), 5);
> > +        assert_string_equal(dst, "plugh");
> > +}
> 
> This deserves explanation. "basename" wouldn't normally strip
> trailing
> whitespace.
> 
> > +/* ends in slash */
> > +static void test_basenamecpy_bad1(void **state)
> > +{
> > +        char dst[10];
> > +
> > +        assert_int_equal(basenamecpy("foo/bar/", dst,
> > sizeof(dst)),
> > 0);
> 
> This, too, deviates from standard "basename" behavior and should be
> explained ("basename /usr/" yields "usr", so does "basename
> /usr///").

I didn't realize the difference between POSIX "basename" and GNU
"basename", and was mislead by the shell behavior. So this and the
following nitpick about "/" can be disregarded. But please add a
comment for the whitespace stripping.

Apart from that, ACK.

Martin

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel