Re: [Xen-devel] [PATCH v0 2/3] mem_event: Added new helper API to teardown mem event setup and unmap ring_page.

["Aravindh Puthiyaparambil (aravindp)" <aravindp@xxxxxxxxx>]

>>diff --git a/tools/libxc/xc_mem_access.c b/tools/libxc/xc_mem_access.c
>>index 89050be..ea24689 100644
>>--- a/tools/libxc/xc_mem_access.c
>>+++ b/tools/libxc/xc_mem_access.c
>>@@ -33,12 +33,11 @@ int xc_mem_access_enable(xc_interface *xch,
>domid_t
>>domain_id,
>>                                port, ring_page, back_ring);  }
>>
>>-int xc_mem_access_disable(xc_interface *xch, domid_t domain_id)
>>+int xc_mem_access_disable(xc_interface *xch, domid_t domain_id, void
>>*ring_page)
>> {
>>-    return xc_mem_event_control(xch, domain_id,
>>-                                XEN_DOMCTL_MEM_EVENT_OP_ACCESS_DISABLE,
>>-                                XEN_DOMCTL_MEM_EVENT_OP_ACCESS,
>>-                                NULL);
>>+    return xc_mem_event_teardown(xch, domain_id,
>>+                                HVM_PARAM_ACCESS_RING_PFN,
>>+                                ring_page);
>> }
>
>Coding style. Please line up the parameters after the "(".
>
>> int xc_mem_access_resume(xc_interface *xch, domid_t domain_id) diff
>>--git a/tools/libxc/xc_mem_event.c b/tools/libxc/xc_mem_event.c index
>>3525a83..6cd1894 100644
>>--- a/tools/libxc/xc_mem_event.c
>>+++ b/tools/libxc/xc_mem_event.c
>>@@ -196,3 +196,62 @@ int xc_mem_event_enable(xc_interface *xch,
>domid_t
>>domain_id, int param,
>>
>>     return rc1;
>> }
>>+
>>+/*
>>+ * Teardown mem_event
>>+ * returns 0 on success, if failure returns -errno with errno properly set.
>>+ * param can be HVM_PARAM_PAGING/ACCESS/SHARING_RING_PFN
>>+ */
>
>Same comment as in the previous review. I did think that the norm was to
>return -1 on error and set errno to the appropriate value. I guess a maintainer
>could confirm this.
>
>>+int xc_mem_event_teardown(xc_interface *xch, domid_t domain_id,
>>+                          int param, void *ring_page) {
>>+    int rc;
>>+    unsigned int op, mode;
>>+    switch ( param )
>>+    {
>>+        case HVM_PARAM_PAGING_RING_PFN:
>>+            op = XEN_DOMCTL_MEM_EVENT_OP_PAGING_DISABLE;
>>+            mode = XEN_DOMCTL_MEM_EVENT_OP_PAGING;
>>+            break;
>>+
>>+        case HVM_PARAM_ACCESS_RING_PFN:
>>+            op = XEN_DOMCTL_MEM_EVENT_OP_ACCESS_DISABLE;
>>+            mode = XEN_DOMCTL_MEM_EVENT_OP_ACCESS;
>>+            break;
>>+
>>+        case HVM_PARAM_SHARING_RING_PFN:
>>+            op = XEN_DOMCTL_MEM_EVENT_OP_SHARING_DISABLE;
>>+            mode = XEN_DOMCTL_MEM_EVENT_OP_SHARING;
>>+            break;
>>+
>>+        /*
>>+         * This is for the outside chance that the HVM_PARAM is valid
>>but is invalid
>>+         * as far as mem_event goes.
>>+         */
>>+        default:
>>+            errno = EINVAL;
>>+            rc = -1;
>>+            goto out;
>>+    }
>>+
>>+    /* Remove the ring page. */
>>+    rc = munmap(ring_page, PAGE_SIZE);
>>+    if ( rc < 0 )
>>+        PERROR("Error while disabling paging in xen");
>
>You could say error in munmap instead of a generic statement. And if you are
>going to display a error message here you should add one in the error path of
>xc_mem_event_enable() too.
>
>>+    ring_page = NULL;
>>+
>>+    rc = xc_mem_event_control(xch, domain_id, op, mode, NULL);
>>+    if ( rc != 0 )
>>+    {
>>+        PERROR("Failed to disable mem_event\n");
>>+        goto out;
>
>Why do you need a goto here? You could set rc to -errno in default case of the
>"switch" statement and in the above "if" and just have a "return rc" at the out
>label.
>
>>+    }
>>+
>>+  out:
>>+    if (rc != 0)
>
>Coding style.
>
>>+        rc = -errno;
>>+
>>+    return rc;
>>+}
>>diff --git a/tools/libxc/xc_private.h b/tools/libxc/xc_private.h index
>>cf9b223..7120a08 100644
>>--- a/tools/libxc/xc_private.h
>>+++ b/tools/libxc/xc_private.h
>>@@ -387,4 +387,12 @@ int xc_mem_event_enable(xc_interface *xch,
>domid_t
>>domain_id, int param,
>>                         uint32_t *port, void *ring_page,
>>                         mem_event_back_ring_t *back_ring);
>>
>>+/*
>>+ * Teardown mem_event
>
>Fullstop.
>
>>+ * returns 0 on success, if failure returns -errno with errno properly set.
>
>Please capitalize the "r".
>
>>+ * param can be HVM_PARAM_PAGING/ACCESS/SHARING_RING_PFN
>>+ */
>>+int xc_mem_event_teardown(xc_interface *xch, domid_t domain_id,
>>+                          int param, void *ring_page);
>>+
>
>Why is this called mem_event_teardown()? Won't mem_event_disable() be
>more appropriate?
>
>Thanks,
>Aravindh

Adding xen-devel back as I dropped it accidently.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel