Hi Pete,
> > - char *link_target; /* NOTE: caller must free if valid */
> > - int dfile_count;
> > + uint64_t link_target; /* NOTE: caller must free if valid. Needs to be
> > uint64_t so that it is a fixed length entity */
> > + int32_t dfile_count;
>
> This is by far the most objectionable component, along with all the
> string casts. Could you use #ifdef on word size to avoid this?
Yes, indeed. Okay. I will change that then...
>
> > struct PVFS_sys_mntent
> > {
> > char **pvfs_config_servers; /* addresses of servers with config
> > info */
> > - int num_pvfs_config_servers;
> > + int32_t num_pvfs_config_servers;
>
> Are you saying int is 64-bit on ppc64? That would be very odd indeed.
> I find the need to encode the type for native ints quite intrusive. Is
> this really the way these things are handled?
Well, LP64 is generally the preferred data model but I am not sure if
ILP64 is followed by any architecture (although that would break tons of
applications, I am sure). All I wanted was to fix the sizes
of structures so that they would be the same regardless of 32/64 bit
issues. If you have any suggestions, do let me know or I will let them
stay the way they are.
> > + tmp_cflags=$CFLAGS
> > + CFLAGS="$CFLAGS"
>
> What good does this do?
No clue... :) I just cut and pasted from the previous lines (obviously
incorrectly though)!.
I will fix those...
>
> > - size_t amt_complete;
> > + int64_t amt_complete;
>
> size_t is unsigned, int64_t is signed.
Hmm.. If you look at pvfs2-client-core, which fills in these values after
the I/O operation is done from the response, all those types are PVFS_size
which is typedef'ed to int64_t. Hence I did the same.
Should this be changed to uint64_t when PVFS_size itself is int64_t?
>
> > - loff_t readahead_size;
> > + int32_t readahead_size;
> > } pvfs2_io_request_t;
>
>
> Lost precision here, 64->32. Do possible values still fit?
This piece of code was already pretty loose about precision prior to the
change. If you look at pvfs2-client-core, it constructs
PVFS_Request_contiguous types before the readahead, and then there is a
cast to an int32_t since PVFS_Request_contiguous requires an int32_t.
So I dont know whether it is worth fixing those.. It seemed easier to fix
this.
That said, I dunno if we would ever request more than 2 GB of read-ahead!
This code is triggered only for executables/mmap(MAP_PRIVATE, RDONLY) as
it stands
> > + {
> > + int ret = 0;
> > ret = copy_from_user(
>
> No need to initialize to zero if you're immediatly giving it a value
> (three places).
Okay :)
Will change those as well!
>
> In spite of the little comments above, I think this is important to
> do and good stuff. But testing on all-64 and all-32 arches has to
> happen to watch out for conversion bugs.
Thanks a lot for the comments and suggestions, Pete!
Regards
Murali
_______________________________________________
Pvfs2-developers mailing list
Pvfs2-developers@beowulf-underground.org
http://www.beowulf-underground.org/mailman/listinfo/pvfs2-developers
