[PATCH v1] xen-livepatch fixes for Xen 4.8

[PATCH v1] xen-livepatch fixes for Xen 4.8

[Konrad Rzeszutek Wilk]

Hey!

Three tiny fixes that came about from various reports from
folks.

 tools/misc/xen-livepatch.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

Konrad Rzeszutek Wilk (3):
      xen-livepatch: Remove the 'test' part
      xen-livepatch: Print the header _after_ the first livepatch hypercall
      xen-livepatch: If hypervisor is not compiled with Livepatching


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

[PATCH v1 1/3] xen-livepatch: Remove the 'test' part

[Konrad Rzeszutek Wilk]

As it has evolved a bit and is more of a test tool.

Reported-by: Bhavesh Davda <bhavesh.davda@oracle.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/misc/xen-livepatch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/misc/xen-livepatch.c b/tools/misc/xen-livepatch.c
index 62c072e..7512a98 100644
--- a/tools/misc/xen-livepatch.c
+++ b/tools/misc/xen-livepatch.c
@@ -20,7 +20,7 @@ static xc_interface *xch;
 void show_help(void)
 {
     fprintf(stderr,
-            "xen-livepatch: live patching test tool\n"
+            "xen-livepatch: live patching tool\n"
             "Usage: xen-livepatch <command> [args]\n"
             " <name> An unique name of payload. Up to %d characters.\n"
             "Commands:\n"
-- 
2.4.11


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

[PATCH v1 2/3] xen-livepatch: Print the header _after_ the first livepatch hypercall

[Konrad Rzeszutek Wilk]

That way we can print out the header if we are sure the
hypervisor has been compiled with Xen Livepatching.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/misc/xen-livepatch.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/misc/xen-livepatch.c b/tools/misc/xen-livepatch.c
index 7512a98..2de04c0 100644
--- a/tools/misc/xen-livepatch.c
+++ b/tools/misc/xen-livepatch.c
@@ -91,8 +91,6 @@ static int list_func(int argc, char *argv[])
         return rc;
     }
 
-    fprintf(stdout," ID                                     | status\n"
-                   "----------------------------------------+------------\n");
     do {
         done = 0;
         /* The memset is done to catch errors. */
@@ -106,6 +104,10 @@ static int list_func(int argc, char *argv[])
                     idx, left, errno, strerror(errno));
             break;
         }
