[Qemu-devel] Re: [PATCH 01/14] Introduce qemu_write_full()

[Qemu-devel] Re: [PATCH 01/14] Introduce qemu_write_full()

[Juan Quintela]

"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.

[Qemu-devel] Re: [PATCH 01/14] Introduce qemu_write_full()

[Kirill A. Shutemov]

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.

Re: [Qemu-devel] Re: [PATCH 01/14] Introduce qemu_write_full()

[Blue Swirl]

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.

Re: [Qemu-devel] Re: [PATCH 01/14] Introduce qemu_write_full()

[Anthony Liguori]

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

>
>    

[Qemu-devel] Re: [PATCH 01/14] Introduce qemu_write_full()

[Juan Quintela]

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.

[Qemu-devel] Re: [PATCH 01/14] Introduce qemu_write_full()

[Anthony Liguori]

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

Re: [Qemu-devel] Re: [PATCH 01/14] Introduce qemu_write_full()

[Jamie Lokier]

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