| Message ID | 1262223199-19062-1-git-send-email-kirill@shutemov.name |
|---|---|
| State | New |
| Headers | show |
On Thu, 31 Dec 2009, Kirill A. Shutemov wrote: > A variant of write(2) which handles partial write. > > Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name> > --- > osdep.c | 27 +++++++++++++++++++++++++++ > qemu-common.h | 1 + > 2 files changed, 28 insertions(+), 0 deletions(-) > > diff --git a/osdep.c b/osdep.c > index e4836e7..d2406f2 100644 > --- a/osdep.c > +++ b/osdep.c > @@ -243,6 +243,33 @@ int qemu_open(const char *name, int flags, ...) > return ret; > } > > +/* > + * A variant of write(2) which handles partial write. > + * > + * Return the number of bytes transferred. > + * Set errno if fewer than `count' bytes are written. > + */ > +ssize_t qemu_write_full(int fd, const void *buf, size_t count) > +{ > + ssize_t ret = 0; > + ssize_t total = 0; > + > + while (count) { > + ret = write(fd, buf, count); > + if (ret < 0) { > + if (errno == EINTR) > + continue; > + break; > + } > + > + count -= ret; > + buf += ret; > + total += ret; > + } > + > + return total; > +} This hides write errors. > + > #ifndef _WIN32 > /* > * Creates a pipe with FD_CLOEXEC set on both file descriptors > diff --git a/qemu-common.h b/qemu-common.h > index 8630f8c..a8144cb 100644 > --- a/qemu-common.h > +++ b/qemu-common.h > @@ -160,6 +160,7 @@ void qemu_mutex_lock_iothread(void); > void qemu_mutex_unlock_iothread(void); > > int qemu_open(const char *name, int flags, ...); > +ssize_t qemu_write_full(int fd, const void *buf, size_t count); > void qemu_set_cloexec(int fd); > > #ifndef _WIN32 >
On Thu, Dec 31, 2009 at 4:00 AM, malc <av1474@comtv.ru> wrote: > On Thu, 31 Dec 2009, Kirill A. Shutemov wrote: > >> A variant of write(2) which handles partial write. >> >> Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name> >> --- >> osdep.c | 27 +++++++++++++++++++++++++++ >> qemu-common.h | 1 + >> 2 files changed, 28 insertions(+), 0 deletions(-) >> >> diff --git a/osdep.c b/osdep.c >> index e4836e7..d2406f2 100644 >> --- a/osdep.c >> +++ b/osdep.c >> @@ -243,6 +243,33 @@ int qemu_open(const char *name, int flags, ...) >> return ret; >> } >> >> +/* >> + * A variant of write(2) which handles partial write. >> + * >> + * Return the number of bytes transferred. >> + * Set errno if fewer than `count' bytes are written. >> + */ >> +ssize_t qemu_write_full(int fd, const void *buf, size_t count) >> +{ >> + ssize_t ret = 0; >> + ssize_t total = 0; >> + >> + while (count) { >> + ret = write(fd, buf, count); >> + if (ret < 0) { >> + if (errno == EINTR) >> + continue; >> + break; >> + } >> + >> + count -= ret; >> + buf += ret; >> + total += ret; >> + } >> + >> + return total; >> +} > > This hides write errors. Why do you think so? Return value < count indicates error.
On Thu, 31 Dec 2009, Kirill A. Shutemov wrote: > On Thu, Dec 31, 2009 at 4:00 AM, malc <av1474@comtv.ru> wrote: > > On Thu, 31 Dec 2009, Kirill A. Shutemov wrote: > > > >> A variant of write(2) which handles partial write. > >> > >> Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name> > >> --- > >> osdep.c | 27 +++++++++++++++++++++++++++ > >> qemu-common.h | 1 + > >> 2 files changed, 28 insertions(+), 0 deletions(-) > >> > >> diff --git a/osdep.c b/osdep.c > >> index e4836e7..d2406f2 100644 > >> --- a/osdep.c > >> +++ b/osdep.c > >> @@ -243,6 +243,33 @@ int qemu_open(const char *name, int flags, ...) > >> return ret; > >> } > >> > >> +/* > >> + * A variant of write(2) which handles partial write. > >> + * > >> + * Return the number of bytes transferred. > >> + * Set errno if fewer than `count' bytes are written. > >> + */ > >> +ssize_t qemu_write_full(int fd, const void *buf, size_t count) > >> +{ > >> + ssize_t ret = 0; > >> + ssize_t total = 0; > >> + > >> + while (count) { > >> + ret = write(fd, buf, count); > >> + if (ret < 0) { > >> + if (errno == EINTR) > >> + continue; > >> + break; > >> + } > >> + > >> + count -= ret; > >> + buf += ret; > >> + total += ret; > >> + } > >> + > >> + return total; > >> +} > > > > This hides write errors. > > Why do you think so? Return value < count indicates error. It does not indicate _which_ error it was.
On 12/31/2009 02:33 AM, Kirill A. Shutemov wrote: > diff --git a/qemu-common.h b/qemu-common.h > index 8630f8c..a8144cb 100644 > --- a/qemu-common.h > +++ b/qemu-common.h > @@ -160,6 +160,7 @@ void qemu_mutex_lock_iothread(void); > void qemu_mutex_unlock_iothread(void); > > int qemu_open(const char *name, int flags, ...); > +ssize_t qemu_write_full(int fd, const void *buf, size_t count); > void qemu_set_cloexec(int fd); > > #ifndef _WIN32 This should also use __attribute__ ((warn_unused_result)). Paolo
2009/12/31 malc <av1474@comtv.ru>: > On Thu, 31 Dec 2009, Kirill A. Shutemov wrote: > >> On Thu, Dec 31, 2009 at 4:00 AM, malc <av1474@comtv.ru> wrote: >> > On Thu, 31 Dec 2009, Kirill A. Shutemov wrote: >> > >> >> A variant of write(2) which handles partial write. >> >> >> >> Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name> >> >> --- >> >> osdep.c | 27 +++++++++++++++++++++++++++ >> >> qemu-common.h | 1 + >> >> 2 files changed, 28 insertions(+), 0 deletions(-) >> >> >> >> diff --git a/osdep.c b/osdep.c >> >> index e4836e7..d2406f2 100644 >> >> --- a/osdep.c >> >> +++ b/osdep.c >> >> @@ -243,6 +243,33 @@ int qemu_open(const char *name, int flags, ...) >> >> return ret; >> >> } >> >> >> >> +/* >> >> + * A variant of write(2) which handles partial write. >> >> + * >> >> + * Return the number of bytes transferred. >> >> + * Set errno if fewer than `count' bytes are written. >> >> + */ >> >> +ssize_t qemu_write_full(int fd, const void *buf, size_t count) >> >> +{ >> >> + ssize_t ret = 0; >> >> + ssize_t total = 0; >> >> + >> >> + while (count) { >> >> + ret = write(fd, buf, count); >> >> + if (ret < 0) { >> >> + if (errno == EINTR) >> >> + continue; >> >> + break; >> >> + } >> >> + >> >> + count -= ret; >> >> + buf += ret; >> >> + total += ret; >> >> + } >> >> + >> >> + return total; >> >> +} >> > >> > This hides write errors. >> >> Why do you think so? Return value < count indicates error. > > It does not indicate _which_ error it was. ??? You can check errno.
On Thu, 31 Dec 2009, Kirill A. Shutemov wrote: > 2009/12/31 malc <av1474@comtv.ru>: > > On Thu, 31 Dec 2009, Kirill A. Shutemov wrote: > > > >> On Thu, Dec 31, 2009 at 4:00 AM, malc <av1474@comtv.ru> wrote: > >> > On Thu, 31 Dec 2009, Kirill A. Shutemov wrote: > >> > > >> >> A variant of write(2) which handles partial write. > >> >> > >> >> Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name> > >> >> --- > >> >> osdep.c | 27 +++++++++++++++++++++++++++ > >> >> qemu-common.h | 1 + > >> >> 2 files changed, 28 insertions(+), 0 deletions(-) > >> >> > >> >> diff --git a/osdep.c b/osdep.c > >> >> index e4836e7..d2406f2 100644 > >> >> --- a/osdep.c > >> >> +++ b/osdep.c > >> >> @@ -243,6 +243,33 @@ int qemu_open(const char *name, int flags, ...) > >> >> return ret; > >> >> } > >> >> > >> >> +/* > >> >> + * A variant of write(2) which handles partial write. > >> >> + * > >> >> + * Return the number of bytes transferred. > >> >> + * Set errno if fewer than `count' bytes are written. > >> >> + */ > >> >> +ssize_t qemu_write_full(int fd, const void *buf, size_t count) > >> >> +{ > >> >> + ssize_t ret = 0; > >> >> + ssize_t total = 0; > >> >> + > >> >> + while (count) { > >> >> + ret = write(fd, buf, count); > >> >> + if (ret < 0) { > >> >> + if (errno == EINTR) > >> >> + continue; > >> >> + break; > >> >> + } > >> >> + > >> >> + count -= ret; > >> >> + buf += ret; > >> >> + total += ret; > >> >> + } > >> >> + > >> >> + return total; > >> >> +} > >> > > >> > This hides write errors. > >> > >> Why do you think so? Return value < count indicates error. > > > > It does not indicate _which_ error it was. > > ??? > > You can check errno. Uh, yes, sorry, been writing too much driver code lately. Nitpick then, no reason to set ret to zero at the beginning of the function.
Am 31.12.2009 um 09:50 schrieb Kirill A. Shutemov: > 2009/12/31 malc <av1474@comtv.ru>: >> On Thu, 31 Dec 2009, Kirill A. Shutemov wrote: >> >>> On Thu, Dec 31, 2009 at 4:00 AM, malc <av1474@comtv.ru> wrote: >>>> On Thu, 31 Dec 2009, Kirill A. Shutemov wrote: >>>> >>>>> A variant of write(2) which handles partial write. >>>>> >>>>> Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name> >>>>> --- >>>>> osdep.c | 27 +++++++++++++++++++++++++++ >>>>> qemu-common.h | 1 + >>>>> 2 files changed, 28 insertions(+), 0 deletions(-) >>>>> >>>>> diff --git a/osdep.c b/osdep.c >>>>> index e4836e7..d2406f2 100644 >>>>> --- a/osdep.c >>>>> +++ b/osdep.c >>>>> @@ -243,6 +243,33 @@ int qemu_open(const char *name, int >>>>> flags, ...) >>>>> return ret; >>>>> } >>>>> >>>>> +/* >>>>> + * A variant of write(2) which handles partial write. >>>>> + * >>>>> + * Return the number of bytes transferred. >>>>> + * Set errno if fewer than `count' bytes are written. >>>>> + */ >>>>> +ssize_t qemu_write_full(int fd, const void *buf, size_t count) >>>>> +{ >>>>> + ssize_t ret = 0; >>>>> + ssize_t total = 0; >>>>> + >>>>> + while (count) { >>>>> + ret = write(fd, buf, count); >>>>> + if (ret < 0) { >>>>> + if (errno == EINTR) >>>>> + continue; >>>>> + break; >>>>> + } >>>>> + >>>>> + count -= ret; >>>>> + buf += ret; >>>>> + total += ret; >>>>> + } >>>>> + >>>>> + return total; >>>>> +} >>>> >>>> This hides write errors. >>> >>> Why do you think so? Return value < count indicates error. >> >> It does not indicate _which_ error it was. > > ??? > > You can check errno. So all callers need to compare count to the return value, to know when to read errno. Nitpick: count is size_t while total is ssize_t, so the required comparison seems a little ugly (without having tested it). Andreas
"Kirill A. Shutemov" <kirill@shutemov.name> wrote: > A variant of write(2) which handles partial write. > > Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name> Hi Have you updated this series? Is there any reason that you know when they haven't been picked? I am also interested in getting _FORTIFY_SOURCE=2 wo compile cleanly. Thanks in advance, Juan.
On Tue, Jan 19, 2010 at 2:11 PM, Juan Quintela <quintela@redhat.com> wrote: > "Kirill A. Shutemov" <kirill@shutemov.name> wrote: >> A variant of write(2) which handles partial write. >> >> Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name> > > Hi > > Have you updated this series? Is there any reason that you know when > they haven't been picked? I don't know any reason, but I'm going to review it once again. I also have plan to get rid of -fno-strict-aliasing where it's possible.
On Tue, Jan 19, 2010 at 12:17 PM, Kirill A. Shutemov <kirill@shutemov.name> wrote: > On Tue, Jan 19, 2010 at 2:11 PM, Juan Quintela <quintela@redhat.com> wrote: >> "Kirill A. Shutemov" <kirill@shutemov.name> wrote: >>> A variant of write(2) which handles partial write. >>> >>> Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name> >> >> Hi >> >> Have you updated this series? Is there any reason that you know when >> they haven't been picked? > > I don't know any reason, but I'm going to review it once again. I don't know about others, but I didn't feel competent enough about all possible corner cases of write(). > I also have plan to get rid of -fno-strict-aliasing where it's possible. That should be interesting too, if it does not make code more unreadable.
On 01/19/2010 06:17 AM, Kirill A. Shutemov wrote: > On Tue, Jan 19, 2010 at 2:11 PM, Juan Quintela<quintela@redhat.com> wrote: > >> "Kirill A. Shutemov"<kirill@shutemov.name> wrote: >> >>> A variant of write(2) which handles partial write. >>> >>> Signed-off-by: Kirill A. Shutemov<kirill@shutemov.name> >>> >> Hi >> >> Have you updated this series? Is there any reason that you know when >> they haven't been picked? >> > I don't know any reason, but I'm going to review it once again. > > I also have plan to get rid of -fno-strict-aliasing where it's possible. > I haven't reviewed the series in detail, but generally speaking I don't feel that good about these sort of series. You're essentially adding dummy error handling to quiet the compiler. That's worse than just disabling -Werror because at least you aren't losing the information in the code. If you're going to update error handling, it should be part of an effort to make code paths resilient to error. IOW, actually audit the full error path of the function and make it deal with errors gracefully. Regards, Anthony Liguori > >
Anthony Liguori <anthony@codemonkey.ws> wrote: > On 01/19/2010 06:17 AM, Kirill A. Shutemov wrote: >> On Tue, Jan 19, 2010 at 2:11 PM, Juan Quintela<quintela@redhat.com> wrote: >> >>> "Kirill A. Shutemov"<kirill@shutemov.name> wrote: >>> >>>> A variant of write(2) which handles partial write. >>>> >>>> Signed-off-by: Kirill A. Shutemov<kirill@shutemov.name> >>>> >>> Hi >>> >>> Have you updated this series? Is there any reason that you know when >>> they haven't been picked? >>> >> I don't know any reason, but I'm going to review it once again. >> >> I also have plan to get rid of -fno-strict-aliasing where it's possible. >> > > I haven't reviewed the series in detail, but generally speaking I > don't feel that good about these sort of series. > > You're essentially adding dummy error handling to quiet the compiler. > That's worse than just disabling -Werror because at least you aren't > losing the information in the code. > > If you're going to update error handling, it should be part of an > effort to make code paths resilient to error. IOW, actually audit the > full error path of the function and make it deal with errors > gracefully. I reviewed his series, and I reviewed callers. Please take a look at my improved series. Appart for the comments added there, I don't know what to do here: @@ -501,8 +501,11 @@ static void aio_signal_handler(int signum) { if (posix_aio_state) { char byte = 0; + ssize_t ret; - write(posix_aio_state->wfd, &byte, sizeof(byte)); + ret = write(posix_aio_state->wfd, &byte, sizeof(byte)); + if (ret < 0 && errno != EAGAIN) + die("write()"); } if write() fails in a pipe in the signal handler, I am at a lost about what to do here. For the rest, I think that I did the proper error path handling. Thanks, Juan.
On 01/19/2010 06:04 PM, Juan Quintela wrote: > Anthony Liguori<anthony@codemonkey.ws> wrote: > >> On 01/19/2010 06:17 AM, Kirill A. Shutemov wrote: >> >>> On Tue, Jan 19, 2010 at 2:11 PM, Juan Quintela<quintela@redhat.com> wrote: >>> >>> >>>> "Kirill A. Shutemov"<kirill@shutemov.name> wrote: >>>> >>>> >>>>> A variant of write(2) which handles partial write. >>>>> >>>>> Signed-off-by: Kirill A. Shutemov<kirill@shutemov.name> >>>>> >>>>> >>>> Hi >>>> >>>> Have you updated this series? Is there any reason that you know when >>>> they haven't been picked? >>>> >>>> >>> I don't know any reason, but I'm going to review it once again. >>> >>> I also have plan to get rid of -fno-strict-aliasing where it's possible. >>> >>> >> I haven't reviewed the series in detail, but generally speaking I >> don't feel that good about these sort of series. >> >> You're essentially adding dummy error handling to quiet the compiler. >> That's worse than just disabling -Werror because at least you aren't >> losing the information in the code. >> >> If you're going to update error handling, it should be part of an >> effort to make code paths resilient to error. IOW, actually audit the >> full error path of the function and make it deal with errors >> gracefully. >> > I reviewed his series, and I reviewed callers. Please take a look at my > improved series. Appart for the comments added there, I don't know what > to do here: > > @@ -501,8 +501,11 @@ static void aio_signal_handler(int signum) > { > if (posix_aio_state) { > char byte = 0; > + ssize_t ret; > > - write(posix_aio_state->wfd,&byte, sizeof(byte)); > + ret = write(posix_aio_state->wfd,&byte, sizeof(byte)); > + if (ret< 0&& errno != EAGAIN) > + die("write()"); > } > > if write() fails in a pipe in the signal handler, I am at a lost about > what to do here. > That's nothing we can do. I guess exiting is reasonable. Regards, Anthony Liguori
Anthony Liguori wrote: > >- write(posix_aio_state->wfd,&byte, sizeof(byte)); > >+ ret = write(posix_aio_state->wfd,&byte, sizeof(byte)); > >+ if (ret< 0&& errno != EAGAIN) > >+ die("write()"); > > } > > > >if write() fails in a pipe in the signal handler, I am at a lost about > >what to do here. > > That's nothing we can do. I guess exiting is reasonable. At least retry if it returns EINTR. That can happen in a signal handler, if the handler does not block all other signals. -- Jamie
diff --git a/osdep.c b/osdep.c index e4836e7..d2406f2 100644 --- a/osdep.c +++ b/osdep.c @@ -243,6 +243,33 @@ int qemu_open(const char *name, int flags, ...) return ret; } +/* + * A variant of write(2) which handles partial write. + * + * Return the number of bytes transferred. + * Set errno if fewer than `count' bytes are written. + */ +ssize_t qemu_write_full(int fd, const void *buf, size_t count) +{ + ssize_t ret = 0; + ssize_t total = 0; + + while (count) { + ret = write(fd, buf, count); + if (ret < 0) { + if (errno == EINTR) + continue; + break; + } + + count -= ret; + buf += ret; + total += ret; + } + + return total; +} + #ifndef _WIN32 /* * Creates a pipe with FD_CLOEXEC set on both file descriptors diff --git a/qemu-common.h b/qemu-common.h index 8630f8c..a8144cb 100644 --- a/qemu-common.h +++ b/qemu-common.h @@ -160,6 +160,7 @@ void qemu_mutex_lock_iothread(void); void qemu_mutex_unlock_iothread(void); int qemu_open(const char *name, int flags, ...); +ssize_t qemu_write_full(int fd, const void *buf, size_t count); void qemu_set_cloexec(int fd); #ifndef _WIN32
A variant of write(2) which handles partial write. Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name> --- osdep.c | 27 +++++++++++++++++++++++++++ qemu-common.h | 1 + 2 files changed, 28 insertions(+), 0 deletions(-)