+        if ( !idx )
+            fprintf(stdout," ID                                     | status\n"
+                           "----------------------------------------+------------\n");
+
         for ( i = 0; i < done; i++ )
         {
             unsigned int j;
-- 
2.4.11


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

[PATCH v1 3/3] xen-livepatch: If hypervisor is not compiled with Livepatching

[Konrad Rzeszutek Wilk]

. print a better error code.

Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/misc/xen-livepatch.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/misc/xen-livepatch.c b/tools/misc/xen-livepatch.c
index 2de04c0..f11e71f07f 100644
--- a/tools/misc/xen-livepatch.c
+++ b/tools/misc/xen-livepatch.c
@@ -100,8 +100,11 @@ static int list_func(int argc, char *argv[])
         rc = xc_livepatch_list(xch, MAX_LEN, idx, info, name, len, &done, &left);
         if ( rc )
         {
-            fprintf(stderr, "Failed to list %d/%d: %d(%s)!\n",
-                    idx, left, errno, strerror(errno));
+            if ( errno == ENOSYS )
+                fprintf(stderr, "Hypervisor compiled without Xen Livepatching!\n");
+            else
+                fprintf(stderr, "Failed to list %d/%d: %d(%s)!\n",
+                        idx, left, errno, strerror(errno));
             break;
         }
         if ( !idx )
-- 
2.4.11


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

Re: [PATCH v1 1/3] xen-livepatch: Remove the 'test' part

[Ross Lagerwall]

On 09/21/2016 08:20 PM, Konrad Rzeszutek Wilk wrote:
> As it has evolved a bit and is more of a test tool.
>
> Reported-by: Bhavesh Davda <bhavesh.davda@oracle.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  tools/misc/xen-livepatch.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/misc/xen-livepatch.c b/tools/misc/xen-livepatch.c
> index 62c072e..7512a98 100644
> --- a/tools/misc/xen-livepatch.c
> +++ b/tools/misc/xen-livepatch.c
> @@ -20,7 +20,7 @@ static xc_interface *xch;
>  void show_help(void)
>  {
>      fprintf(stderr,
> -            "xen-livepatch: live patching test tool\n"
> +            "xen-livepatch: live patching tool\n"
>              "Usage: xen-livepatch <command> [args]\n"
>              " <name> An unique name of payload. Up to %d characters.\n"
>              "Commands:\n"
>

Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>

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

Re: [PATCH v1 2/3] xen-livepatch: Print the header _after_ the first livepatch hypercall

[Ross Lagerwall]

On 09/21/2016 08:20 PM, Konrad Rzeszutek Wilk wrote:
> That way we can print out the header if we are sure the
> hypervisor has been compiled with Xen Livepatching.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>

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

Re: [PATCH v1 3/3] xen-livepatch: If hypervisor is not compiled with Livepatching

[Ross Lagerwall]

On 09/21/2016 08:20 PM, Konrad Rzeszutek Wilk wrote:
> . print a better error code.
>
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  tools/misc/xen-livepatch.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/tools/misc/xen-livepatch.c b/tools/misc/xen-livepatch.c
> index 2de04c0..f11e71f07f 100644
> --- a/tools/misc/xen-livepatch.c
> +++ b/tools/misc/xen-livepatch.c
> @@ -100,8 +100,11 @@ static int list_func(int argc, char *argv[])
>          rc = xc_livepatch_list(xch, MAX_LEN, idx, info, name, len, &done, &left);
>          if ( rc )
>          {
> -            fprintf(stderr, "Failed to list %d/%d: %d(%s)!\n",
> -                    idx, left, errno, strerror(errno));
> +            if ( errno == ENOSYS )
> +                fprintf(stderr, "Hypervisor compiled without Xen Livepatching!\n");
> +            else
> +                fprintf(stderr, "Failed to list %d/%d: %d(%s)!\n",
> +                        idx, left, errno, strerror(errno));
>              break;
>          }
>          if ( !idx )
>

You should save errno since you have an fprintf before you check it.

Also, it would be good to do the same check for the first 
xc_livepatch_get in action_func() and for the xc_livepatch_upload in 
upload_func.

-- 
Ross Lagerwall

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

Re: [PATCH v1 3/3] xen-livepatch: If hypervisor is not compiled with Livepatching

[Konrad Rzeszutek Wilk]

On Thu, Sep 22, 2016 at 08:25:33AM +0100, Ross Lagerwall wrote:
> On 09/21/2016 08:20 PM, Konrad Rzeszutek Wilk wrote:
> > . print a better error code.
> > 
> > Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > ---
> >  tools/misc/xen-livepatch.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/misc/xen-livepatch.c b/tools/misc/xen-livepatch.c
> > index 2de04c0..f11e71f07f 100644
> > --- a/tools/misc/xen-livepatch.c
> > +++ b/tools/misc/xen-livepatch.c
> > @@ -100,8 +100,11 @@ static int list_func(int argc, char *argv[])
> >          rc = xc_livepatch_list(xch, MAX_LEN, idx, info, name, len, &done, &left);
> >          if ( rc )
> >          {
> > -            fprintf(stderr, "Failed to list %d/%d: %d(%s)!\n",
> > -                    idx, left, errno, strerror(errno));
> > +            if ( errno == ENOSYS )
> > +                fprintf(stderr, "Hypervisor compiled without Xen Livepatching!\n");
> > +            else
> > +                fprintf(stderr, "Failed to list %d/%d: %d(%s)!\n",
> > +                        idx, left, errno, strerror(errno));
> >              break;
> >          }
> >          if ( !idx )
> > 
> 
> You should save errno since you have an fprintf before you check it.

I am missing something.

Why would fprintf mess up 'errno' ?
> 
> Also, it would be good to do the same check for the first xc_livepatch_get
> in action_func() and for the xc_livepatch_upload in upload_func.

<nods>
> 
> -- 
> Ross Lagerwall

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

Re: [PATCH v1 3/3] xen-livepatch: If hypervisor is not compiled with Livepatching

[Ian Jackson]

Konrad Rzeszutek Wilk writes ("Re: [PATCH v1 3/3] xen-livepatch: If hypervisor is not compiled with Livepatching"):
> On Thu, Sep 22, 2016 at 08:25:33AM +0100, Ross Lagerwall wrote:
> > You should save errno since you have an fprintf before you check it.
> 
> I am missing something.
> 
> Why would fprintf mess up 'errno' ?

Any libc function is entitled to set errno, if it fails.

Ian.

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

Re: [PATCH v1 3/3] xen-livepatch: If hypervisor is not compiled with Livepatching

[Ian Jackson]

Ian Jackson writes ("Re: [PATCH v1 3/3] xen-livepatch: If hypervisor is not compiled with Livepatching"):
> Konrad Rzeszutek Wilk writes ("Re: [PATCH v1 3/3] xen-livepatch: If hypervisor is not compiled with Livepatching"):
> > Why would fprintf mess up 'errno' ?
> 
> Any libc function is entitled to set errno, if it fails.

And, I forget the exact rules, but I think most are allowed to trash
it even if they succeed.  I just remember that errno needs to be saved
across libc calls, if the value is important.

In practice, for example, the first fprintf on a particular FILE* can
end up setting errno to ENOTTY as the libc calls isatty() to decide on
the buffering mode.

Ian.

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

Re: [PATCH v1 3/3] xen-livepatch: If hypervisor is not compiled with Livepatching

[Ross Lagerwall]

On 09/22/2016 11:12 AM, Konrad Rzeszutek Wilk wrote:
> On Thu, Sep 22, 2016 at 08:25:33AM +0100, Ross Lagerwall wrote:
>> On 09/21/2016 08:20 PM, Konrad Rzeszutek Wilk wrote:
>>> . print a better error code.
>>>
>>> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>> ---
>>>  tools/misc/xen-livepatch.c | 7 +++++--
>>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tools/misc/xen-livepatch.c b/tools/misc/xen-livepatch.c
>>> index 2de04c0..f11e71f07f 100644
>>> --- a/tools/misc/xen-livepatch.c
>>> +++ b/tools/misc/xen-livepatch.c
>>> @@ -100,8 +100,11 @@ static int list_func(int argc, char *argv[])
>>>          rc = xc_livepatch_list(xch, MAX_LEN, idx, info, name, len, &done, &left);
>>>          if ( rc )
>>>          {
>>> -            fprintf(stderr, "Failed to list %d/%d: %d(%s)!\n",
>>> -                    idx, left, errno, strerror(errno));
>>> +            if ( errno == ENOSYS )
>>> +                fprintf(stderr, "Hypervisor compiled without Xen Livepatching!\n");
>>> +            else
>>> +                fprintf(stderr, "Failed to list %d/%d: %d(%s)!\n",
>>> +                        idx, left, errno, strerror(errno));
>>>              break;
>>>          }
>>>          if ( !idx )
>>>
>>
>> You should save errno since you have an fprintf before you check it.
>
> I am missing something.
>
> Why would fprintf mess up 'errno' ?
>>

If the first fprintf fails for whatever reason (e.g. ENOSPC), then it 
will set errno and the subsequent check of errno afterwards will serve 
no purpose.

I have a patch to fix this issue for the rest of xen-livepatch.c, and 
will send it out shortly.

-- 
Ross Lagerwall

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

Re: [PATCH v1 3/3] xen-livepatch: If hypervisor is not compiled with Livepatching

[Konrad Rzeszutek Wilk]

On Thu, Sep 22, 2016 at 11:36:40AM +0100, Ian Jackson wrote:
> Ian Jackson writes ("Re: [PATCH v1 3/3] xen-livepatch: If hypervisor is not compiled with Livepatching"):
> > Konrad Rzeszutek Wilk writes ("Re: [PATCH v1 3/3] xen-livepatch: If hypervisor is not compiled with Livepatching"):
> > > Why would fprintf mess up 'errno' ?
> > 
> > Any libc function is entitled to set errno, if it fails.
> 
> And, I forget the exact rules, but I think most are allowed to trash
> it even if they succeed.  I just remember that errno needs to be saved
> across libc calls, if the value is important.
> 
> In practice, for example, the first fprintf on a particular FILE* can
> end up setting errno to ENOTTY as the libc calls isatty() to decide on
> the buffering mode.

Aaah! Thanks!
> 
> Ian.

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