| Message ID | 5443C8F8.2000003@ramsay1.demon.co.uk (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
On Sun, Oct 19, 2014 at 10:21 PM, Ramsay Jones <ramsay@ramsay1.demon.co.uk> wrote: > Sorry for the late reply, it's been a busy few days. ;-) > > On 16/10/14 02:45, Christopher Li wrote: >> Hi, >> >> I create a branch in the chrisl repo call "review-ramsay" for your new >> follow up patches. >> I plan to do incremental fix up if any on that branch. Then when we >> both happy about it, >> I will do rebase and smash the commit before push to master branch. > > Yes, I noticed the new branch - thanks! > > Did you have any thoughts regarding Josh Triplett's comments on the > "compile-i386.c: don't ignore return value of write(2)" patch? I am fine with either way. The current fix is fine. If you are up for a new xwrite function, that is of course fine as well. Using printf() is not much an improvement because printf() can still return bytes less than the amount you requested. > > No, no, you don't want to do that even on Linux (let alone more > exotic platforms like Windows, MinGW and cygwin). In that case, I will leave the patch as it is. Thanks for the explain. Chris -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 22/10/14 01:23, Christopher Li wrote: > On Sun, Oct 19, 2014 at 10:21 PM, Ramsay Jones > <ramsay@ramsay1.demon.co.uk> wrote: >> Sorry for the late reply, it's been a busy few days. ;-) >> >> On 16/10/14 02:45, Christopher Li wrote: >>> Hi, >>> >>> I create a branch in the chrisl repo call "review-ramsay" for your new >>> follow up patches. >>> I plan to do incremental fix up if any on that branch. Then when we >>> both happy about it, >>> I will do rebase and smash the commit before push to master branch. >> >> Yes, I noticed the new branch - thanks! >> >> Did you have any thoughts regarding Josh Triplett's comments on the >> "compile-i386.c: don't ignore return value of write(2)" patch? > > I am fine with either way. The current fix is fine. I'm fine with this too. ;-) > If you are up for > a new xwrite function, that is of course fine as well. Without putting too much thought into it, such a beast would probably look something like this: #include <stddef.h> #include <unistd.h> #include <errno.h> ssize_t xwrite(int fd, const void *buf, size_t len) { const char *p = buf; ssize_t count = 0; while (len > 0) { ssize_t nw = write(fd, p, len); if (nw < 0) { if (errno == EAGAIN || errno == EINTR) continue; /* recoverable error */ return -1; } len -= nw; p += nw; count += nw; } return count; } Actually, I would need to think about what to do if write() returns zero. Such a return is not an error, it simply didn't write anything. Are we guaranteed that some progress will eventually be made, or should there be another check for non-progress in the loop. dunno. :( > Using printf() > is not much an improvement because printf() can still return > bytes less than the amount you requested. Hmm, I don't understand this. _In general_, you don't know how many bytes you are going to write before you call printf() and you don't explicitly request any amount. Assuming no errors, which are indicated with a negative return value, printf() returns the number of bytes actually 'written'. The standard I/O routines, including all the internal buffering, are much easier to use than the raw system calls (you don't have to cater for short writes etc., like the code above, because the standard library takes care of all of that for you). My preference for the first patch (replacing calls to write with printf), has little to do with this issue. I simply see no advantage in mixing calls to the standard I/O library with write() system calls. (The call to fflush(stdout) on line 889 has nothing to do with paranoia; it has to do with correctness and is very much needed!) Using printf() is more portable than write(). The emit_insn_atom() could be greatly simplified by calling printf() directly rather than using a sprintf/write combo. (not implemented in my patch). I would be very surprised if using write() was any more efficient than printf(). I find the code easier to comprehend without mixing I/O styles. So, once again, what advantage does write() bestow? Having said that, although I think the latest patch is perfectly fine, if you would like me to try expanding on the above xwrite() idea, just let me know. ATB, Ramsay Jones -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 23, 2014 at 8:38 AM, Ramsay Jones <ramsay@ramsay1.demon.co.uk> wrote: > > Without putting too much thought into it, such a beast would probably > look something like this: > > #include <stddef.h> > #include <unistd.h> > #include <errno.h> > > ssize_t xwrite(int fd, const void *buf, size_t len) > { > const char *p = buf; > ssize_t count = 0; > > while (len > 0) { > ssize_t nw = write(fd, p, len); > if (nw < 0) { > if (errno == EAGAIN || errno == EINTR) > continue; /* recoverable error */ > return -1; > } > len -= nw; > p += nw; > count += nw; > } > return count; > } > > Actually, I would need to think about what to do if write() returns > zero. Such a return is not an error, it simply didn't write anything. > Are we guaranteed that some progress will eventually be made, or > should there be another check for non-progress in the loop. dunno. :( The while loop need to have some ending conditions otherwise it can result in no progress make on a loop. e.g. calling on a non blocking file descriptor and the write will block. I think for the compile-i386.c. It is fine just die on short write. It is just an example program. > Hmm, I don't understand this. _In general_, you don't know how many > bytes you are going to write before you call printf() and you don't > explicitly request any amount. Assuming no errors, which are indicated > with a negative return value, printf() returns the number of bytes > actually 'written'. The standard I/O routines, including all the internal > buffering, are much easier to use than the raw system calls (you don't > have to cater for short writes etc., like the code above, because the > standard library takes care of all of that for you). My feed back is base on the reason of the change. If your intend is to get rid of the warning of not checking the error condition. Then the right thing to do is to just check and handle it. Die on error is also one kind of handle. Might be a bit harsh, but fine for the example program. In this respect, printf is not better. I am actually not sure how printf take care care of the short write. "printf" is very complex. I image if there is real error on the file system level. e.g. disk full. "printf" will have to return shorter than you expected. So printf will not get rid of the short write completely. > > My preference for the first patch (replacing calls to write with printf), > has little to do with this issue. I simply see no advantage in mixing > calls to the standard I/O library with write() system calls. (The call > to fflush(stdout) on line 889 has nothing to do with paranoia; it has to > do with correctness and is very much needed!) Then your intend is different than what I previously understand. If you call that change to unify the writing style of printf vs write. Converting write to use printf is acceptable. BTW, if there is no formatting string, why not use some thing like "puts()" instead? However, using printf without checking is not a right fix for the reason to address error condition not check. It still have unchecked error conditions. > Using printf() is more portable than write(). The emit_insn_atom() could > be greatly simplified by calling printf() directly rather than using > a sprintf/write combo. (not implemented in my patch). I would be very > surprised if using write() was any more efficient than printf(). I find Actually write is more efficient than printf. "write" is just a wrapper to the system call "write". "printf" is another strong. It is very complex. But that is not relevant here. I vote for left the patch as it is. Thanks Chris -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 25/10/14 05:14, Christopher Li wrote: > On Thu, Oct 23, 2014 at 8:38 AM, Ramsay Jones > <ramsay@ramsay1.demon.co.uk> wrote: >> >> Without putting too much thought into it, such a beast would probably >> look something like this: >> >> #include <stddef.h> >> #include <unistd.h> >> #include <errno.h> >> >> ssize_t xwrite(int fd, const void *buf, size_t len) >> { >> const char *p = buf; >> ssize_t count = 0; >> >> while (len > 0) { >> ssize_t nw = write(fd, p, len); >> if (nw < 0) { >> if (errno == EAGAIN || errno == EINTR) >> continue; /* recoverable error */ >> return -1; >> } >> len -= nw; >> p += nw; >> count += nw; >> } >> return count; >> } >> >> Actually, I would need to think about what to do if write() returns >> zero. Such a return is not an error, it simply didn't write anything. >> Are we guaranteed that some progress will eventually be made, or >> should there be another check for non-progress in the loop. dunno. :( > > The while loop need to have some ending conditions otherwise > it can result in no progress make on a loop. e.g. calling on a non > blocking file descriptor and the write will block. Yeah, I just typed that directly into my mail client and the first version had an 'if (nw == 0) return -1;' in the loop. However, I couldn't justify doing that, because it is not an error. I briefly thought about adding a 'non-progress' count to use to exit, but at this point I felt I would have to go away and start reading ... so I just punted. It was only a quick 'something like this ...' anyway! ;-) > I think for the compile-i386.c. It is fine just die on short write. > It is just an example program. Again, I'm happy to go with this too! >> Hmm, I don't understand this. _In general_, you don't know how many >> bytes you are going to write before you call printf() and you don't >> explicitly request any amount. Assuming no errors, which are indicated >> with a negative return value, printf() returns the number of bytes >> actually 'written'. The standard I/O routines, including all the internal >> buffering, are much easier to use than the raw system calls (you don't >> have to cater for short writes etc., like the code above, because the >> standard library takes care of all of that for you). > > My feed back is base on the reason of the change. If your intend is > to get rid of the warning of not checking the error condition. Then the > right thing to do is to just check and handle it. Die on error is also one > kind of handle. Might be a bit harsh, but fine for the example program. > In this respect, printf is not better. OK, point taken. [snip] > I vote for left the patch as it is. Me too. Thanks! ATB, Ramsay Jones -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/cgcc b/cgcc index 8e38174..364c77c 100755 --- a/cgcc +++ b/cgcc @@ -83,7 +83,9 @@ if ($do_check) { print "$check\n" if $verbose; if ($do_compile) { - system ($check) == 0 or exit 1; + my $code = system ($check); + print "code is: $code\n" ; + exit $code if $code != 0; } else { exec ($check); }