| Message ID | 1456411743-17741-4-git-send-email-george.dunlap@eu.citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
George Dunlap writes ("[PATCH 3/8] tools/xenalyze: Handle fstat errors properly"):
> They're pretty unlikely to fail, but doesn't hurt to check.
...
> - fstat(fd, &s);
> + if ( fstat(fd, &s) ) {
> + perror("fstat");
> + free(h);
> + h = NULL;
> + goto out;
> + }
Is there some reason why you're not calling exit ? mread64 already
does so on mmap failure.
For that matter, is error() not available in this file ?
> struct stat s;
> - fstat(G.fd, &s);
> + if ( fstat(G.fd, &s) ) {
> + perror("fstat");
> + error(ERR_SYSTEM, NULL);
> + }
> G.file_size = s.st_size;
This hunk LGTM.
Ian.
On 26/02/16 12:25, Ian Jackson wrote: > George Dunlap writes ("[PATCH 3/8] tools/xenalyze: Handle fstat errors properly"): >> They're pretty unlikely to fail, but doesn't hurt to check. > ... >> - fstat(fd, &s); >> + if ( fstat(fd, &s) ) { >> + perror("fstat"); >> + free(h); >> + h = NULL; >> + goto out; >> + } > > Is there some reason why you're not calling exit ? mread64 already > does so on mmap failure. > > For that matter, is error() not available in this file ? I originally meant this to be library-like (a faster implementation of pread actually). So it doesn't have error(), and for consistency probably shouldn't be calling exit() on mmap() failure. Is there a reason not to just return null and let the caller figure out what to do? -George
diff --git a/tools/xentrace/mread.c b/tools/xentrace/mread.c index a22c4ea..c6f6ec6 100644 --- a/tools/xentrace/mread.c +++ b/tools/xentrace/mread.c @@ -24,9 +24,16 @@ mread_handle_t mread_init(int fd) h->fd = fd; - fstat(fd, &s); + if ( fstat(fd, &s) ) { + perror("fstat"); + free(h); + h = NULL; + goto out; + } + h->file_size = s.st_size; +out: return h; } diff --git a/tools/xentrace/xenalyze.c b/tools/xentrace/xenalyze.c index 33f8129..9f8c065 100644 --- a/tools/xentrace/xenalyze.c +++ b/tools/xentrace/xenalyze.c @@ -10348,7 +10348,10 @@ int main(int argc, char *argv[]) { error(ERR_SYSTEM, NULL); } else { struct stat s; - fstat(G.fd, &s); + if ( fstat(G.fd, &s) ) { + perror("fstat"); + error(ERR_SYSTEM, NULL); + } G.file_size = s.st_size; }
They're pretty unlikely to fail, but doesn't hurt to check. CID 1311502 CID 1311501 Signed-off-by: George Dunlap <george.dunlap@citrix.com> --- CC: Ian Jackson <ian.jackson@citrix.com> CC: Wei Liu <wei.liu2@citrix.com> --- tools/xentrace/mread.c | 9 ++++++++- tools/xentrace/xenalyze.c | 5 ++++- 2 files changed, 12 insertions(+), 2 deletions(-)