[PATCH] kexec: followup to STATUS patch v3 already in staging

[PATCH] kexec: followup to STATUS patch v3 already in staging

[Eric DeVolder]

This contains the two corrections pointed out by Jan Beulich
for the kexec STATUS call just introduced.

Note: In kexec_status(), the use of test_bit() can also return
EPERM, so the return value from test_bit() must be checked to
ensure that kexec_status() always returns 0, 1 or -1, per the
public header description.

Note: My handling of the test_bit() scenario is to explicitly
check for return value of 1, so any value other than 1 causes
kexec_status to return 0.


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

[PATCH 1/2] Put back blank line for readability purposes.

[Eric DeVolder]

This blank line was accidentally removed during
the insertion of the kexec_status() declarations.

Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
---
 xen/include/public/kexec.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/include/public/kexec.h b/xen/include/public/kexec.h
index c200e8c..74ea981 100644
--- a/xen/include/public/kexec.h
+++ b/xen/include/public/kexec.h
@@ -240,6 +240,7 @@ typedef struct xen_kexec_status {
     uint8_t type;
 } xen_kexec_status_t;
 DEFINE_XEN_GUEST_HANDLE(xen_kexec_status_t);
+
 #else /* __XEN_INTERFACE_VERSION__ < 0x00040400 */
 
 #define KEXEC_CMD_kexec_load KEXEC_CMD_kexec_load_v1
-- 
2.7.4


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

Re: [PATCH 1/2] Put back blank line for readability purposes.

[Daniel Kiper]

On Wed, Jan 18, 2017 at 03:47:27PM -0600, Eric DeVolder wrote:
> This blank line was accidentally removed during
> the insertion of the kexec_status() declarations.
>
> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

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

[PATCH 2/2] kexec: ensure kexec_status() always returns 0 or 1

[Eric DeVolder]

The use of test_bit() can also return EPERM, so the
return value from test_bit() must be checked to
ensure that kexec_status() always returns 0, 1 or
-1, per the public header description.

Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
---
 xen/common/kexec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/common/kexec.c b/xen/common/kexec.c
index aa808cb..40b76d5 100644
--- a/xen/common/kexec.c
+++ b/xen/common/kexec.c
@@ -1182,7 +1182,7 @@ static int kexec_status(XEN_GUEST_HANDLE_PARAM(void) uarg)
     if ( kexec_load_get_bits(status.type, &base, &bit) )
         return -EINVAL;
 
-    return test_bit(bit, &kexec_flags);
+    return (test_bit(bit, &kexec_flags) == 1);
 }
 
 static int do_kexec_op_internal(unsigned long op,
-- 
2.7.4


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

Re: [PATCH 2/2] kexec: ensure kexec_status() always returns 0 or 1

[Daniel Kiper]

On Wed, Jan 18, 2017 at 03:47:28PM -0600, Eric DeVolder wrote:
> The use of test_bit() can also return EPERM, so the
> return value from test_bit() must be checked to
> ensure that kexec_status() always returns 0, 1 or
> -1, per the public header description.
>
> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

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

Re: [PATCH 2/2] kexec: ensure kexec_status() always returns 0 or 1

[Jan Beulich]

>>> On 18.01.17 at 22:47, <eric.devolder@oracle.com> wrote:
> The use of test_bit() can also return EPERM, so the
> return value from test_bit() must be checked to
> ensure that kexec_status() always returns 0, 1 or
> -1, per the public header description.

Well, no, and this is rather disappointing. Did you look at the test_bit()
implementation, as I did suggest you do? As said before, it returns
non-zero if the bit was set, and zero if it was clear. It won't ever
return -EPERM (it was only your original code which converted the
meaning of ~0 to -EPERM). Hence the result here isn't much better
than the original.

Jan


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

Re: [PATCH 2/2] kexec: ensure kexec_status() always returns 0 or 1

[Daniel Kiper]

On Thu, Jan 19, 2017 at 01:07:52AM -0700, Jan Beulich wrote:
> >>> On 18.01.17 at 22:47, <eric.devolder@oracle.com> wrote:
> > The use of test_bit() can also return EPERM, so the
> > return value from test_bit() must be checked to
> > ensure that kexec_status() always returns 0, 1 or
> > -1, per the public header description.
>
> Well, no, and this is rather disappointing. Did you look at the test_bit()
> implementation, as I did suggest you do? As said before, it returns
> non-zero if the bit was set, and zero if it was clear. It won't ever
> return -EPERM (it was only your original code which converted the
> meaning of ~0 to -EPERM). Hence the result here isn't much better
> than the original.

Errr... It looks that I have checked incorrect test_bit() implementation
and my Reviewed-by was skewed. Sorry about that. Jan is right. You must
use "!!" instead of "... == 1". And commit message is also incorrect...

Daniel

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

Re: [PATCH] kexec: followup to STATUS patch v3 already in staging

[Daniel Kiper]

On Wed, Jan 18, 2017 at 03:47:26PM -0600, Eric DeVolder wrote:
> This contains the two corrections pointed out by Jan Beulich
> for the kexec STATUS call just introduced.
>
> Note: In kexec_status(), the use of test_bit() can also return
> EPERM, so the return value from test_bit() must be checked to
> ensure that kexec_status() always returns 0, 1 or -1, per the
> public header description.
>
> Note: My handling of the test_bit() scenario is to explicitly
> check for return value of 1, so any value other than 1 causes
> kexec_status to return 0.

One nitpick. Subject should be:

[PATCH v2 0/2] kexec: followup to STATUS patch v3 already in staging

and then following patches should have in subject:

[PATCH v2 1/2] ...
[PATCH v2 2/2] ...

However, IMO regardless of that patches can go in.

Daniel

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