Let management know if there were any problems communicating with
qemu-pr-helper.  The event is edge-triggered, and is sent every time
the connection status of the pr-manager-helper object changes.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qapi/block.json          | 24 ++++++++++++++++++++++++
 scsi/pr-manager-helper.c | 25 +++++++++++++++++++------
 2 files changed, 43 insertions(+), 6 deletions(-)

diff --git a/qapi/block.json b/qapi/block.json
index dc3323c954..1882a8d107 100644
--- a/qapi/block.json
+++ b/qapi/block.json
@@ -334,6 +334,30 @@
 { 'event': 'DEVICE_TRAY_MOVED',
   'data': { 'device': 'str', 'id': 'str', 'tray-open': 'bool' } }
 
+##
+# @PR_MANAGER_STATUS_CHANGED:
+#
+# Emitted whenever the connected status of a persistent reservation
+# manager changes.
+#
+# @id: The QOM path of the PR manager object
+#
+# @connected: true if the PR manager is connected to a backend
+#
+# Since: 2.12
+#
+# Example:
+#
+# <- { "event": "PR_MANAGER_STATUS_CHANGED",
+#      "data": { "id": "/object/pr-helper0",
+#                "connected": true
+#      },
+#      "timestamp": { "seconds": 1519840375, "microseconds": 450486 } }
+#
+##
+{ 'event': 'PR_MANAGER_STATUS_CHANGED',
+  'data': { 'id': 'str', 'connected': 'bool' } }
+
 ##
 # @QuorumOpType:
 #
diff --git a/scsi/pr-manager-helper.c b/scsi/pr-manager-helper.c
index b11481be9e..6643caf4cf 100644
--- a/scsi/pr-manager-helper.c
+++ b/scsi/pr-manager-helper.c
@@ -17,6 +17,7 @@
 #include "io/channel.h"
 #include "io/channel-socket.h"
 #include "pr-helper.h"
+#include "qapi/qapi-events-block.h"
 
 #include <scsi/sg.h>
 
@@ -33,6 +34,7 @@ typedef struct PRManagerHelper {
     PRManager parent;
 
     char *path;
+    char *qom_path;
 
     QemuMutex lock;
     QIOChannel *ioc;
@@ -127,6 +129,8 @@ static int pr_manager_helper_initialize(PRManagerHelper *pr_mgr,
         goto out_close;
     }
 
+    qapi_event_send_pr_manager_status_changed(pr_mgr->qom_path, true,
+                                              &error_abort);
     return 0;
 
 out_close:
@@ -142,9 +146,11 @@ static int pr_manager_helper_run(PRManager *p,
 
     uint32_t len;
     PRHelperResponse resp;
+    int sense_len;
     int ret;
     int expected_dir;
     int attempts;
+    bool was_connected = pr_mgr->ioc != NULL;
     uint8_t cdb[PR_HELPER_CDB_SIZE] = { 0 };
 
     if (!io_hdr->cmd_len || io_hdr->cmd_len > PR_HELPER_CDB_SIZE) {
@@ -222,15 +228,19 @@ static int pr_manager_helper_run(PRManager *p,
         io_hdr->sb_len_wr = MIN(io_hdr->mx_sb_len, PR_HELPER_SENSE_SIZE);
         memcpy(io_hdr->sbp, resp.sense, io_hdr->sb_len_wr);
     }
+    qemu_mutex_unlock(&pr_mgr->lock);
+    return ret;
 
 out:
-    if (ret < 0) {
-        int sense_len = scsi_build_sense(io_hdr->sbp,
-                                         SENSE_CODE(LUN_COMM_FAILURE));
-        io_hdr->driver_status = SG_ERR_DRIVER_SENSE;
-        io_hdr->sb_len_wr = MIN(io_hdr->mx_sb_len, sense_len);
-        io_hdr->status = CHECK_CONDITION;
+    if (was_connected) {
+        qapi_event_send_pr_manager_status_changed(pr_mgr->qom_path, false,
+                                                  &error_abort);
     }
+
+    sense_len = scsi_build_sense(io_hdr->sbp, SENSE_CODE(LUN_COMM_FAILURE));
+    io_hdr->driver_status = SG_ERR_DRIVER_SENSE;
+    io_hdr->sb_len_wr = MIN(io_hdr->mx_sb_len, sense_len);
+    io_hdr->status = CHECK_CONDITION;
     qemu_mutex_unlock(&pr_mgr->lock);
     return ret;
 }
@@ -251,6 +261,8 @@ static void pr_manager_helper_complete(UserCreatable *uc, Error **errp)
 {
     PRManagerHelper *pr_mgr = PR_MANAGER_HELPER(uc);
 
+    pr_mgr->qom_path = object_get_canonical_path(OBJECT(pr_mgr));
+
     qemu_mutex_lock(&pr_mgr->lock);
     pr_manager_helper_initialize(pr_mgr, errp);
     qemu_mutex_unlock(&pr_mgr->lock);
@@ -276,6 +288,7 @@ static void pr_manager_helper_instance_finalize(Object *obj)
     PRManagerHelper *pr_mgr = PR_MANAGER_HELPER(obj);
 
     object_unref(OBJECT(pr_mgr->ioc));
+    g_free(pr_mgr->qom_path);
     qemu_mutex_destroy(&pr_mgr->lock);
 }
 
-- 
2.17.1

Hi Paolo,

On 06/26/2018 12:40 PM, Paolo Bonzini wrote:
> Let management know if there were any problems communicating with
> qemu-pr-helper.  The event is edge-triggered, and is sent every time
> the connection status of the pr-manager-helper object changes.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  qapi/block.json          | 24 ++++++++++++++++++++++++
>  scsi/pr-manager-helper.c | 25 +++++++++++++++++++------
>  2 files changed, 43 insertions(+), 6 deletions(-)
> 
> diff --git a/qapi/block.json b/qapi/block.json
> index dc3323c954..1882a8d107 100644
> --- a/qapi/block.json
> +++ b/qapi/block.json
> @@ -334,6 +334,30 @@
>  { 'event': 'DEVICE_TRAY_MOVED',
>    'data': { 'device': 'str', 'id': 'str', 'tray-open': 'bool' } }
>  
> +##
> +# @PR_MANAGER_STATUS_CHANGED:
> +#
> +# Emitted whenever the connected status of a persistent reservation
> +# manager changes.
> +#
> +# @id: The QOM path of the PR manager object
> +#
> +# @connected: true if the PR manager is connected to a backend
> +#
> +# Since: 2.12

3.0?

> +#
> +# Example:
> +#
> +# <- { "event": "PR_MANAGER_STATUS_CHANGED",
> +#      "data": { "id": "/object/pr-helper0",
> +#                "connected": true
> +#      },
> +#      "timestamp": { "seconds": 1519840375, "microseconds": 450486 } }
> +#
> +##
> +{ 'event': 'PR_MANAGER_STATUS_CHANGED',
> +  'data': { 'id': 'str', 'connected': 'bool' } }
> +
>  ##
>  # @QuorumOpType:
>  #
> diff --git a/scsi/pr-manager-helper.c b/scsi/pr-manager-helper.c
> index b11481be9e..6643caf4cf 100644
> --- a/scsi/pr-manager-helper.c
> +++ b/scsi/pr-manager-helper.c
> @@ -17,6 +17,7 @@
>  #include "io/channel.h"
>  #include "io/channel-socket.h"
>  #include "pr-helper.h"
> +#include "qapi/qapi-events-block.h"
>  
>  #include <scsi/sg.h>
>  
> @@ -33,6 +34,7 @@ typedef struct PRManagerHelper {
>      PRManager parent;
>  
>      char *path;
> +    char *qom_path;
>  
>      QemuMutex lock;
>      QIOChannel *ioc;
> @@ -127,6 +129,8 @@ static int pr_manager_helper_initialize(PRManagerHelper *pr_mgr,
>          goto out_close;
>      }
>  
> +    qapi_event_send_pr_manager_status_changed(pr_mgr->qom_path, true,
> +                                              &error_abort);
>      return 0;
>  
>  out_close:
> @@ -142,9 +146,11 @@ static int pr_manager_helper_run(PRManager *p,
>  
>      uint32_t len;
>      PRHelperResponse resp;
> +    int sense_len;
>      int ret;
>      int expected_dir;
>      int attempts;
> +    bool was_connected = pr_mgr->ioc != NULL;
>      uint8_t cdb[PR_HELPER_CDB_SIZE] = { 0 };
>  
>      if (!io_hdr->cmd_len || io_hdr->cmd_len > PR_HELPER_CDB_SIZE) {
> @@ -222,15 +228,19 @@ static int pr_manager_helper_run(PRManager *p,
>          io_hdr->sb_len_wr = MIN(io_hdr->mx_sb_len, PR_HELPER_SENSE_SIZE);
>          memcpy(io_hdr->sbp, resp.sense, io_hdr->sb_len_wr);
>      }
> +    qemu_mutex_unlock(&pr_mgr->lock);
> +    return ret;

Maybe this big function can be refactored in 2, the second being the
bottom code with the mutex locked.

>  
>  out:
> -    if (ret < 0) {
> -        int sense_len = scsi_build_sense(io_hdr->sbp,
> -                                         SENSE_CODE(LUN_COMM_FAILURE));
> -        io_hdr->driver_status = SG_ERR_DRIVER_SENSE;
> -        io_hdr->sb_len_wr = MIN(io_hdr->mx_sb_len, sense_len);
> -        io_hdr->status = CHECK_CONDITION;
> +    if (was_connected) {
> +        qapi_event_send_pr_manager_status_changed(pr_mgr->qom_path, false,
> +                                                  &error_abort);
>      }
> +
> +    sense_len = scsi_build_sense(io_hdr->sbp, SENSE_CODE(LUN_COMM_FAILURE));
> +    io_hdr->driver_status = SG_ERR_DRIVER_SENSE;
> +    io_hdr->sb_len_wr = MIN(io_hdr->mx_sb_len, sense_len);
> +    io_hdr->status = CHECK_CONDITION;
>      qemu_mutex_unlock(&pr_mgr->lock);
>      return ret;
>  }
> @@ -251,6 +261,8 @@ static void pr_manager_helper_complete(UserCreatable *uc, Error **errp)
>  {
>      PRManagerHelper *pr_mgr = PR_MANAGER_HELPER(uc);
>  
> +    pr_mgr->qom_path = object_get_canonical_path(OBJECT(pr_mgr));
> +
>      qemu_mutex_lock(&pr_mgr->lock);
>      pr_manager_helper_initialize(pr_mgr, errp);
>      qemu_mutex_unlock(&pr_mgr->lock);
> @@ -276,6 +288,7 @@ static void pr_manager_helper_instance_finalize(Object *obj)
>      PRManagerHelper *pr_mgr = PR_MANAGER_HELPER(obj);
>  
>      object_unref(OBJECT(pr_mgr->ioc));
> +    g_free(pr_mgr->qom_path);
>      qemu_mutex_destroy(&pr_mgr->lock);
>  }
>  
> 

On 06/26/2018 05:40 PM, Paolo Bonzini wrote:
> Let management know if there were any problems communicating with
> qemu-pr-helper.  The event is edge-triggered, and is sent every time
> the connection status of the pr-manager-helper object changes.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  qapi/block.json          | 24 ++++++++++++++++++++++++
>  scsi/pr-manager-helper.c | 25 +++++++++++++++++++------
>  2 files changed, 43 insertions(+), 6 deletions(-)
> 
> diff --git a/qapi/block.json b/qapi/block.json
> index dc3323c954..1882a8d107 100644
> --- a/qapi/block.json
> +++ b/qapi/block.json
> @@ -334,6 +334,30 @@
>  { 'event': 'DEVICE_TRAY_MOVED',
>    'data': { 'device': 'str', 'id': 'str', 'tray-open': 'bool' } }
>  
> +##
> +# @PR_MANAGER_STATUS_CHANGED:
> +#
> +# Emitted whenever the connected status of a persistent reservation
> +# manager changes.
> +#
> +# @id: The QOM path of the PR manager object
> +#
> +# @connected: true if the PR manager is connected to a backend
> +#
> +# Since: 2.12
> +#
> +# Example:
> +#
> +# <- { "event": "PR_MANAGER_STATUS_CHANGED",
> +#      "data": { "id": "/object/pr-helper0",
> +#                "connected": true
> +#      },
> +#      "timestamp": { "seconds": 1519840375, "microseconds": 450486 } }
> +#
> +##
> +{ 'event': 'PR_MANAGER_STATUS_CHANGED',
> +  'data': { 'id': 'str', 'connected': 'bool' } }
> +
>  ##
>  # @QuorumOpType:
>  #
> diff --git a/scsi/pr-manager-helper.c b/scsi/pr-manager-helper.c
> index b11481be9e..6643caf4cf 100644
> --- a/scsi/pr-manager-helper.c
> +++ b/scsi/pr-manager-helper.c
> @@ -17,6 +17,7 @@
>  #include "io/channel.h"
>  #include "io/channel-socket.h"
>  #include "pr-helper.h"
> +#include "qapi/qapi-events-block.h"
>  
>  #include <scsi/sg.h>
>  
> @@ -33,6 +34,7 @@ typedef struct PRManagerHelper {
>      PRManager parent;
>  
>      char *path;
> +    char *qom_path;
>  
>      QemuMutex lock;
>      QIOChannel *ioc;
> @@ -127,6 +129,8 @@ static int pr_manager_helper_initialize(PRManagerHelper *pr_mgr,
>          goto out_close;
>      }
>  
> +    qapi_event_send_pr_manager_status_changed(pr_mgr->qom_path, true,
> +                                              &error_abort);
>      return 0;
>  
>  out_close:
> @@ -142,9 +146,11 @@ static int pr_manager_helper_run(PRManager *p,
>  
>      uint32_t len;
>      PRHelperResponse resp;
> +    int sense_len;
>      int ret;
>      int expected_dir;
>      int attempts;
> +    bool was_connected = pr_mgr->ioc != NULL;
>      uint8_t cdb[PR_HELPER_CDB_SIZE] = { 0 };
>  
>      if (!io_hdr->cmd_len || io_hdr->cmd_len > PR_HELPER_CDB_SIZE) {
> @@ -222,15 +228,19 @@ static int pr_manager_helper_run(PRManager *p,
>          io_hdr->sb_len_wr = MIN(io_hdr->mx_sb_len, PR_HELPER_SENSE_SIZE);
>          memcpy(io_hdr->sbp, resp.sense, io_hdr->sb_len_wr);
>      }
> +    qemu_mutex_unlock(&pr_mgr->lock);
> +    return ret;
>  
>  out:
> -    if (ret < 0) {
> -        int sense_len = scsi_build_sense(io_hdr->sbp,
> -                                         SENSE_CODE(LUN_COMM_FAILURE));
> -        io_hdr->driver_status = SG_ERR_DRIVER_SENSE;
> -        io_hdr->sb_len_wr = MIN(io_hdr->mx_sb_len, sense_len);
> -        io_hdr->status = CHECK_CONDITION;
> +    if (was_connected) {
> +        qapi_event_send_pr_manager_status_changed(pr_mgr->qom_path, false,
> +                                                  &error_abort);
>      }
> +
> +    sense_len = scsi_build_sense(io_hdr->sbp, SENSE_CODE(LUN_COMM_FAILURE));
> +    io_hdr->driver_status = SG_ERR_DRIVER_SENSE;
> +    io_hdr->sb_len_wr = MIN(io_hdr->mx_sb_len, sense_len);
> +    io_hdr->status = CHECK_CONDITION;
>      qemu_mutex_unlock(&pr_mgr->lock);
>      return ret;
>  }
> @@ -251,6 +261,8 @@ static void pr_manager_helper_complete(UserCreatable *uc, Error **errp)
>  {
>      PRManagerHelper *pr_mgr = PR_MANAGER_HELPER(uc);
>  
> +    pr_mgr->qom_path = object_get_canonical_path(OBJECT(pr_mgr));

Might be worth returning just object_get_canonical_path_component() so
that the event looks like this:

{"event": "PR_MANAGER_STATUS_CHANGED", "data": {"connected": false,
"id": "pr-helper0"}}

(instead of full /object/pr-helper0 path). This corresponds to what
'query-pr-managers' returns too.

Michal

On 06/26/2018 05:40 PM, Paolo Bonzini wrote:
> Let management know if there were any problems communicating with
> qemu-pr-helper.  The event is edge-triggered, and is sent every time
> the connection status of the pr-manager-helper object changes.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  qapi/block.json          | 24 ++++++++++++++++++++++++
>  scsi/pr-manager-helper.c | 25 +++++++++++++++++++------
>  2 files changed, 43 insertions(+), 6 deletions(-)
> 
> diff --git a/qapi/block.json b/qapi/block.json
> index dc3323c954..1882a8d107 100644
> --- a/qapi/block.json
> +++ b/qapi/block.json
> @@ -334,6 +334,30 @@
>  { 'event': 'DEVICE_TRAY_MOVED',
>    'data': { 'device': 'str', 'id': 'str', 'tray-open': 'bool' } }
>  
> +##
> +# @PR_MANAGER_STATUS_CHANGED:
> +#
> +# Emitted whenever the connected status of a persistent reservation
> +# manager changes.
> +#
> +# @id: The QOM path of the PR manager object
> +#
> +# @connected: true if the PR manager is connected to a backend
> +#
> +# Since: 2.12
> +#
> +# Example:
> +#
> +# <- { "event": "PR_MANAGER_STATUS_CHANGED",
> +#      "data": { "id": "/object/pr-helper0",
> +#                "connected": true
> +#      },
> +#      "timestamp": { "seconds": 1519840375, "microseconds": 450486 } }
> +#
> +##
> +{ 'event': 'PR_MANAGER_STATUS_CHANGED',
> +  'data': { 'id': 'str', 'connected': 'bool' } }
> +
>  ##
>  # @QuorumOpType:
>  #
> diff --git a/scsi/pr-manager-helper.c b/scsi/pr-manager-helper.c
> index b11481be9e..6643caf4cf 100644
> --- a/scsi/pr-manager-helper.c
> +++ b/scsi/pr-manager-helper.c
> @@ -17,6 +17,7 @@
>  #include "io/channel.h"
>  #include "io/channel-socket.h"
>  #include "pr-helper.h"
> +#include "qapi/qapi-events-block.h"
>  
>  #include <scsi/sg.h>
>  
> @@ -33,6 +34,7 @@ typedef struct PRManagerHelper {
>      PRManager parent;
>  
>      char *path;
> +    char *qom_path;
>  
>      QemuMutex lock;
>      QIOChannel *ioc;
> @@ -127,6 +129,8 @@ static int pr_manager_helper_initialize(PRManagerHelper *pr_mgr,
>          goto out_close;
>      }
>  
> +    qapi_event_send_pr_manager_status_changed(pr_mgr->qom_path, true,
> +                                              &error_abort);
>      return 0;

Also, can we emit this earlier? For instance at the two places where we
unset pr_mgr->ioc:

diff --git i/scsi/pr-manager-helper.c w/scsi/pr-manager-helper.c
index 6643caf4cf..4ca15dff3f 100644
--- i/scsi/pr-manager-helper.c
+++ w/scsi/pr-manager-helper.c
@@ -48,6 +48,8 @@ static int pr_manager_helper_read(PRManagerHelper *pr_mgr,
 
     if (r < 0) {
         object_unref(OBJECT(pr_mgr->ioc));
+        qapi_event_send_pr_manager_status_changed(pr_mgr->qom_path, false,
+                                                  &error_abort);
         pr_mgr->ioc = NULL;
         return -EINVAL;
     }
@@ -74,6 +76,8 @@ static int pr_manager_helper_write(PRManagerHelper *pr_mgr,
             assert(n_written != QIO_CHANNEL_ERR_BLOCK);
             object_unref(OBJECT(pr_mgr->ioc));
             pr_mgr->ioc = NULL;
+            qapi_event_send_pr_manager_status_changed(pr_mgr->qom_path, false,
+                                                      &error_abort);
             return n_written < 0 ? -EINVAL : 0;
         }
 
@@ -150,7 +154,6 @@ static int pr_manager_helper_run(PRManager *p,
     int ret;
     int expected_dir;
     int attempts;
-    bool was_connected = pr_mgr->ioc != NULL;
     uint8_t cdb[PR_HELPER_CDB_SIZE] = { 0 };
 
     if (!io_hdr->cmd_len || io_hdr->cmd_len > PR_HELPER_CDB_SIZE) {
@@ -232,11 +235,6 @@ static int pr_manager_helper_run(PRManager *p,
     return ret;
 
 out:
-    if (was_connected) {
-        qapi_event_send_pr_manager_status_changed(pr_mgr->qom_path, false,
-                                                  &error_abort);
-    }
-
     sense_len = scsi_build_sense(io_hdr->sbp, SENSE_CODE(LUN_COMM_FAILURE));
     io_hdr->driver_status = SG_ERR_DRIVER_SENSE;
     io_hdr->sb_len_wr = MIN(io_hdr->mx_sb_len, sense_len);
@@ -261,7 +259,7 @@ static void pr_manager_helper_complete(UserCreatable *uc, Error **errp)
 {
     PRManagerHelper *pr_mgr = PR_MANAGER_HELPER(uc);
 
-    pr_mgr->qom_path = object_get_canonical_path(OBJECT(pr_mgr));
+    pr_mgr->qom_path = object_get_canonical_path_component(OBJECT(pr_mgr));
 
     qemu_mutex_lock(&pr_mgr->lock);
     pr_manager_helper_initialize(pr_mgr, errp);


With your approach the event is emitted only after the 5 reconnect 
attempts (which makes little sense IMO). With my approach the event is 
emitted a bit sooner so that reconnect has at least chance of 
succeeding.

I've written some patches for libvirt and with these changes it works 
well.

Michal

On 27/06/2018 12:16, Michal Privoznik wrote:
> Also, can we emit this earlier? For instance at the two places where we
> unset pr_mgr->ioc:
> 
> With your approach the event is emitted only after the 5 reconnect 
> attempts (which makes little sense IMO). With my approach the event is 
> emitted a bit sooner so that reconnect has at least chance of 
> succeeding.

Much better, yes.  I included your change.

Paolo