| Message ID | 1591980255-18811-1-git-send-email-igor.druzhinin@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tools/xen-ucode: fix error code propagation of microcode load operation | expand |
Igor Druzhinin writes ("[PATCH] tools/xen-ucode: fix error code propagation of microcode load operation"):
> Otherwise it's impossible to know the reason for a fault or blob rejection
> inside the automation.
...
> fprintf(stderr, "Failed to update microcode. (err: %s)\n",
> strerror(errno));
This part is fine.
> + ret = errno;
> xc_interface_close(xch);
...
> }
> close(fd);
>
> - return 0;
> + return ret;
Unfortunately I don't think this is right. errno might not fit into a
return value. Returning nonzero on microcode loading error would
definitely be right, but ...
... oh I have just read the rest of this file.
I think what is missing here is simply `return errno' (and the braces)
There is no need to call xc_interface_close, or munmap, if we are
about to exit.
I think fixing the lost error return is 4.14 material, so I have
added that to the subject line.
Paul, would you Release-ack a patch that replaced every `return errno'
with (say) exit(12) ? Otherwise, fixing this program not to try to
fit errno into an exit status is future work. Also I notice that the
program exits 0 if invoked wrongly. Unhelpful! I would want to fix
that too.
Ian.
> -----Original Message----- > From: Ian Jackson <ian.jackson@citrix.com> > Sent: 12 June 2020 17:54 > To: Igor Druzhinin <igor.druzhinin@citrix.com> > Cc: xen-devel@lists.xenproject.org; Andrew Cooper <Andrew.Cooper3@citrix.com>; wl@xen.org; Paul > Durrant <xadimgnik@gmail.com> > Subject: Re: [XEN PATCH for-4.14] tools/xen-ucode: fix error code propagation of microcode load > operation > > Igor Druzhinin writes ("[PATCH] tools/xen-ucode: fix error code propagation of microcode load > operation"): > > Otherwise it's impossible to know the reason for a fault or blob rejection > > inside the automation. > ... > > fprintf(stderr, "Failed to update microcode. (err: %s)\n", > > strerror(errno)); > > This part is fine. > > > + ret = errno; > > xc_interface_close(xch); > ... > > } > > close(fd); > > > > - return 0; > > + return ret; > > Unfortunately I don't think this is right. errno might not fit into a > return value. I don't understand this part. Why would errno not fit? > Returning nonzero on microcode loading error would > definitely be right, but ... > > ... oh I have just read the rest of this file. > > I think what is missing here is simply `return errno' (and the braces) > There is no need to call xc_interface_close, or munmap, if we are > about to exit. > > I think fixing the lost error return is 4.14 material, so I have > added that to the subject line. > > Paul, would you Release-ack a patch that replaced every `return errno' > with (say) exit(12) ? Why 12? > Otherwise, fixing this program not to try to > fit errno into an exit status is future work. Also I notice that the > program exits 0 if invoked wrongly. Unhelpful! I would want to fix > that too. Agreed that is unhelpful. I'm happy in principle to release-ack something replacing the returns with exits. Paul > > Ian.
On 12/06/2020 17:53, Ian Jackson wrote: > Igor Druzhinin writes ("[PATCH] tools/xen-ucode: fix error code propagation of microcode load operation"): >> Otherwise it's impossible to know the reason for a fault or blob rejection >> inside the automation. > ... >> fprintf(stderr, "Failed to update microcode. (err: %s)\n", >> strerror(errno)); > > This part is fine. > >> + ret = errno; >> xc_interface_close(xch); > ... >> } >> close(fd); >> >> - return 0; >> + return ret; > > Unfortunately I don't think this is right. errno might not fit into a > return value. errno codes that Xen return are all lower than 127. It fits perfectly fine. > Returning nonzero on microcode loading error would > definitely be right, but ... > > ... oh I have just read the rest of this file. > > I think what is missing here is simply `return errno' (and the braces) > There is no need to call xc_interface_close, or munmap, if we are > about to exit. Probably but that is identical to what was suggested. Cleaning resource is just a nice thing to do although unnecessary. Can change to just "return errno" if that's what you'd prefer. > I think fixing the lost error return is 4.14 material, so I have > added that to the subject line. > > Paul, would you Release-ack a patch that replaced every `return errno' > with (say) exit(12) ? That would again conceal real return code from automation. Igor
Paul Durrant writes ("RE: [XEN PATCH for-4.14] tools/xen-ucode: fix error code propagation of microcode load operation"):
> > -----Original Message-----
> > From: Ian Jackson <ian.jackson@citrix.com>
> > Paul, would you Release-ack a patch that replaced every `return errno'
> > with (say) exit(12) ?
>
> Why 12?
I tend to use 12 to mean `things went wrong'. 1 is a poor choice for
this because you want to use smaller numbers for less severe errors
and you want some space for things like "everything went OK but the
thing you asked me to delete already didn't exist" or "I compared
these files like you asked, and they are different".
> > Otherwise, fixing this program not to try to
> > fit errno into an exit status is future work. Also I notice that the
> > program exits 0 if invoked wrongly. Unhelpful! I would want to fix
> > that too.
>
> Agreed that is unhelpful. I'm happy in principle to release-ack
> something replacing the returns with exits.
Thanks,
Ian.
Igor Druzhinin writes ("Re: [XEN PATCH for-4.14] tools/xen-ucode: fix error code propagation of microcode load operation"):
> On 12/06/2020 17:53, Ian Jackson wrote:
> > Unfortunately I don't think this is right. errno might not fit into a
> > return value.
>
> errno codes that Xen return are all lower than 127. It fits perfectly fine.
I don't think this is a stable aspect of Xen's ABI, is it ? And of
course what you get from libxc is not a Xen errno in Xen encoding, but
a Xen errno in host errno encoding, whioch might be >=127 even if the
Xen number for the same EFOOBAR is <=127.
But anyway, if this is controversial I will drop it.
> Probably but that is identical to what was suggested.
> Cleaning resource is just a nice thing to do although unnecessary.
> Can change to just "return errno" if that's what you'd prefer.
Yes please.
Would you care to at least arrange for bad usage to exit nonzero ?
I will leave it up to you to resolve this quandry: your view is that
this program's exit status is a Xen errno value (in host encoding,
presumably, although you don't state this) but now you need to return
an error that didn't come from Xen.
Ian.
diff --git a/tools/misc/xen-ucode.c b/tools/misc/xen-ucode.c index 0c257f4..2c907e1 100644 --- a/tools/misc/xen-ucode.c +++ b/tools/misc/xen-ucode.c @@ -62,8 +62,11 @@ int main(int argc, char *argv[]) ret = xc_microcode_update(xch, buf, len); if ( ret ) + { + ret = errno; fprintf(stderr, "Failed to update microcode. (err: %s)\n", strerror(errno)); + } xc_interface_close(xch); @@ -74,5 +77,5 @@ int main(int argc, char *argv[]) } close(fd); - return 0; + return ret; }
Otherwise it's impossible to know the reason for a fault or blob rejection inside the automation. Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com> --- tools/misc/xen-ucode.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)