[PATCH] tools/libxl: Fix the errno

[PATCH] tools/libxl: Fix the errno

[Wen Congyang]

After commit 6d896e13, we should pass -errno on read failure.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
 tools/libxl/libxl_aoutils.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
index 0d4c8af..b93f0e4 100644
--- a/tools/libxl/libxl_aoutils.c
+++ b/tools/libxl/libxl_aoutils.c
@@ -262,7 +262,7 @@ static void datacopier_readable(libxl__egc *egc, libxl__ev_fd *ev,
                 assert(ferror(dc->log));
                 assert(errno);
                 LOGE(ERROR, "error logging %s", dc->copywhat);
-                datacopier_callback(egc, dc, 0, errno);
+                datacopier_callback(egc, dc, 0, -errno);
                 return;
             }
         }
-- 
2.1.0

Re: [PATCH] tools/libxl: Fix the errno

[Ross Lagerwall]

On 03/20/2015 08:17 AM, Wen Congyang wrote:
> After commit 6d896e13, we should pass -errno on read failure.
>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> ---
>   tools/libxl/libxl_aoutils.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
> index 0d4c8af..b93f0e4 100644
> --- a/tools/libxl/libxl_aoutils.c
> +++ b/tools/libxl/libxl_aoutils.c
> @@ -262,7 +262,7 @@ static void datacopier_readable(libxl__egc *egc, libxl__ev_fd *ev,
>                   assert(ferror(dc->log));
>                   assert(errno);
>                   LOGE(ERROR, "error logging %s", dc->copywhat);
> -                datacopier_callback(egc, dc, 0, errno);
> +                datacopier_callback(egc, dc, 0, -errno);
>                   return;
>               }
>           }
>

Acked-by: Ross Lagerwall <ross.lagerwall@citrix.com>

-- 
Ross Lagerwall

Re: [PATCH] tools/libxl: Fix the errno

[Wen Congyang]

On 03/20/2015 04:25 PM, Ross Lagerwall wrote:
> On 03/20/2015 08:17 AM, Wen Congyang wrote:
>> After commit 6d896e13, we should pass -errno on read failure.
>>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> ---
>>   tools/libxl/libxl_aoutils.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
>> index 0d4c8af..b93f0e4 100644
>> --- a/tools/libxl/libxl_aoutils.c
>> +++ b/tools/libxl/libxl_aoutils.c
>> @@ -262,7 +262,7 @@ static void datacopier_readable(libxl__egc *egc, libxl__ev_fd *ev,
>>                   assert(ferror(dc->log));
>>                   assert(errno);
>>                   LOGE(ERROR, "error logging %s", dc->copywhat);
>> -                datacopier_callback(egc, dc, 0, errno);
>> +                datacopier_callback(egc, dc, 0, -errno);
>>                   return;
>>               }
>>           }
>>
> 
> Acked-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> 

I forgot another place, it should be:

diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
index 0d4c8af..965afc3 100644
--- a/tools/libxl/libxl_aoutils.c
+++ b/tools/libxl/libxl_aoutils.c
@@ -249,7 +249,7 @@ static void datacopier_readable(libxl__egc *egc, libxl__ev_fd *ev,
             if (errno == EWOULDBLOCK) break;
             LOGE(ERROR, "error reading %s during copy of %s",
                  dc->readwhat, dc->copywhat);
-            datacopier_callback(egc, dc, 0, errno);
+            datacopier_callback(egc, dc, 0, -errno);
             return;
         }
         if (r == 0) {
@@ -262,7 +262,7 @@ static void datacopier_readable(libxl__egc *egc, libxl__ev_fd *ev,
                 assert(ferror(dc->log));
                 assert(errno);
                 LOGE(ERROR, "error logging %s", dc->copywhat);
-                datacopier_callback(egc, dc, 0, errno);
+                datacopier_callback(egc, dc, 0, -errno);
                 return;
             }
         }

Re: [PATCH] tools/libxl: Fix the errno

[Ian Campbell]

On Fri, 2015-03-20 at 16:17 +0800, Wen Congyang wrote:
> After commit 6d896e13, we should pass -errno on read failure.

Hrm, this means that 6d896e13 was a more subtle interface change than I
was expecting (I'd forgotten that errno wasn't negative in userspace).

This means that someone needs to audit all of the callbacks and check
that they do the right thing. But...

