<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Tue, Jun 5, 2018 at 4:40 PM Richard W.M. Jones <<a href="mailto:rjones@redhat.com">rjones@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Thanks: Nir Soffer<br>
---<br>
 v2v/rhv-upload-plugin.py | 66 ++++++++++++++++++++++++++++--------------------<br>
 1 file changed, 39 insertions(+), 27 deletions(-)<br>
<br>
diff --git a/v2v/rhv-upload-plugin.py b/v2v/rhv-upload-plugin.py<br>
index 791c9e7d2..c3de7d555 100644<br>
--- a/v2v/rhv-upload-plugin.py<br>
+++ b/v2v/rhv-upload-plugin.py<br>
@@ -228,6 +228,29 @@ def can_flush(h):<br>
 def get_size(h):<br>
     return params['disk_size']<br>
<br>
+# Any unexpected HTTP response status from the server will end up<br>
+# calling this function which logs the full error, pauses the<br>
+# transfer, sets the failed state, and raises a RuntimeError<br>
+# exception.<br>
+def unexpected_response(h, r, msg):<br></blockquote><div><br></div><div>This is not really unexpected response, maybe "request_failed"?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+    # Setting the failed flag in the handle causes the disk to be<br>
+    # cleaned up on close.<br>
+    h['failed'] = True<br>
+    h['transfer_service'].pause()<br>
+<br>
+    status = r.status<br>
+    reason = r.reason<br>
+    body = r.read()<br></blockquote><div><br></div><div>This should not fail, but if we there is a possible failure, it would be more</div><div>robust to do:</div><div><br></div><div>    try:</div><div>        body = r.read()</div><div>    except EnvironmentError as e:</div><div>        body = "(Unable to read response body: %s)" % e</div><div><br></div><div>If this raises as is, we would fail with EnvironmentError, hiding</div><div>the status, reason, and msg.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+    # Log the full error if we're verbose.<br>
+    debug("unexpected response from imageio server:")<br>
+    debug(msg)<br>
+    debug("%d: %s" % (status, reason))<br>
+    debug(body)<br></blockquote><div><br></div><div>Hiding fatal error in non-verbose will make it hard to support.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+    # Only a short error is included in the exception.<br>
+    raise RuntimeError("%s: %d: %s" % (msg, status, reason))<br></blockquote><div><br></div><div>And instead include the start of the body in the error text like this:</div><div><br></div><div>    raise RuntimeError("%s: [%d] %s: %r", msg, status, reason, bofy[:200])</div><div><br></div><div>(Using %r to avoid unwanted newlines in the response).</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
 # For documentation see:<br>
 # <a href="https://github.com/oVirt/ovirt-imageio/blob/master/docs/random-io.md" rel="noreferrer" target="_blank">https://github.com/oVirt/ovirt-imageio/blob/master/docs/random-io.md</a><br>
 # For examples of working code to read/write from the server, see:<br>
@@ -248,16 +271,14 @@ def pread(h, count, offset):<br>
     r = http.getresponse()<br>
     # 206 = HTTP Partial Content.<br>
     if r.status != 206:<br>
-        h['transfer_service'].pause()<br>
-        h['failed'] = True<br>
-        raise RuntimeError("could not read sector (%d, %d): %d: %s" %<br>
-                           (offset, count, r.status, r.reason))<br>
+        unexpected_response(h, r,<br>
+                            "could not read sector offset %d size %d" %<br>
+                            (offset, count))<br></blockquote><div><br></div><div>This is much nicer.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
     return r.read()<br>
<br>
 def pwrite(h, buf, offset):<br>
     http = h['http']<br>
     transfer = h['transfer']<br>
-    transfer_service = h['transfer_service']<br>
<br>
     count = len(buf)<br>
     h['highestwrite'] = max(h['highestwrite'], offset+count)<br>
@@ -275,15 +296,13 @@ def pwrite(h, buf, offset):<br>
<br>
     r = http.getresponse()<br>
     if r.status != 200:<br>
-        transfer_service.pause()<br>
-        h['failed'] = True<br>
-        raise RuntimeError("could not write sector (%d, %d): %d: %s" %<br>
-                           (offset, count, r.status, r.reason))<br>
+        unexpected_response(h, r,<br>
+                            "could not write sector offset %d size %d" %<br>
+                            (offset, count))<br>
<br>
 def zero(h, count, offset, may_trim):<br>
     http = h['http']<br>
     transfer = h['transfer']<br>
-    transfer_service = h['transfer_service']<br>
<br>
     # Unlike the trim and flush calls, there is no 'can_zero' method<br>
     # so nbdkit could call this even if the server doesn't support<br>
@@ -306,10 +325,9 @@ def zero(h, count, offset, may_trim):<br>
<br>
     r = http.getresponse()<br>
     if r.status != 200:<br>
-        transfer_service.pause()<br>
-        h['failed'] = True<br>
-        raise RuntimeError("could not zero sector (%d, %d): %d: %s" %<br>
-                           (offset, count, r.status, r.reason))<br>
+        unexpected_response(h, r,<br>
+                            "could not zero sector offset %d size %d" %<br>
+                            (offset, count))<br>
<br>
 def emulate_zero(h, count, offset):<br>
     # qemu-img convert starts by trying to zero/trim the whole device.<br>
@@ -334,15 +352,13 @@ def emulate_zero(h, count, offset):<br>
<br>
         r = http.getresponse()<br>
         if r.status != 200:<br>
-            transfer_service.pause()<br>
-            h['failed'] = True<br>
-            raise RuntimeError("could not write zeroes (%d, %d): %d: %s" %<br>
-                               (offset, count, r.status, r.reason))<br>
+            unexpected_response(h, r,<br>
+                                "could not write zeroes offset %d size %d" %<br>
+                                (offset, count))<br>
<br>
 def trim(h, count, offset):<br>
     http = h['http']<br>
     transfer = h['transfer']<br>
-    transfer_service = h['transfer_service']<br>
<br>
     # Construct the JSON request for trimming.<br>
     buf = json.dumps({'op': "trim",<br>
@@ -358,15 +374,13 @@ def trim(h, count, offset):<br>
<br>
     r = http.getresponse()<br>
     if r.status != 200:<br>
-        transfer_service.pause()<br>
-        h['failed'] = True<br>
-        raise RuntimeError("could not trim sector (%d, %d): %d: %s" %<br>
-                           (offset, count, r.status, r.reason))<br>
+        unexpected_response(h, r,<br>
+                            "could not trim sector offset %d size %d" %<br>
+                            (offset, count))<br>
<br>
 def flush(h):<br>
     http = h['http']<br>
     transfer = h['transfer']<br>
-    transfer_service = h['transfer_service']<br>
<br>
     # Construct the JSON request for flushing.<br>
     buf = json.dumps({'op': "flush"}).encode()<br>
@@ -379,9 +393,7 @@ def flush(h):<br>
<br>
     r = http.getresponse()<br>
     if r.status != 200:<br>
-        transfer_service.pause()<br>
-        h['failed'] = True<br>
-        raise RuntimeError("could not flush: %d: %s" % (r.status, r.reason))<br>
+        unexpected_response(h, r, "could not flush")<br>
<br>
 def delete_disk_on_failure(h):<br>
     disk_service = h['disk_service']<br>
<br></blockquote><div><br></div><div>Nir </div></div></div>