| Message ID | 1474485630-24113-4-git-send-email-konrad.wilk@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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.
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.
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.
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.
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 )
. 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(-)