Patchwork [01/14] Introduce qemu_write_full()

login
register
mail settings
Submitter Kirill A. Shutemov
Date Dec. 31, 2009, 1:33 a.m.
Message ID <1262223199-19062-1-git-send-email-kirill@shutemov.name>
Download mbox | patch
Permalink /patch/41946/
State New
Headers show

Comments

Kirill A. Shutemov - Dec. 31, 2009, 1:33 a.m.
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(-)
malc - Dec. 31, 2009, 2 a.m.
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
>
Kirill A. Shutemov - Dec. 31, 2009, 7:03 a.m.
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.
malc - Dec. 31, 2009, 7:42 a.m.
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.
Paolo Bonzini - Dec. 31, 2009, 7:53 a.m.
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
Kirill A. Shutemov - Dec. 31, 2009, 8:50 a.m.
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.
malc - Dec. 31, 2009, 11:47 a.m.
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.
Andreas Färber - Dec. 31, 2009, 12:39 p.m.
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
Juan Quintela - Jan. 19, 2010, 12:11 p.m.
"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.
Kirill A. Shutemov - Jan. 19, 2010, 12:17 p.m.
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.
Blue Swirl - Jan. 19, 2010, 6:50 p.m.
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.
Anthony Liguori - Jan. 19, 2010, 7:43 p.m.
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

>
>
Juan Quintela - Jan. 20, 2010, 12:04 a.m.
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.
Anthony Liguori - Jan. 20, 2010, 1:04 a.m.
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
Jamie Lokier - Jan. 20, 2010, 1:30 a.m.
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

Patch

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