[1/2] libfsimage: replace deprecated readdir_r() with readdir()

Commit Message

Chris Patterson May 30, 2016, 2:32 a.m. UTC

From: Chris Patterson <[e-mail]>

Replace the usage of readdir_r() with readdir() to address
a compilation error due to the deprecation of readdir_r.

glibc has deprecated this for their next release (2.24):
https://sourceware.org/bugzilla/show_bug.cgi?id=19056

Signed-off-by: Chris Patterson <[e-mail]>
---
 tools/libfsimage/common/fsimage_plugin.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

George Dunlap May 31, 2016, 10:42 a.m. UTC | #1

On Mon, May 30, 2016 at 3:32 AM, Chris Patterson <[e-mail]> wrote:
> From: Chris Patterson <[e-mail]>
>
> Replace the usage of readdir_r() with readdir() to address
> a compilation error due to the deprecation of readdir_r.
>
> glibc has deprecated this for their next release (2.24):
> https://sourceware.org/bugzilla/show_bug.cgi?id=19056
>
> Signed-off-by: Chris Patterson <[e-mail]>

Thanks for the patch, Chris.  A bit more background would have been
helpful -- I did some searching and found a description[1] which says:

In the current POSIX.1 specification (POSIX.1-2008), readdir(3) is
not required to be thread-safe.  However, in modern
implementations (including the glibc implementation), concurrent
calls to readdir(3) that specify different directory streams are
thread-safe.  Therefore, the use of readdir_r() is generally
unnecessary in multithreaded programs.  In cases where multiple
threads must read from the same directory stream, using readdir(3)
with external synchronization is still preferable to the use of
readdir_r(), for the reasons given in the points above.

The use of the specific directory stream is single-threaded, so for
glibc, it looks like using readdir() will be safe.  But libxl needs to
be able to build on a number of libc's that are not glibc and still be
thread-safe.  So we need to either 1) verify that readdir() is thread
safe on all libc's against which we may compile, or 2) add some
#ifdef-ery to switch to readdir_r() on systems unless we know it's
thread-safe.

I'm less familiar with best practices for this kind of thing -- Ian,
do you have an idea?

Thanks again for raising this, Chris.

 -George

[1] http://man7.org/linux/man-pages/man3/readdir_r.3.html

