| Message ID | 1294224062-18745-1-git-send-email-Jes.Sorensen@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Jan 05, 2011 at 11:41:02AM +0100, Jes.Sorensen@redhat.com wrote: > From: Jes Sorensen <Jes.Sorensen@redhat.com> > > strtosz() needs to return a 64 bit type even on 32 bit > architectures. Otherwise qemu-img will fail to create disk > images >= 2GB > > Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com> Nothing wrong with this patch, but should the function be renamed to strtos64 then? > --- > cutils.c | 8 ++++---- > monitor.c | 2 +- > qemu-common.h | 4 ++-- > qemu-img.c | 2 +- > vl.c | 4 ++-- > 5 files changed, 10 insertions(+), 10 deletions(-) > > diff --git a/cutils.c b/cutils.c > index 7984bc1..4d2e27c 100644 > --- a/cutils.c > +++ b/cutils.c > @@ -291,9 +291,9 @@ int fcntl_setfl(int fd, int flag) > * value must be terminated by whitespace, ',' or '\0'. Return -1 on > * error. > */ > -ssize_t strtosz_suffix(const char *nptr, char **end, const char default_suffix) > +int64_t strtosz_suffix(const char *nptr, char **end, const char default_suffix) > { > - ssize_t retval = -1; > + int64_t retval = -1; > char *endptr, c, d; > int mul_required = 0; > double val, mul, integral, fraction; > @@ -365,7 +365,7 @@ ssize_t strtosz_suffix(const char *nptr, char **end, const char default_suffix) > goto fail; > } > } > - if ((val * mul >= ~(size_t)0) || val < 0) { > + if ((val * mul >= INT64_MAX) || val < 0) { > goto fail; > } > retval = val * mul; > @@ -378,7 +378,7 @@ fail: > return retval; > } > > -ssize_t strtosz(const char *nptr, char **end) > +int64_t strtosz(const char *nptr, char **end) > { > return strtosz_suffix(nptr, end, STRTOSZ_DEFSUFFIX_MB); > } > diff --git a/monitor.c b/monitor.c > index f258000..fcdae15 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -4162,7 +4162,7 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon, > break; > case 'o': > { > - ssize_t val; > + int64_t val; > char *end; > > while (qemu_isspace(*p)) { > diff --git a/qemu-common.h b/qemu-common.h > index 63d9943..cce6e61 100644 > --- a/qemu-common.h > +++ b/qemu-common.h > @@ -158,8 +158,8 @@ int fcntl_setfl(int fd, int flag); > #define STRTOSZ_DEFSUFFIX_MB 'M' > #define STRTOSZ_DEFSUFFIX_KB 'K' > #define STRTOSZ_DEFSUFFIX_B 'B' > -ssize_t strtosz(const char *nptr, char **end); > -ssize_t strtosz_suffix(const char *nptr, char **end, const char default_suffix); > +int64_t strtosz(const char *nptr, char **end); > +int64_t strtosz_suffix(const char *nptr, char **end, const char default_suffix); > > /* path.c */ > void init_paths(const char *prefix); > diff --git a/qemu-img.c b/qemu-img.c > index afd9ed2..6af2a4c 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -320,7 +320,7 @@ static int img_create(int argc, char **argv) > > /* Get image size, if specified */ > if (optind < argc) { > - ssize_t sval; > + int64_t sval; > sval = strtosz_suffix(argv[optind++], NULL, STRTOSZ_DEFSUFFIX_B); > if (sval < 0) { > error_report("Invalid image size specified! You may use k, M, G or " > diff --git a/vl.c b/vl.c > index 78fcef1..93425f4 100644 > --- a/vl.c > +++ b/vl.c > @@ -804,7 +804,7 @@ static void numa_add(const char *optarg) > if (get_param_value(option, 128, "mem", optarg) == 0) { > node_mem[nodenr] = 0; > } else { > - ssize_t sval; > + int64_t sval; > sval = strtosz(option, NULL); > if (sval < 0) { > fprintf(stderr, "qemu: invalid numa mem size: %s\n", optarg); > @@ -2245,7 +2245,7 @@ int main(int argc, char **argv, char **envp) > exit(0); > break; > case QEMU_OPTION_m: { > - ssize_t value; > + int64_t value; > > value = strtosz(optarg, NULL); > if (value < 0) { > -- > 1.7.3.4 >
On 01/05/11 13:34, Michael S. Tsirkin wrote: > On Wed, Jan 05, 2011 at 11:41:02AM +0100, Jes.Sorensen@redhat.com wrote: >> From: Jes Sorensen <Jes.Sorensen@redhat.com> >> >> strtosz() needs to return a 64 bit type even on 32 bit >> architectures. Otherwise qemu-img will fail to create disk >> images >= 2GB >> >> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com> > > Nothing wrong with this patch, but should the function > be renamed to strtos64 then? I don't think that adds any value to be honest. The problem with the old interface was that the return type differed depending on whether it was compiled on 32 vs 64 bit systems. Cheers, Jes
On Wed, Jan 05, 2011 at 01:36:35PM +0100, Jes Sorensen wrote: > On 01/05/11 13:34, Michael S. Tsirkin wrote: > > On Wed, Jan 05, 2011 at 11:41:02AM +0100, Jes.Sorensen@redhat.com wrote: > >> From: Jes Sorensen <Jes.Sorensen@redhat.com> > >> > >> strtosz() needs to return a 64 bit type even on 32 bit > >> architectures. Otherwise qemu-img will fail to create disk > >> images >= 2GB > >> > >> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com> > > > > Nothing wrong with this patch, but should the function > > be renamed to strtos64 then? > > I don't think that adds any value to be honest. The problem with the old > interface was that the return type differed depending on whether it was > compiled on 32 vs 64 bit systems. > > Cheers, > Jes Yes, but does not the name strtosz imply 'string to size'? Further changing the name will help make sure there are no users you forgot to update.
On 01/05/2011 04:41 AM, Jes.Sorensen@redhat.com wrote: > From: Jes Sorensen<Jes.Sorensen@redhat.com> > > strtosz() needs to return a 64 bit type even on 32 bit > architectures. Otherwise qemu-img will fail to create disk > images>= 2GB > > Signed-off-by: Jes Sorensen<Jes.Sorensen@redhat.com> > off_t would be the proper type to use. Regards, Anthony Liguori > --- > cutils.c | 8 ++++---- > monitor.c | 2 +- > qemu-common.h | 4 ++-- > qemu-img.c | 2 +- > vl.c | 4 ++-- > 5 files changed, 10 insertions(+), 10 deletions(-) > > diff --git a/cutils.c b/cutils.c > index 7984bc1..4d2e27c 100644 > --- a/cutils.c > +++ b/cutils.c > @@ -291,9 +291,9 @@ int fcntl_setfl(int fd, int flag) > * value must be terminated by whitespace, ',' or '\0'. Return -1 on > * error. > */ > -ssize_t strtosz_suffix(const char *nptr, char **end, const char default_suffix) > +int64_t strtosz_suffix(const char *nptr, char **end, const char default_suffix) > { > - ssize_t retval = -1; > + int64_t retval = -1; > char *endptr, c, d; > int mul_required = 0; > double val, mul, integral, fraction; > @@ -365,7 +365,7 @@ ssize_t strtosz_suffix(const char *nptr, char **end, const char default_suffix) > goto fail; > } > } > - if ((val * mul>= ~(size_t)0) || val< 0) { > + if ((val * mul>= INT64_MAX) || val< 0) { > goto fail; > } > retval = val * mul; > @@ -378,7 +378,7 @@ fail: > return retval; > } > > -ssize_t strtosz(const char *nptr, char **end) > +int64_t strtosz(const char *nptr, char **end) > { > return strtosz_suffix(nptr, end, STRTOSZ_DEFSUFFIX_MB); > } > diff --git a/monitor.c b/monitor.c > index f258000..fcdae15 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -4162,7 +4162,7 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon, > break; > case 'o': > { > - ssize_t val; > + int64_t val; > char *end; > > while (qemu_isspace(*p)) { > diff --git a/qemu-common.h b/qemu-common.h > index 63d9943..cce6e61 100644 > --- a/qemu-common.h > +++ b/qemu-common.h > @@ -158,8 +158,8 @@ int fcntl_setfl(int fd, int flag); > #define STRTOSZ_DEFSUFFIX_MB 'M' > #define STRTOSZ_DEFSUFFIX_KB 'K' > #define STRTOSZ_DEFSUFFIX_B 'B' > -ssize_t strtosz(const char *nptr, char **end); > -ssize_t strtosz_suffix(const char *nptr, char **end, const char default_suffix); > +int64_t strtosz(const char *nptr, char **end); > +int64_t strtosz_suffix(const char *nptr, char **end, const char default_suffix); > > /* path.c */ > void init_paths(const char *prefix); > diff --git a/qemu-img.c b/qemu-img.c > index afd9ed2..6af2a4c 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -320,7 +320,7 @@ static int img_create(int argc, char **argv) > > /* Get image size, if specified */ > if (optind< argc) { > - ssize_t sval; > + int64_t sval; > sval = strtosz_suffix(argv[optind++], NULL, STRTOSZ_DEFSUFFIX_B); > if (sval< 0) { > error_report("Invalid image size specified! You may use k, M, G or " > diff --git a/vl.c b/vl.c > index 78fcef1..93425f4 100644 > --- a/vl.c > +++ b/vl.c > @@ -804,7 +804,7 @@ static void numa_add(const char *optarg) > if (get_param_value(option, 128, "mem", optarg) == 0) { > node_mem[nodenr] = 0; > } else { > - ssize_t sval; > + int64_t sval; > sval = strtosz(option, NULL); > if (sval< 0) { > fprintf(stderr, "qemu: invalid numa mem size: %s\n", optarg); > @@ -2245,7 +2245,7 @@ int main(int argc, char **argv, char **envp) > exit(0); > break; > case QEMU_OPTION_m: { > - ssize_t value; > + int64_t value; > > value = strtosz(optarg, NULL); > if (value< 0) { >
On 01/05/11 14:46, Anthony Liguori wrote: > On 01/05/2011 04:41 AM, Jes.Sorensen@redhat.com wrote: >> From: Jes Sorensen<Jes.Sorensen@redhat.com> >> >> strtosz() needs to return a 64 bit type even on 32 bit >> architectures. Otherwise qemu-img will fail to create disk >> images>= 2GB >> >> Signed-off-by: Jes Sorensen<Jes.Sorensen@redhat.com> >> > > off_t would be the proper type to use. I discussed this with Markus, and we both agree that it isn't. Two reasons, off_t is for filesystem offsets, not random sizes. Second, off_t doesn't have an unsigned type or a max to compare against. Using int64_t is cleaner and safer. Cheers, Jes
On 01/05/2011 08:40 AM, Jes Sorensen wrote: > On 01/05/11 14:46, Anthony Liguori wrote: > >> On 01/05/2011 04:41 AM, Jes.Sorensen@redhat.com wrote: >> >>> From: Jes Sorensen<Jes.Sorensen@redhat.com> >>> >>> strtosz() needs to return a 64 bit type even on 32 bit >>> architectures. Otherwise qemu-img will fail to create disk >>> images>= 2GB >>> >>> Signed-off-by: Jes Sorensen<Jes.Sorensen@redhat.com> >>> >>> >> off_t would be the proper type to use. >> > I discussed this with Markus, and we both agree that it isn't. > > Two reasons, off_t is for filesystem offsets, not random sizes. Second, > off_t doesn't have an unsigned type or a max to compare against. > That's because the size of off_t depends on whether it's a 32 or 64-bit platform and what FILESIZEBITS is defined to be. Basically, if you're looking for the type to represent offsets in a file, it's off_t. That's why it exists. That said, using this to represent memory too, I can buy that as a justification to use int64_t. > Using int64_t is cleaner and safer. > I wouldn't make such bold claims but I'll concede that one is not significantly better than the other and won't object to int64_t if you feel strongly. Regards, Anthony Liguori > Cheers, > Jes >
On 01/05/11 19:29, Anthony Liguori wrote: > I wouldn't make such bold claims but I'll concede that one is not > significantly better than the other and won't object to int64_t if you > feel strongly. The more I think of it, the more I come to the conclusion that int64_t is the best solution. Since we can in theory have a system where we only allow 32 bit file system offsets, but have > 4GB of memory, int64_t does it best - it is more generic. Cheers, Jes
Am 05.01.2011 11:41, schrieb Jes.Sorensen@redhat.com: > From: Jes Sorensen <Jes.Sorensen@redhat.com> > > strtosz() needs to return a 64 bit type even on 32 bit > architectures. Otherwise qemu-img will fail to create disk > images >= 2GB > > Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com> Thanks, applied to the block branch. Kevin
diff --git a/cutils.c b/cutils.c index 7984bc1..4d2e27c 100644 --- a/cutils.c +++ b/cutils.c @@ -291,9 +291,9 @@ int fcntl_setfl(int fd, int flag) * value must be terminated by whitespace, ',' or '\0'. Return -1 on * error. */ -ssize_t strtosz_suffix(const char *nptr, char **end, const char default_suffix) +int64_t strtosz_suffix(const char *nptr, char **end, const char default_suffix) { - ssize_t retval = -1; + int64_t retval = -1; char *endptr, c, d; int mul_required = 0; double val, mul, integral, fraction; @@ -365,7 +365,7 @@ ssize_t strtosz_suffix(const char *nptr, char **end, const char default_suffix) goto fail; } } - if ((val * mul >= ~(size_t)0) || val < 0) { + if ((val * mul >= INT64_MAX) || val < 0) { goto fail; } retval = val * mul; @@ -378,7 +378,7 @@ fail: return retval; } -ssize_t strtosz(const char *nptr, char **end) +int64_t strtosz(const char *nptr, char **end) { return strtosz_suffix(nptr, end, STRTOSZ_DEFSUFFIX_MB); } diff --git a/monitor.c b/monitor.c index f258000..fcdae15 100644 --- a/monitor.c +++ b/monitor.c @@ -4162,7 +4162,7 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon, break; case 'o': { - ssize_t val; + int64_t val; char *end; while (qemu_isspace(*p)) { diff --git a/qemu-common.h b/qemu-common.h index 63d9943..cce6e61 100644 --- a/qemu-common.h +++ b/qemu-common.h @@ -158,8 +158,8 @@ int fcntl_setfl(int fd, int flag); #define STRTOSZ_DEFSUFFIX_MB 'M' #define STRTOSZ_DEFSUFFIX_KB 'K' #define STRTOSZ_DEFSUFFIX_B 'B' -ssize_t strtosz(const char *nptr, char **end); -ssize_t strtosz_suffix(const char *nptr, char **end, const char default_suffix); +int64_t strtosz(const char *nptr, char **end); +int64_t strtosz_suffix(const char *nptr, char **end, const char default_suffix); /* path.c */ void init_paths(const char *prefix); diff --git a/qemu-img.c b/qemu-img.c index afd9ed2..6af2a4c 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -320,7 +320,7 @@ static int img_create(int argc, char **argv) /* Get image size, if specified */ if (optind < argc) { - ssize_t sval; + int64_t sval; sval = strtosz_suffix(argv[optind++], NULL, STRTOSZ_DEFSUFFIX_B); if (sval < 0) { error_report("Invalid image size specified! You may use k, M, G or " diff --git a/vl.c b/vl.c index 78fcef1..93425f4 100644 --- a/vl.c +++ b/vl.c @@ -804,7 +804,7 @@ static void numa_add(const char *optarg) if (get_param_value(option, 128, "mem", optarg) == 0) { node_mem[nodenr] = 0; } else { - ssize_t sval; + int64_t sval; sval = strtosz(option, NULL); if (sval < 0) { fprintf(stderr, "qemu: invalid numa mem size: %s\n", optarg); @@ -2245,7 +2245,7 @@ int main(int argc, char **argv, char **envp) exit(0); break; case QEMU_OPTION_m: { - ssize_t value; + int64_t value; value = strtosz(optarg, NULL); if (value < 0) {