unconditional use of st_blocks and st_blksize in .../source3/client/client.c, ./source3/libsmb/clifile.c, ./source3/modules/vfs_streams_xattr.c and .../source3/lib/system.c. Patch: diff -u ./source3/client/client.c.orig ./source3/client/client.c --- ./source3/client/client.c.orig 2010-05-17 06:51:23.000000000 -0500 +++ ./source3/client/client.c 2010-05-26 08:41:43.000000000 -0500 @@ -3185,10 +3185,16 @@ /* Print out the stat values. */ d_printf("File: %s\n", src); +#ifdef HAVE_STAT_ST_BLOCKS d_printf("Size: %-12.0f\tBlocks: %u\t%s\n", (double)sbuf.st_ex_size, (unsigned int)sbuf.st_ex_blocks, filetype_to_str(sbuf.st_ex_mode)); +#else + d_printf("Size: %-12.0f\t%s\n", + (double)sbuf.st_ex_size, + filetype_to_str(sbuf.st_ex_mode)); +#endif #if defined(S_ISCHR) && defined(S_ISBLK) if (S_ISCHR(sbuf.st_ex_mode) || S_ISBLK(sbuf.st_ex_mode)) { ./source3/client/mount.cifs.c missing diff -u ./source3/libsmb/clifile.c.orig ./source3/libsmb/clifile.c --- ./source3/libsmb/clifile.c.orig 2010-05-17 06:51:23.000000000 -0500 +++ ./source3/libsmb/clifile.c 2010-05-26 08:56:24.000000000 -0500 @@ -867,12 +867,14 @@ } sbuf->st_ex_size = IVAL2_TO_SMB_BIG_UINT(state->data,0); /* total size, in bytes */ +#if defined (HAVE_STAT_ST_BLOCKS) sbuf->st_ex_blocks = IVAL2_TO_SMB_BIG_UINT(state->data,8); /* number of blocks allocated */ -#if defined (HAVE_STAT_ST_BLOCKS) && defined(STAT_ST_BLOCKSIZE) +# if defined defined(STAT_ST_BLOCKSIZE) sbuf->st_ex_blocks /= STAT_ST_BLOCKSIZE; -#else +# else /* assume 512 byte blocks */ sbuf->st_ex_blocks /= 512; +# endif #endif sbuf->st_ex_ctime = interpret_long_date((char *)(state->data + 16)); /* time of last change */ sbuf->st_ex_atime = interpret_long_date((char *)(state->data + 24)); /* time of last access */ diff -u ./source3/modules/vfs_streams_xattr.c.orig ./source3/modules/vfs_streams_xattr.c --- ./source3/modules/vfs_streams_xattr.c.orig 2010-05-17 06:51:23.000000000 -0500 +++ ./source3/modules/vfs_streams_xattr.c 2010-05-26 06:46:57.000000000 -0500 @@ -237,7 +237,9 @@ sbuf->st_ex_ino = stream_inode(sbuf, io->xattr_name); sbuf->st_ex_mode &= ~S_IFMT; sbuf->st_ex_mode |= S_IFREG; +#ifdef HAVE_STAT_ST_BLOCKS sbuf->st_ex_blocks = sbuf->st_ex_size % STAT_ST_BLOCKSIZE + 1; +#endif return 0; } @@ -289,8 +291,10 @@ smb_fname->st.st_ex_ino = stream_inode(&smb_fname->st, xattr_name); smb_fname->st.st_ex_mode &= ~S_IFMT; smb_fname->st.st_ex_mode |= S_IFREG; +#ifdef HAVE_STAT_ST_BLOCKS smb_fname->st.st_ex_blocks = smb_fname->st.st_ex_size % STAT_ST_BLOCKSIZE + 1; +#endif result = 0; fail: @@ -340,8 +344,10 @@ smb_fname->st.st_ex_ino = stream_inode(&smb_fname->st, xattr_name); smb_fname->st.st_ex_mode &= ~S_IFMT; smb_fname->st.st_ex_mode |= S_IFREG; +#ifdef HAVE_STAT_ST_BLOCKS smb_fname->st.st_ex_blocks = smb_fname->st.st_ex_size % STAT_ST_BLOCKSIZE + 1; +#endif result = 0; diff -u ./source3/lib/system.c.orig ./source3/lib/system.c --- ./source3/lib/system.c.orig 2010-05-17 06:51:23.000000000 -0500 +++ ./source3/lib/system.c 2010-05-27 07:13:48.000000000 -0500 @@ -534,8 +534,12 @@ dst->st_ex_mtime = get_mtimespec(src); dst->st_ex_ctime = get_ctimespec(src); make_create_timespec(src, dst, fake_dir_create_times); +#ifdef HAVE_STAT_ST_BLKSIZE dst->st_ex_blksize = src->st_blksize; +#endif +#ifdef HAVE_STAT_ST_BLOCKS dst->st_ex_blocks = src->st_blocks; +#endif #ifdef HAVE_STAT_ST_FLAGS dst->st_ex_flags = src->st_flags; Similar to the very old and long closed bug #550 Bye, Jojo
for future patches - can you please attach them to the bug report instead of pasting inline?
(In reply to comment #1) > for future patches - can you please attach them to the bug report instead of > pasting inline? I found some more spots where st_blocks and st_blksize is used. I will created attach a patch later. bye, Jojo
here we go, properly #ifdef'ing them in ...source3/includes/includes.h reveals it's unconditional use in .../source3/smbd/smb2_create.c Patch (maybe not the most clever one): diff -u ./source3/include/includes.h.orig ./source3/include/includes.h --- ./source3/include/includes.h.orig 2010-05-17 06:51:23.000000000 -0500 +++ ./source3/include/includes.h 2010-05-28 03:46:26.000000000 -0500 @@ -442,8 +459,12 @@ struct timespec st_ex_btime; /* birthtime */ /* Is birthtime real, or was it calculated ? */ bool st_ex_calculated_birthtime; +#ifdef HAVE_ST_BLKSIZE blksize_t st_ex_blksize; +#endif +#ifdef HAVE_ST_BLOCKS blkcnt_t st_ex_blocks; +#endif uint32_t st_ex_flags; uint32_t st_ex_mask; diff -u ./source3/smbd/smb2_create.c.orig ./source3/smbd/smb2_create.c --- ./source3/smbd/smb2_create.c.orig 2010-05-17 06:51:23.000000000 -0500 +++ ./source3/smbd/smb2_create.c 2010-05-28 04:06:22.000000000 -0500 @@ -699,8 +699,16 @@ get_change_timespec(smbreq->conn, result, result->fsp_name)); state->out_allocation_size = +#ifdef HAVE_ST_BLKSIZE result->fsp_name->st.st_ex_blksize * +#else + STAT_ST_BLOCKSIZE * +#endif +#ifdef HAVE_ST_BLOCKS result->fsp_name->st.st_ex_blocks; +#else + result->fsp_name->st.st_ex_size % STAT_ST_BLOCKSIZE + 1; +#endif state->out_end_of_file = result->fsp_name->st.st_ex_size; if (state->out_file_attributes == 0) { state->out_file_attributes = FILE_ATTRIBUTE_NORMAL; Full patch will be attached shortly... Bye, Jojo
Created attachment 5745 [details] Patch
jojo, I think you mistyped HAVE_STAT_ST_BLKSIZE with HAVE_ST_BLKSIZE in the patch? The intention of the st_ex struct is to eliminate as many #ifdef's as possible. Maybe something like the following patch also works for you?
Created attachment 5761 [details] untested alternative patch
(In reply to comment #5) > jojo, I think you mistyped HAVE_STAT_ST_BLKSIZE with HAVE_ST_BLKSIZE in the > patch? Yes, I did, sorry > The intention of the st_ex struct is to eliminate as many #ifdef's as possible. > Maybe something like the following patch also works for you? I'll give it a try, but it should work. Looks much leaner and I like it better then my version. Bye, Jojo Bye, Jojo
It compiles cleanly with your patch. Bye, Joo
thanks! It's fixed in master with 711a30aa61bb5f6a9b3970007bad8a70f411fb87. Volker, can you please review that commit for cherry-pick to 3.5 and then reassign to Karorin?
Is it realy '%' to be used here rather than '/'? when calculating the number of blocks using size and blocksize I think it is: dst->st_ex_blocks = src->st_size / STAT_ST_BLOCKSIZE + 1; With the "+ 1" to cater for the remainder. I had stolen that from .../source3/modules/vfs_streams_xattr.c, but now believe it is wrong there too. Also we should be using src->st_blksize, shouldn't we? dst->st_ex_blocks = src->st_size / src->st_blksize + 1;
Nonsense, I meant dst->st_ex_blocks = src->st_size / dst->st_ex_blksize + 1;
Good point, looks wrong to me too. That code has been inheritted since a while, initially from Darwin's samba stream module :-) The "+ 1" should actually also just be applied if "dst->st_ex_size % dst->st_ex_blksize > 0".
(In reply to comment #12) > The "+ 1" should actually also just be applied if > "dst->st_ex_size % dst->st_ex_blksize > 0". True, but this is a corner case and only valid for systems that don't HAVE_STAT_ST_BLOCKS, so that possible little error wouldn't be to bad and that extra calculation not worth the efford, would it? And better allocating a bit too much rather than too little. in .../source3/modules/vfs_streams_xattr.c (in 3 places) we should get it right though and I have the feeling that .../source3/libsmb/clifile.c needs fixing too, as it totaly ignores the possible remainder when calculating the number of blocks allocated (line ~871). Bye, Jojo
Created attachment 5775 [details] new patch for 3.5 Volker, can you please review and reassign to Karolin?
Comment on attachment 5775 [details] new patch for 3.5 I'm not sure if this is always correct: dst->st_ex_blocks = src->st_size / dst->st_ex_blksize + 1; I mean the '+1' should only be there if st_size isn't a multiple of st_ex_blksize. metze
Comment on attachment 5775 [details] new patch for 3.5 Ok, I read the other comments in the bug... as it's just a corner case.
(In reply to comment #16) > (From update of attachment 5775 [details]) > Ok, I read the other comments in the bug... > as it's just a corner case. > Metze, please clarify if I should pick the patch or not. Shall I wait for Volker's review also? Thanks!
Metze's ack is sufficiant :-)
Pushed to v3-5-test. Closing out bug report. Thanks!
I think we still have a potential problem here, see patch: diff -u ./source3/lib/system.c.orig ./source3/lib/system.c --- ./source3/lib/system.c.orig 2010-10-07 11:41:16.000000000 -0500 +++ ./source3/lib/system.c 2010-10-12 12:15:39.000000000 -0500 @@ -537,7 +537,11 @@ #ifdef HAVE_STAT_ST_BLKSIZE dst->st_ex_blksize = src->st_blksize; #else +# ifdef STAT_ST_BLOCKSIZE dst->st_ex_blksize = STAT_ST_BLOCKSIZE; +# else + dst->st_ex_blksize = 512; /* we have to assume something... */ +# endif #endif #ifdef HAVE_STAT_ST_BLOCKS Bye, Jojo
not required. STAT_ST_BLOCKSIZE has to be defined (see AC_DEFINE(...) in configure.in) - otherwise the code would not compile - which would require a fix in configure.in then.
Thanks for clarification