> ---
>  tools/libfsimage/common/fsimage_plugin.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/libfsimage/common/fsimage_plugin.c b/tools/libfsimage/common/fsimage_plugin.c
> index 3fa06c7..03212e1 100644
> --- a/tools/libfsimage/common/fsimage_plugin.c
> +++ b/tools/libfsimage/common/fsimage_plugin.c
> @@ -147,7 +147,7 @@ static int load_plugins(void)
>
>         bzero(dp, sizeof (struct dirent) + name_max + 1);
>
> -       while (readdir_r(dir, dp, &dpp) == 0 && dpp != NULL) {
> +       while ((dpp = readdir(dir)) != NULL) {
>                 if (strcmp(dpp->d_name, ".") == 0)
>                         continue;
>                 if (strcmp(dpp->d_name, "..") == 0)
> --
> 2.1.4
>
>
> _______________________________________________
> Xen-devel mailing list
> [e-mail]
> http://lists.xen.org/xen-devel

Chris Patterson May 31, 2016, 3:43 p.m. UTC | #2

On Tue, May 31, 2016 at 6:42 AM, George Dunlap <[e-mail]> wrote:
> On Mon, May 30, 2016 at 3:32 AM, Chris Patterson <[e-mail]> wrote:
>> From: Chris Patterson <[e-mail]>
>>
>> Replace the usage of readdir_r() with readdir() to address
>> a compilation error due to the deprecation of readdir_r.
>>
>> glibc has deprecated this for their next release (2.24):
>> https://sourceware.org/bugzilla/show_bug.cgi?id=19056
>>
>> Signed-off-by: Chris Patterson <[e-mail]>
>
> Thanks for the patch, Chris.  A bit more background would have been
> helpful -- I did some searching and found a description[1] which says:
>

Thank you. I should have added these details in the commit message or cover.

> In the current POSIX.1 specification (POSIX.1-2008), readdir(3) is
> not required to be thread-safe.  However, in modern
> implementations (including the glibc implementation), concurrent
> calls to readdir(3) that specify different directory streams are
> thread-safe.  Therefore, the use of readdir_r() is generally
> unnecessary in multithreaded programs.  In cases where multiple
> threads must read from the same directory stream, using readdir(3)
> with external synchronization is still preferable to the use of
> readdir_r(), for the reasons given in the points above.
>
> The use of the specific directory stream is single-threaded, so for
> glibc, it looks like using readdir() will be safe.  But libxl needs to
> be able to build on a number of libc's that are not glibc and still be
> thread-safe.  So we need to either 1) verify that readdir() is thread
> safe on all libc's against which we may compile, or 2) add some

Is there a list of supported libc libraries?  I can look into it and
provide more definitive answers if there are.

> #ifdef-ery to switch to readdir_r() on systems unless we know it's
> thread-safe.
>
> I'm less familiar with best practices for this kind of thing -- Ian,
> do you have an idea?
>
> Thanks again for raising this, Chris.
>
>  -George
>
> [1] http://man7.org/linux/man-pages/man3/readdir_r.3.html
>

Wei Liu May 31, 2016, 5:53 p.m. UTC | #3

On Tue, May 31, 2016 at 11:43:13AM -0400, Chris Patterson wrote:
> On Tue, May 31, 2016 at 6:42 AM, George Dunlap <[e-mail]> wrote:
> > On Mon, May 30, 2016 at 3:32 AM, Chris Patterson <[e-mail]> wrote:
> >> From: Chris Patterson <[e-mail]>
> >>
> >> Replace the usage of readdir_r() with readdir() to address
> >> a compilation error due to the deprecation of readdir_r.
> >>
> >> glibc has deprecated this for their next release (2.24):
> >> https://sourceware.org/bugzilla/show_bug.cgi?id=19056
> >>
> >> Signed-off-by: Chris Patterson <[e-mail]>
> >
> > Thanks for the patch, Chris.  A bit more background would have been
> > helpful -- I did some searching and found a description[1] which says:
> >
> 
> Thank you. I should have added these details in the commit message or cover.
> 
> > In the current POSIX.1 specification (POSIX.1-2008), readdir(3) is
> > not required to be thread-safe.  However, in modern
> > implementations (including the glibc implementation), concurrent
> > calls to readdir(3) that specify different directory streams are
> > thread-safe.  Therefore, the use of readdir_r() is generally
> > unnecessary in multithreaded programs.  In cases where multiple
> > threads must read from the same directory stream, using readdir(3)
> > with external synchronization is still preferable to the use of
> > readdir_r(), for the reasons given in the points above.
> >
> > The use of the specific directory stream is single-threaded, so for
> > glibc, it looks like using readdir() will be safe.  But libxl needs to
> > be able to build on a number of libc's that are not glibc and still be
> > thread-safe.  So we need to either 1) verify that readdir() is thread
> > safe on all libc's against which we may compile, or 2) add some
> 
> Is there a list of supported libc libraries?  I can look into it and
> provide more definitive answers if there are.
> 

We at least care about FreeBSD and NetBSD. Sadly their manuaks don't
provide statement regarding thread safety for readdir and readdir_r.
It's better to err on the safe side.

Wei.

> > #ifdef-ery to switch to readdir_r() on systems unless we know it's
> > thread-safe.
> >
> > I'm less familiar with best practices for this kind of thing -- Ian,
> > do you have an idea?
> >
> > Thanks again for raising this, Chris.
> >
> >  -George
> >
> > [1] http://man7.org/linux/man-pages/man3/readdir_r.3.html
> >

Chris Patterson May 31, 2016, 8:37 p.m. UTC | #4

On Tue, May 31, 2016 at 1:53 PM, Wei Liu <[e-mail]> wrote:
> On Tue, May 31, 2016 at 11:43:13AM -0400, Chris Patterson wrote:
>> On Tue, May 31, 2016 at 6:42 AM, George Dunlap <[e-mail]> wrote:
>> > On Mon, May 30, 2016 at 3:32 AM, Chris Patterson <[e-mail]> wrote:
>> >> From: Chris Patterson <[e-mail]>
>> >>
>> >> Replace the usage of readdir_r() with readdir() to address
>> >> a compilation error due to the deprecation of readdir_r.
>> >>
>> >> glibc has deprecated this for their next release (2.24):
>> >> https://sourceware.org/bugzilla/show_bug.cgi?id=19056
>> >>
>> >> Signed-off-by: Chris Patterson <[e-mail]>
>> >
>> > Thanks for the patch, Chris.  A bit more background would have been
>> > helpful -- I did some searching and found a description[1] which says:
>> >
>>
>> Thank you. I should have added these details in the commit message or cover.
>>
>> > In the current POSIX.1 specification (POSIX.1-2008), readdir(3) is
>> > not required to be thread-safe.  However, in modern
>> > implementations (including the glibc implementation), concurrent
>> > calls to readdir(3) that specify different directory streams are
>> > thread-safe.  Therefore, the use of readdir_r() is generally
>> > unnecessary in multithreaded programs.  In cases where multiple
>> > threads must read from the same directory stream, using readdir(3)
>> > with external synchronization is still preferable to the use of
>> > readdir_r(), for the reasons given in the points above.
>> >
>> > The use of the specific directory stream is single-threaded, so for
>> > glibc, it looks like using readdir() will be safe.  But libxl needs to
>> > be able to build on a number of libc's that are not glibc and still be
>> > thread-safe.  So we need to either 1) verify that readdir() is thread
>> > safe on all libc's against which we may compile, or 2) add some
>>
>> Is there a list of supported libc libraries?  I can look into it and
>> provide more definitive answers if there are.
>>
>
> We at least care about FreeBSD and NetBSD. Sadly their manuaks don't
> provide statement regarding thread safety for readdir and readdir_r.
> It's better to err on the safe side.
>

