tools/libxl: Fix the errno

PATCH In review Wen Congyang 3 participants 7 messages Mar 20, 2015

lore

Share & tools

lore

Wen Congyang

author

2015-03-20 08:17 UTC #0 lore raw

PATCH 1/1 tools/libxl: Fix the errno

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

Signed-off-by Wen Congyang <[email protected]>

---
 tools/libxl/libxl_aoutils.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

tools/libxl/libxl_aoutils.c +1 -1

index 0d4c8af..b93f0e4 100644
@@ -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;
             }
         }

Signature · 1 line

2.1.0

Ross Lagerwall

Acked-by on v1 1/1

2015-03-20 08:25 UTC #1 lore raw

Quoted 13 lines · Wen Congyang

After commit 6d896e13, we should pass -errno on read failure.
Signed-off-by Wen Congyang <[email protected]>
---
  tools/libxl/libxl_aoutils.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
tools/libxl/libxl_aoutils.c +0 -0
index 0d4c8af..b93f0e4 100644

tools/libxl/libxl_aoutils.c static void datacopier_readable(libxl__egc *egc, libxl__ev_fd *ev, · line 268

Quoted 8 lines

tools/libxl/libxl_aoutils.c +1 -1

@@ -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 <[email protected]>

Signature · 1 line

Ross Lagerwall

Wen Congyang

author

2015-03-20 08:39 UTC #2 lore raw

Quoted 8 lines · Ross Lagerwall

Quoted 7 lines · Wen Congyang
After commit 6d896e13, we should pass -errno on read failure.
Signed-off-by Wen Congyang <[email protected]>
---
  tools/libxl/libxl_aoutils.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 17 lines

Quoted 6 lines

tools/libxl/libxl_aoutils.c +0 -0
index 0d4c8af..b93f0e4 100644

tools/libxl/libxl_aoutils.c static void datacopier_readable(libxl__egc *egc, libxl__ev_fd *ev, · line 268

Quoted 8 lines

tools/libxl/libxl_aoutils.c +1 -1

@@ -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 <[email protected]>

I forgot another place, it should be:

tools/libxl/libxl_aoutils.c +2 -2

index 0d4c8af..965afc3 100644
@@ -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;
             }
         }

Ian Campbell

2015-03-20 11:03 UTC #3 lore raw

In reply to Wen Congyang

Quoted 1 line · Wen Congyang

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.

Ian Campbell

2015-03-20 11:15 UTC #4 lore raw

Quoted 4 lines · Ian Campbell

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.

Quoted 16 lines

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.

Ross Lagerwall

2015-03-20 16:02 UTC #5 lore raw

Quoted 4 lines · Ian Campbell

Quoted 3 lines · Ian Campbell
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?

Quoted 9 lines

Quoted 1 line
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

Ian Campbell

2015-03-20 16:09 UTC #6 lore raw

Quoted 11 lines · Ross Lagerwall

Quoted 10 lines · Ian Campbell
Quoted 4 lines · Ian Campbell
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.

Quoted 8 lines

Quoted 3 lines
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)

Quoted 2 lines

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

Thanks.

Ian.