John Ferlan <jferlan@xxxxxxxxxx> [2018-01-08, 07:55AM -0500]:
>
>
> On 01/08/2018 03:39 AM, Bjoern Walk wrote:
> > John Ferlan <jferlan@xxxxxxxxxx> [2018-01-04, 03:56PM -0500]:
> >> On 12/19/2017 05:08 AM, Bjoern Walk wrote:
> >>> diff --git a/src/util/virsysinfo.c b/src/util/virsysinfo.c
> >>> index ab81b1f7..0c2267e3 100644
> >>> --- a/src/util/virsysinfo.c
> >>> +++ b/src/util/virsysinfo.c
> >>> @@ -495,6 +495,7 @@ virSysinfoParseS390Processor(const char *base, virSysinfoDefPtr ret)
> >>> char *tmp_base;
> >>> char *manufacturer = NULL;
> >>> char *procline = NULL;
> >>> + char *ncpu = NULL;
> >>> int result = -1;
> >>> virSysinfoProcessorDefPtr processor;
> >>>
> >>> @@ -524,11 +525,41 @@ virSysinfoParseS390Processor(const char *base, virSysinfoDefPtr ret)
> >>>
> >>> VIR_FREE(procline);
> >>> }
> >>> +
> >>> + /* now, for each processor found, extract the frequency information */
> >>> + tmp_base = (char *) base;
> >>> +
> >>> + while ((tmp_base = strstr(tmp_base, "cpu number")) &&
> >>> + (tmp_base = virSysinfoParseS390Line(tmp_base, "cpu number", &ncpu))) {
> >>> + unsigned int n;
> >>> + char *mhz = NULL;
> >>> +
> >>> + if (virStrToLong_ui(ncpu, NULL, 10, &n) < 0 || n >= ret->nprocessor)
> >>> + goto cleanup;
> >>
> >> Should these be split? Reason I ask is if n >= ret->nprocessor, then
> >> going to cleanup results in returning a failure. That leads to an
> >> eventual generic command failed for some reason. Of course that reason
> >> shouldn't be possible, but since this is a CYA exercise, the check
> >> should have a specific error message - similar to what one would get if
> >> other calls failed...
> >
> > I don't quite follow. You want an explicit error message here if n >=
> > ret->nprocessor? Right now, for this call sequence there is no error
> > reporting at all. This just fills the respective driver->hostsysinfo
> > struct and sets this to NULL in case of an error. Later on, when the
> > hostsysinfo is used and not available, an error is generated.
> >
>
> When I first read, I read it "out of context"... If the first call
> fails, then you get an error, if the second call fails, then there is no
> error so the first question that pops in my mind is - should we provide
> an error message in that case?
In general, yes, we should provide some information about what's
happening. But I found all this code to be somewhat unsatisfactory in
this regard and wanted to avoid doing to many changes in this series.
> I would hope that n >= ret->nprocessor couldn't be true considering what
> was recently read a few lines earlier and if the number was wrong here,
> then the cpuinfo output would be AFU'd, right? I don't have picture in
> my mind of what's being processed, but didn't want to ignore this
> possible condition.
>
> Since @result would be -1, then going to cleanup at this point would be
> a failure. The question thus becomes should we ignore (and return 0)
> when cpuinfo is bad and maybe toss out a VIR_DEBUG or should we error
> out and throw everything away?
Alright, this sounds good. I agree that we should try to gather as much
information as possible and don't discard everything when one part
fails. But again, this whole file has somewhat different semantics.
> The one thing with waiting for some caller to determine hostsysinfo is
> not available and an error generated is that we lose the reason why. I
> defer to you to decide what the best course of action is.
>
> >>> +
> >>> + if (!(tmp_base = strstr(tmp_base, "cpu MHz dynamic")) ||
> >>> + !virSysinfoParseS390Line(tmp_base, "cpu MHz dynamic", &mhz) ||
> >>> + !mhz)
> >>> + goto cleanup;
Now, what about those? Should we also just log and return 0 here? CPU
frequency information has only recently been introduced on S390 and
running this on a kernel without support for it would now discard
hostsysinfo entirely.
--
IBM Systems
Linux on z Systems & Virtualization Development
------------------------------------------------------------------------
IBM Deutschland
Schönaicher Str. 220
71032 Böblingen
Phone: +49 7031 16 1819
E-Mail: bwalk@xxxxxxxxxx
------------------------------------------------------------------------
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