I'm far from the expert here, but it would appear that both NetBSD's
and FreeBSD's libc readdir()/readdir_r() implementations are
consistent in their locking mechanisms [1,2].  As such, I would expect
readdir() to be equally as safe as readdir_r() for their users.  As
you noted, there does not appear to be any documented distinction
within their man pages [3,4] with regards to thread safety and it
seems reasonable that they would have documented these limitations, if
present.

[1] FreeBSD: https://svnweb.freebsd.org/base/head/lib/libc/gen/readdir.c?view=markup#l98
[2] NetBSD: http://cvsweb.netbsd.org/bsdweb.cgi/src/lib/libc/gen/readdir.c?rev=1.25.6.1&content-type=text/x-cvsweb-markup
[3] FreeBSD readdir manpage:
https://www.freebsd.org/cgi/man.cgi?query=readdir&section=3
[4] NetBSD readdir manpage: http://netbsd.gw.com/cgi-bin/man-cgi?readdir

Are there other known supported libc implementations?

Ian Jackson June 1, 2016, 11:06 a.m. UTC | #5

Chris Patterson writes ("Re: [Xen-devel] [PATCH 1/2] libfsimage: replace deprecated readdir_r() with readdir()"):
> I'm far from the expert here, but it would appear that both NetBSD's
> and FreeBSD's libc readdir()/readdir_r() implementations are
> consistent in their locking mechanisms [1,2].  As such, I would expect
> readdir() to be equally as safe as readdir_r() for their users.  As
> you noted, there does not appear to be any documented distinction
> within their man pages [3,4] with regards to thread safety and it
> seems reasonable that they would have documented these limitations, if
> present.

Thanks for chasing up this breakage.  However, I think that:

1. This is a fundamentally wrong way to go about assessing the proper
way to use a supposedly-portable API.  The right way is to refer to
the published specification, in the first instance.  That published
specification says that readdir() is not threadsafe.

2. There may be good reasons to deviate from a formal specification.
Formal specifications can be wrong (for example, they can differ from
established practice, or unuseable, or incoherent).  But there has
been no discussion (at least in this thread on xen-devel) which might
suggest that the POSIX specification is wrongheaded here.

3. Perhaps the documentation accompanying, or discussion justifying,
the glibc readdir deprecation warning, will provide something
resembling what I discuss in (2).

4. I am not satisfied with an approach which enumerates all the
currently-supported dom0 operating systems.  These patches should be
accompanied by an explantion of a good reason to believe that all
operating systems we are likely to ever want to run on (or be able to
run on) will also provide a threadsafe readdir.

5. I am not opposed to replacing readdir_r with readdir in areas of
code which do not need the threadsafety properties.  (Eg, in xl.)

Thanks,
Ian.

Ian Jackson June 1, 2016, 12:04 p.m. UTC | #6

Ian Jackson writes ("Re: [Xen-devel] [PATCH 1/2] libfsimage: replace deprecated readdir_r() with readdir()"):
> 2. There may be good reasons to deviate from a formal specification.
> Formal specifications can be wrong (for example, they can differ from
> established practice, or unuseable, or incoherent).  But there has
> been no discussion (at least in this thread on xen-devel) which might
> suggest that the POSIX specification is wrongheaded here.

I have been helpfully referred by a local irc channel to the following
attempt to change posix to require that readdir() is threadsafe in the
senses required by libx, and to deprecate readdir_r():

 http://austingroupbugs.net/view.php?id=696

I find the comment 0001606 by "dalias" (et seq) totally convincing.
The published specification of readdir_r is indeed incoherent.  And
only contrived implementations of readdir will not be threadsafe in
the required sense.

> 3. Perhaps the documentation accompanying, or discussion justifying,
> the glibc readdir deprecation warning, will provide something
> resembling what I discuss in (2).
> 
> 4. I am not satisfied with an approach which enumerates all the
> currently-supported dom0 operating systems.  These patches should be
> accompanied by an explantion of a good reason to believe that all
> operating systems we are likely to ever want to run on (or be able to
> run on) will also provide a threadsafe readdir.