I'm not at all convinced that this is true for the bootloader ones: They
treat -1 as pollhup, which is not the same as -EPERM, which is because
the same callback is reused as callback_pollhup, with a comment:
       /* pollhup gets called with errnoval==-1 which is not otherwise
        * possible since errnos are nonnegative, so it's unambiguous */
which is now no longer true.

Do the new callers actually need the number of bytes read/written or was
this just something which seemed like a good idea since it was in hand?

If it isn't needed then lets go back to the old semantics and pass 0 on
EOF or bytes_to_read have been read (essentially "0 bytes left to
read"), I expect the recipient of the callback should know (or could
remember) the initial value of bytes_to_read? 

Otherwise I think the only sensible approach would be to add a new
parameter to the callback for the number of bytes and but errnoval back
to the old semantics.

Or perhaps requiring a separater callback vs. pollhup_callback could
solve this too, they would have different prototypes.

Please can one of you look into this ASAP, otherwise I think we should
revert until it can get fixed.

Ian.

Re: [PATCH] tools/libxl: Fix the errno

[Ian Campbell]

On Fri, 2015-03-20 at 11:03 +0000, Ian Campbell wrote:
> Do the new callers actually need the number of bytes read/written or was
> this just something which seemed like a good idea since it was in hand?
> 
> If it isn't needed

In fact, irrespective of the needs of the future callers lets go back to
the old semantics of errnoval for now, since it should be a quick and
easy patch, I think.

Then if it is actually needed we can sort out the propagation of the
number of bytes read in a new patch as part of that series.

>  then lets go back to the old semantics and pass 0 on
> EOF or bytes_to_read have been read (essentially "0 bytes left to
> read"), I expect the recipient of the callback should know (or could
> remember) the initial value of bytes_to_read? 
> 
> Otherwise I think the only sensible approach would be to add a new
> parameter to the callback for the number of bytes and but errnoval back
> to the old semantics.
> 
> Or perhaps requiring a separater callback vs. pollhup_callback could
> solve this too, they would have different prototypes.
> 
> Please can one of you look into this ASAP, otherwise I think we should
> revert until it can get fixed.
> 
> Ian.

Re: [PATCH] tools/libxl: Fix the errno

[Ross Lagerwall]

On 03/20/2015 11:15 AM, Ian Campbell wrote:
> On Fri, 2015-03-20 at 11:03 +0000, Ian Campbell wrote:
>> Do the new callers actually need the number of bytes read/written or was
>> this just something which seemed like a good idea since it was in hand?
>>
>> If it isn't needed
>
> In fact, irrespective of the needs of the future callers lets go back to
> the old semantics of errnoval for now, since it should be a quick and
> easy patch, I think.
>
> Then if it is actually needed we can sort out the propagation of the
> number of bytes read in a new patch as part of that series.
>

Some indication of whether the number of bytes requested were actually 
read is needed. Consider reading a struct of size X off the wire. The 
caller needs to know whether the number of bytes requested was actually 
was read or if they got an early EOF.

But in the meantime I will send a patch to fix restore the original 
behavior for errnoval.

--
Ross Lagerwall

Re: [PATCH] tools/libxl: Fix the errno

[Ian Campbell]

On Fri, 2015-03-20 at 16:02 +0000, Ross Lagerwall wrote:
> On 03/20/2015 11:15 AM, Ian Campbell wrote:
> > On Fri, 2015-03-20 at 11:03 +0000, Ian Campbell wrote:
> >> Do the new callers actually need the number of bytes read/written or was
> >> this just something which seemed like a good idea since it was in hand?
> >>
> >> If it isn't needed
> >
> > In fact, irrespective of the needs of the future callers lets go back to
> > the old semantics of errnoval for now, since it should be a quick and
> > easy patch, I think.
> >
> > Then if it is actually needed we can sort out the propagation of the
> > number of bytes read in a new patch as part of that series.
> >
> 
> Some indication of whether the number of bytes requested were actually 
> read is needed. Consider reading a struct of size X off the wire. The 
> caller needs to know whether the number of bytes requested was actually 
> was read or if they got an early EOF.

OK, so not actually the full number of bytes but just all vs. !all would
do? (it might be easier to massage that into the current interface)

> But in the meantime I will send a patch to fix restore the original 
> behavior for errnoval.

Thanks.

Ian.