[PATCH] more off_t correctness

Discussion:

Johannes Weißl

2011-05-25 20:13:29 UTC

---
aac.c | 4 ++--
ape.c | 11 ++++++-----
flac.c | 4 ++--
id3.c | 8 ++++----
modplug.c | 7 ++++---
mpc.c | 4 +---
nomad.c | 12 ++++++------
nomad.h | 2 +-
vorbis.c | 12 ++++++------
wav.c | 19 +++++++++----------
wavpack.c | 10 ++++++----
11 files changed, 47 insertions(+), 46 deletions(-)

diff --git a/aac.c b/aac.c
index 1f658b8..23a3bd2 100644
--- a/aac.c
+++ b/aac.c
@@ -428,12 +428,12 @@ static int aac_duration(struct input_plugin_data *ip_data)
off_t file_size;

file_size = lseek(ip_data->fd, 0, SEEK_END);
- if (file_size == -1)
+ if (file_size == (off_t) -1)
return -IP_ERROR_FUNCTION_NOT_SUPPORTED;

/* Seek to the middle of the file. There is almost always silence at
* the beginning, which gives wrong results. */
- if (lseek(ip_data->fd, file_size/2, SEEK_SET) == -1)
+ if (lseek(ip_data->fd, file_size/2, SEEK_SET) == (off_t) -1)
return -IP_ERROR_FUNCTION_NOT_SUPPORTED;

priv->rbuf_pos = 0;
diff --git a/ape.c b/ape.c
index 9e8bb5f..ee5ec5c 100644
--- a/ape.c
+++ b/ape.c
@@ -45,7 +45,7 @@ static int find_ape_tag_slow(int fd)
int pos = 0;

/* seek to start of file */
- if (lseek(fd, pos, SEEK_SET) == -1)
+ if (lseek(fd, pos, SEEK_SET) == (off_t) -1)
return -1;

