[Top] [All Lists]

Re: [PATCH v2 2/7] xfsdump: process EPIPE instead of catching SIGPIPE

To: Bill Kendall <wkendall@xxxxxxx>
Subject: Re: [PATCH v2 2/7] xfsdump: process EPIPE instead of catching SIGPIPE
From: Alex Elder <aelder@xxxxxxx>
Date: Tue, 9 Aug 2011 17:40:10 -0500
Cc: <xfs@xxxxxxxxxxx>
In-reply-to: <1312497011-24840-3-git-send-email-wkendall@xxxxxxx>
References: <1312497011-24840-1-git-send-email-wkendall@xxxxxxx> <1312497011-24840-3-git-send-email-wkendall@xxxxxxx>
Reply-to: <aelder@xxxxxxx>
On Thu, 2011-08-04 at 17:30 -0500, Bill Kendall wrote:
> Looking forward towards a multi-threaded xfsdump, it's simpler to
> handle pipe failures as a system call failure (EPIPE) rather than
> through a signal handler which may run in a separate thread. The
> existing error handling code handles EPIPE just fine, so the only
> required change is to ignore SIGPIPE. Some sections of code already
> temporarily ignore SIGPIPE -- they no longer need to do so since it
> will already be ignored.
> Signed-off-by: Bill Kendall <wkendall@xxxxxxx>

I don't know the real structure of the code well enough
to verify your statement that it "handles EPIPE just fine."

I see that only _rmt_open() calls pipe(2), setting up a
pipeline between an rsh (child process) and the the code
in _rmt_command().  Thereafter, only _rmt_command() and
_rmt_write() write to the write side of that pipe, and
both of those abort the dump if a write doesn't result
in the expected number of bytes being written.

I see only restore_spec() opens a socket, but it closes
it again.  So I guess I don't see any place that I
expect would produce a EPIPE that doesn't handle it

In any case, I assume your statement is right, and
with that in mind I think the change looks good.

Reviewed-by: Alex Elder <aelder@xxxxxxx>

PS  I'm in the midst of reviewing patch 3 but I'm out
    of time and will have to pick it up again later.

<Prev in Thread] Current Thread [Next in Thread>