I think the comments by dalias in the thread above, provide exactly
the justification I wanted in my paragraphs 2-4 above.

Accordingly, I think all occurrences of readdir_r in our codebase
should be replaced by readdir, as proposed by Chris.

However, I think the patch is not quite complete, as the change from
readdir_r to readdir should also involve removing the local dirent
variables associated with each call site.

Thanks,
Ian.

Ian Jackson June 1, 2016, 12:06 p.m. UTC | #7

Ian Jackson writes ("Re: [Xen-devel] [PATCH 1/2] libfsimage: replace deprecated readdir_r() with readdir()"):
> Ian Jackson writes ("Re: [Xen-devel] [PATCH 1/2] libfsimage: replace deprecated readdir_r() with readdir()"):
> > 2. There may be good reasons to deviate from a formal specification.
> > Formal specifications can be wrong (for example, they can differ from
> > established practice, or unuseable, or incoherent).  But there has
> > been no discussion (at least in this thread on xen-devel) which might
> > suggest that the POSIX specification is wrongheaded here.
> 
> I have been helpfully referred by a local irc channel to the following
> attempt to change posix to require that readdir() is threadsafe in the
> senses required by libx, and to deprecate readdir_r():
> 
>  http://austingroupbugs.net/view.php?id=696
> 
> I find the comment 0001606 by "dalias" (et seq) totally convincing.
> The published specification of readdir_r is indeed incoherent.  And
> only contrived implementations of readdir will not be threadsafe in
> the required sense.
...
> Accordingly, I think all occurrences of readdir_r in our codebase
> should be replaced by readdir, as proposed by Chris.
> 
> However, I think the patch is not quite complete, as the change from
> readdir_r to readdir should also involve removing the local dirent
> variables associated with each call site.

Also, the commit message needs to be expanded to provide the
rationale.  It should restate the reasoning provided by "dalias" and
provide links to the austingroupbugs thread and references to the
comment numbers.

Ian.

Chris Patterson June 1, 2016, 1:55 p.m. UTC | #8

On Wed, Jun 1, 2016 at 8:06 AM, Ian Jackson <[e-mail]> wrote:
> Ian Jackson writes ("Re: [Xen-devel] [PATCH 1/2] libfsimage: replace deprecated readdir_r() with readdir()"):
>> Ian Jackson writes ("Re: [Xen-devel] [PATCH 1/2] libfsimage: replace deprecated readdir_r() with readdir()"):
>> > 2. There may be good reasons to deviate from a formal specification.
>> > Formal specifications can be wrong (for example, they can differ from
>> > established practice, or unuseable, or incoherent).  But there has
>> > been no discussion (at least in this thread on xen-devel) which might
>> > suggest that the POSIX specification is wrongheaded here.
>>
>> I have been helpfully referred by a local irc channel to the following
>> attempt to change posix to require that readdir() is threadsafe in the
>> senses required by libx, and to deprecate readdir_r():
>>
>>  http://austingroupbugs.net/view.php?id=696
>>
>> I find the comment 0001606 by "dalias" (et seq) totally convincing.
>> The published specification of readdir_r is indeed incoherent.  And
>> only contrived implementations of readdir will not be threadsafe in
>> the required sense.
> ...
>> Accordingly, I think all occurrences of readdir_r in our codebase
>> should be replaced by readdir, as proposed by Chris.
>>
>> However, I think the patch is not quite complete, as the change from
>> readdir_r to readdir should also involve removing the local dirent
>> variables associated with each call site.
>
> Also, the commit message needs to be expanded to provide the
> rationale.  It should restate the reasoning provided by "dalias" and
> provide links to the austingroupbugs thread and references to the
> comment numbers.
>
> Ian.

Agreed, will post a v2.  Thanks for the feedback.

Patch

diff --git a/tools/libfsimage/common/fsimage_plugin.c b/tools/libfsimage/common/fsimage_plugin.c
index 3fa06c7..03212e1 100644
--- a/tools/libfsimage/common/fsimage_plugin.c
+++ b/tools/libfsimage/common/fsimage_plugin.c
@@ -147,7 +147,7 @@  static int load_plugins(void)
 
 	bzero(dp, sizeof (struct dirent) + name_max + 1);
 
-	while (readdir_r(dir, dp, &dpp) == 0 && dpp != NULL) {
+	while ((dpp = readdir(dir)) != NULL) {
 		if (strcmp(dpp->d_name, ".") == 0)
 			continue;
 		if (strcmp(dpp->d_name, "..") == 0)