while (1) {
@@ -102,7 +102,7 @@ static int find_ape_tag(int fd, struct ape_header *h, int slow)
{
int pos;

- if (lseek(fd, -HEADER_SIZE, SEEK_END) == -1)
+ if (lseek(fd, -HEADER_SIZE, SEEK_END) == (off_t) -1)
return 0;
if (read_header(fd, h))
return 1;
@@ -113,7 +113,7 @@ static int find_ape_tag(int fd, struct ape_header *h, int slow)
pos = find_ape_tag_slow(fd);
if (pos == -1)
return 0;
- if (lseek(fd, pos, SEEK_SET) == -1)
+ if (lseek(fd, pos, SEEK_SET) == (off_t) -1)
return 0;
return read_header(fd, h);
}
@@ -211,7 +211,8 @@ static off_t get_size(int fd)
int ape_read_tags(struct apetag *ape, int fd, int slow)
{
struct ape_header *h = &ape->header;
- int old_pos, rc = -1;
+ int rc = -1;
+ off_t old_pos;

/* save position */
old_pos = lseek(fd, 0, SEEK_CUR);
@@ -221,7 +222,7 @@ int ape_read_tags(struct apetag *ape, int fd, int slow)

if (AF_IS_FOOTER(h->flags)) {
/* seek back right after the header */
- if (lseek(fd, get_size(fd) - h->size, SEEK_SET) == -1)
+ if (lseek(fd, get_size(fd) - h->size, SEEK_SET) == (off_t) -1)
goto fail;
}

diff --git a/flac.c b/flac.c
index be05219..68fdd34 100644
--- a/flac.c
+++ b/flac.c
@@ -150,7 +150,7 @@ static T(SeekStatus) seek_cb(const Dec *dec, uint64_t offset, void *data)
if (priv->len == UINT64_MAX)
return E(SEEK_STATUS_ERROR);
off = lseek(ip_data->fd, offset, SEEK_SET);
- if (off == -1) {
+ if (off == (off_t) -1) {
return E(SEEK_STATUS_ERROR);
}
priv->pos = off;
@@ -412,7 +412,7 @@ static int flac_open(struct input_plugin_data *ip_data)
} else {
off_t off = lseek(ip_data->fd, 0, SEEK_END);

- if (off == -1 || lseek(ip_data->fd, 0, SEEK_SET) == -1) {
+ if (off == (off_t) -1 || lseek(ip_data->fd, 0, SEEK_SET) == (off_t) -1) {
int save = errno;

F(delete)(dec);
diff --git a/id3.c b/id3.c
index 315163c..fb645c6 100644
--- a/id3.c
+++ b/id3.c
@@ -1127,7 +1127,7 @@ int id3_read_tags(struct id3tag *id3, int fd, unsigned int flags)
/* get v2 from end and optionally v1 */

off = lseek(fd, -138, SEEK_END);
- if (off == -1)
+ if (off == (off_t) -1)
goto error;
rc = read_all(fd, buf, 138);
if (rc == -1)
@@ -1141,7 +1141,7 @@ int id3_read_tags(struct id3tag *id3, int fd, unsigned int flags)
if (v2_footer_parse(&header, buf)) {
/* footer at end of file - 128 */
off = lseek(fd, -((off_t) header.size + 138), SEEK_END);
- if (off == -1)
+ if (off == (off_t) -1)
goto error;
rc = v2_read(id3, fd, &header);
if (rc)
@@ -1150,7 +1150,7 @@ int id3_read_tags(struct id3tag *id3, int fd, unsigned int flags)
} else if (v2_footer_parse(&header, buf + 128)) {
/* footer at end of file */
off = lseek(fd, -((off_t) header.size + 10), SEEK_END);
- if (off == -1)
+ if (off == (off_t) -1)
goto error;
rc = v2_read(id3, fd, &header);
if (rc)
@@ -1161,7 +1161,7 @@ int id3_read_tags(struct id3tag *id3, int fd, unsigned int flags)
}
if (flags & ID3_V1) {
off = lseek(fd, -128, SEEK_END);
- if (off == -1)
+ if (off == (off_t) -1)
goto error;
rc = read_all(fd, id3->v1, 128);
if (rc == -1)
diff --git a/modplug.c b/modplug.c
index bedeebf..9ee13cc 100644
--- a/modplug.c
+++ b/modplug.c
@@ -36,14 +36,15 @@ static int mod_open(struct input_plugin_data *ip_data)
{
struct mod_private *priv;
char *contents;
- int size, rc;
+ off_t size;
+ ssize_t rc;
ModPlugFile *file;
ModPlug_Settings settings;

size = lseek(ip_data->fd, 0, SEEK_END);
- if (size == -1)
+ if (size == (off_t) -1)
return -IP_ERROR_ERRNO;
- if (lseek(ip_data->fd, 0, SEEK_SET) == -1)
+ if (lseek(ip_data->fd, 0, SEEK_SET) == (off_t) -1)
return -IP_ERROR_ERRNO;

contents = xnew(char, size);
diff --git a/mpc.c b/mpc.c
index 507d116..103d85b 100644
--- a/mpc.c
+++ b/mpc.c
@@ -96,10 +96,8 @@ static mpc_int32_t read_impl(callback_t *data, void *ptr, mpc_int32_t size)
static mpc_bool_t seek_impl(callback_t *data, mpc_int32_t offset)
{
struct input_plugin_data *ip_data = get_ip_data(data);
- int rc;

- rc = lseek(ip_data->fd, offset, SEEK_SET);
- if (rc == -1)
+ if (lseek(ip_data->fd, offset, SEEK_SET) == (off_t) -1)
return MPC_FALSE;
return MPC_TRUE;
}
diff --git a/nomad.c b/nomad.c
index 46e238a..20443fd 100644
--- a/nomad.c
+++ b/nomad.c
@@ -499,7 +499,7 @@ start:
nomad->cur_frame++;
nomad->current.bitrate_sum += nomad->frame.header.bitrate;
nomad->current.nr_frames++;
- if (nomad->info.filesize > 0) {
+ if (nomad->info.filesize != (off_t) -1) {
build_seek_index(nomad);
} else {
mad_timer_add(&nomad->timer, nomad->frame.header.duration);
@@ -535,9 +535,9 @@ static int do_open(struct nomad *nomad)

init_mad(nomad);
nomad->info.filesize = nomad->cbs.lseek(nomad->datasource, 0, SEEK_END);
- if (nomad->info.filesize != -1)
+ if (nomad->info.filesize != (off_t) -1)
nomad->cbs.lseek(nomad->datasource, 0, SEEK_SET);
- if (nomad->info.filesize == -1) {
+ if (nomad->info.filesize == (off_t) -1) {
rc = decode(nomad);
if (rc < 0)
goto error;
@@ -725,7 +725,7 @@ static int nomad_time_seek_accurate(struct nomad *nomad, double pos)
int rc;

/* seek to beginning of file and search frame-by-frame */
- if (nomad->cbs.lseek(nomad->datasource, 0, SEEK_SET) < 0)
+ if (nomad->cbs.lseek(nomad->datasource, 0, SEEK_SET) == (off_t) -1)
return -1;

/* XING header should NOT be counted - if we're here, we know it's present */
@@ -766,7 +766,7 @@ int nomad_time_seek(struct nomad *nomad, double pos)
errno = EINVAL;
return -1;
}
- if (nomad->info.filesize == -1) {
+ if (nomad->info.filesize == (off_t) -1) {
errno = ESPIPE;
return -1;
}
@@ -805,7 +805,7 @@ int nomad_time_seek(struct nomad *nomad, double pos)
nomad->timer = nomad->seek_idx.table[idx].timer;
}
}
- if (nomad->cbs.lseek(nomad->datasource, offset, SEEK_SET) < 0)
+ if (nomad->cbs.lseek(nomad->datasource, offset, SEEK_SET) == (off_t) -1)
return -1;

nomad->input_offset = offset;
diff --git a/nomad.h b/nomad.h
index add2537..06cd475 100644
--- a/nomad.h
+++ b/nomad.h
@@ -70,7 +70,7 @@ struct nomad_info {
/* guessed */
int avg_bitrate;
/* -1 if file not seekable */
- int filesize;
+ off_t filesize;
unsigned int joint_stereo : 1;
unsigned int dual_channel : 1;
};
diff --git a/vorbis.c b/vorbis.c
index d3089a1..a6377d9 100644
--- a/vorbis.c
+++ b/vorbis.c
@@ -62,10 +62,8 @@ static size_t read_func(void *ptr, size_t size, size_t nmemb, void *datasource)
static int seek_func(void *datasource, ogg_int64_t offset, int whence)
{
struct input_plugin_data *ip_data = datasource;
- int rc;

- rc = lseek(ip_data->fd, offset, whence);
- if (rc == -1)
+ if (lseek(ip_data->fd, offset, whence) == (off_t) -1)
return -1;
return 0;
}
@@ -83,10 +81,12 @@ static int close_func(void *datasource)
static long tell_func(void *datasource)
{
struct input_plugin_data *ip_data = datasource;
- int rc;
+ off_t off;

- rc = lseek(ip_data->fd, 0, SEEK_CUR);
- return rc;
+ off = lseek(ip_data->fd, 0, SEEK_CUR);
+ if (off == (off_t) -1)
+ return -1;
+ return off;
}

/*
diff --git a/wav.c b/wav.c
index 1e29945..e0d3a96 100644
--- a/wav.c
+++ b/wav.c
@@ -84,7 +84,7 @@ static int find_chunk(int fd, const char *name, unsigned int *size)
if (rc != -IP_ERROR_FILE_FORMAT)
return rc;
d_print("seeking %d\n", *size);
- if (lseek(fd, *size, SEEK_CUR) == -1) {
+ if (lseek(fd, *size, SEEK_CUR) == (off_t) -1) {
d_print("seek failed\n");
return -IP_ERROR_ERRNO;
}
@@ -194,8 +194,7 @@ static int wav_open(struct input_plugin_data *ip_data)
rc = find_chunk(ip_data->fd, "data", &priv->pcm_size);
if (rc)
goto error_exit;
- rc = lseek(ip_data->fd, 0, SEEK_CUR);
- if (rc == -1) {
+ if (lseek(ip_data->fd, 0, SEEK_CUR) == (off_t) -1) {
rc = -IP_ERROR_ERRNO;
goto error_exit;
}
@@ -261,14 +260,12 @@ static int wav_seek(struct input_plugin_data *ip_data, double _offset)
{
struct wav_private *priv = ip_data->private;
unsigned int offset;
- off_t rc;

offset = (unsigned int)(_offset * (double)priv->sec_size + 0.5);
/* align to frame size */
offset -= offset % priv->frame_size;
priv->pos = offset;
- rc = lseek(ip_data->fd, priv->pcm_start + offset, SEEK_SET);
- if (rc == (off_t)-1)
+ if (lseek(ip_data->fd, priv->pcm_start + offset, SEEK_SET) == (off_t) -1)
return -1;
return 0;
}
@@ -311,9 +308,10 @@ static int wav_read_comments(struct input_plugin_data *ip_data,
priv = ip_data->private;
id[4] = '\0';

- rc = lseek(ip_data->fd, 12, SEEK_SET);
- if (rc == -1)
+ if (lseek(ip_data->fd, 12, SEEK_SET) == (off_t) -1) {
+ rc = -1;
goto out;
+ }

while (1) {
rc = read_chunk_header(ip_data->fd, id, &size);
@@ -345,9 +343,10 @@ static int wav_read_comments(struct input_plugin_data *ip_data,
}
}

- rc = lseek(ip_data->fd, size, SEEK_CUR);
- if (rc == -1)
+ if (lseek(ip_data->fd, size, SEEK_CUR) == (off_t) -1) {
+ rc = -1;
break;
+ }
}

out:
diff --git a/wavpack.c b/wavpack.c
index fda0363..62421dc 100644
--- a/wavpack.c
+++ b/wavpack.c
@@ -82,17 +82,19 @@ static int32_t read_bytes(void *data, void *ptr, int32_t count)
static uint32_t get_pos(void *data)
{
struct wavpack_file *file = data;
+ off_t off;

- return lseek(file->fd, 0, SEEK_CUR);
+ off = lseek(file->fd, 0, SEEK_CUR);
+ if (off == (off_t) -1)
+ return -1;
+ return off;
}

static int set_pos_rel(void *data, int32_t delta, int mode)
{
struct wavpack_file *file = data;
- int rc;

- rc = lseek(file->fd, delta, mode);
- if (rc == -1)
+ if (lseek(file->fd, delta, mode) == (off_t) -1)
return -1;

file->push_back_byte = EOF;

--
1.7.5.1

Gregory Petrosyan

2011-05-25 20:17:40 UTC

Permalink

Hm, is this really needed? As I see this, it does nothing, since
compiler casts both parts of comparison to larger type anyways, which
is off_t in this case, no? And there's nothing wrong with comparing
int with long or long long.

Gregory

Johannes Weißl

2011-05-25 20:46:11 UTC

Permalink

Post by Gregory Petrosyan

- Â Â Â if (file_size == -1)
+ Â Â Â if (file_size == (off_t) -1)

Yes, it seems you are right... I read the sections in the C99 standard
(6.3.1.8, 6.3.1.3 and 6.2.5). So we have two alternatives:
- consistently use an explicit cast, for documentary reasons
(the manpage for lseek also uses (off_t) -1, and so does lots of other
code)
- consistently remove *all* (xxx) -1 casts, so nobody thinks the
missing casts are bugs (and because consistency is good)

what do you suggest?

Johannes

Gregory Petrosyan

2011-05-25 21:14:17 UTC

Permalink

Post by Johannes WeiÃl

Post by Gregory Petrosyan

- if (file_size == -1)
+ if (file_size == (off_t) -1)

Yes, it seems you are right... I read the sections in the C99 standard
- consistently use an explicit cast, for documentary reasons
(the manpage for lseek also uses (off_t) -1, and so does lots of other
code)
- consistently remove *all* (xxx) -1 casts, so nobody thinks the
missing casts are bugs (and because consistency is good)
what do you suggest?

I personally prefer the option 2), since I see no documentation value
in this cast — it reminds us that lseek() returns off_t? So what? My
position is that if there is a cast in the code, it should be
justified by something.

BTW, OS X man page does not have the cast :-)

Gregory

Johannes Weißl

2011-05-25 22:03:11 UTC

Permalink

Post by Gregory Petrosyan
I personally prefer the option 2), since I see no documentation value
in this cast â it reminds us that lseek() returns off_t? So what? My
position is that if there is a cast in the code, it should be
justified by something.
BTW, OS X man page does not have the cast :-)

Ok, I updated my patch (attached). If I interpret the standard
correctly, int x = lseek() is undefined, so this is replaced. Also all
remaining (off_t)-1 casts are removed, and some minor cleanups!

Johannes

Gregory Petrosyan

2011-05-25 22:31:12 UTC

Permalink

Post by Johannes WeiÃl

Post by Gregory Petrosyan
I personally prefer the option 2), since I see no documentation value
in this cast — it reminds us that lseek() returns off_t? So what? My
position is that if there is a cast in the code, it should be
justified by something.
BTW, OS X man page does not have the cast :-)

Ok, I updated my patch (attached). If I interpret the standard
correctly, int x = lseek() is undefined, so this is replaced. Also all
remaining (off_t)-1 casts are removed, and some minor cleanups!

AFAIK it is undefined (implementation-defined) only when down-casting signed
values that can not be represented in the target signed type (6.3.1.3), so this
is only the case when lseek returns more than can fit into int.

Post by Johannes WeiÃl
static uint32_t get_pos(void *data)
{
struct wavpack_file *file = data;
+ off_t off;
- return lseek(file->fd, 0, SEEK_CUR);
+ off = lseek(file->fd, 0, SEEK_CUR);
+ return (off == -1) ? -1 : off;

Since the function is returning unsigned value, and off_t -> uint conversion is
defined, there is no need for this change I think.

Post by Johannes WeiÃl
- rc = lseek(ip_data->fd, size, SEEK_CUR);
- if (rc == -1)
+ if (lseek(ip_data->fd, size, SEEK_CUR) == -1) {
+ rc = -1;
break;
+ }

I can come up with a theoretical corner case when this change makes sense, but
it is not easy :-)

Other changes look good!

Gregory

Johannes Weißl

2011-05-25 22:47:53 UTC

Permalink

Post by Gregory Petrosyan

Post by Johannes WeiÃl
Ok, I updated my patch (attached). If I interpret the standard
correctly, int x = lseek() is undefined, so this is replaced. Also all
remaining (off_t)-1 casts are removed, and some minor cleanups!

Hmm, so it is theoretically backed up :-)!

Post by Gregory Petrosyan

Since the function is returning unsigned value, and off_t -> uint conversion is
defined, there is no need for this change I think.

You are right, changed it back!

Post by Gregory Petrosyan

Post by Johannes WeiÃl
- rc = lseek(ip_data->fd, size, SEEK_CUR);
- if (rc == -1)
+ if (lseek(ip_data->fd, size, SEEK_CUR) == -1) {
+ rc = -1;
break;
+ }

I can come up with a theoretical corner case when this change makes sense, but
it is not easy :-)

Ok, since we are far away from any practical issues, I guess this
justifies the change :-).

Updated patch is attached!

Johannes