[PATCH 0/8] Fixes to Coverity issues reported on xenalyze

[PATCH 0/8] Fixes to Coverity issues reported on xenalyze

[George Dunlap]

This series fixes a number of issues to coverity with xenalyze.

George Dunlap (8):
  tools/xenalyze: Close symbol_file after reading it
  tools/xenalyze: Avoid redundant check
  tools/xenalyze: Handle fstat errors properly
  tools/xenalyze: Mark unreachable code as unreachable
  tools/xenalyze: Fix check for error return value
  tools/xenalyze: Fix off-by-one in MAX_CPUS range checks
  tools/xenalyze: Fix multiple instances of *HYPERCALL_MAX
  tools/xenalyze: Actually handle case where number of ipi vectors
    exceeds static max

 tools/xentrace/mread.c    |  9 ++++++++-
 tools/xentrace/xenalyze.c | 46 ++++++++++++++++++++++++++++------------------
 2 files changed, 36 insertions(+), 19 deletions(-)

---
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>


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

[PATCH 1/8] tools/xenalyze: Close symbol_file after reading it

[George Dunlap]

...to avoid leaking the FD and associated memory.

CID 1306872

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
Not sure if this will actually make coverity happy, or if we need to
actually set symbol_file to NULL.

CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/xentrace/xenalyze.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/xentrace/xenalyze.c b/tools/xentrace/xenalyze.c
index 1651302..3c90a0f 100644
--- a/tools/xentrace/xenalyze.c
+++ b/tools/xentrace/xenalyze.c
@@ -363,6 +363,8 @@ void parse_symbol_file(char *fn) {
             p=&((*p)->next);
         }
     }
+
+    fclose(symbol_file);
 }
 
 /* WARNING not thread safe */
-- 
2.1.4


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

[PATCH 2/8] tools/xenalyze: Avoid redundant check

[George Dunlap]

Coverity notices that if !head is true, that !N can never be true.

Don't bother checking N, since we know it has to be at least one.

CID 1354243

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/xentrace/xenalyze.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/tools/xentrace/xenalyze.c b/tools/xentrace/xenalyze.c
index 3c90a0f..33f8129 100644
--- a/tools/xentrace/xenalyze.c
+++ b/tools/xentrace/xenalyze.c
@@ -4146,9 +4146,6 @@ void cr3_dump_list(struct cr3_value_struct *head){
     for(p=head; p; p=p->next)
         N++;
 
-    if(!N)
-        return;
-
     /* Alloc a struct of the right size */
     qsort_array = malloc(N * sizeof(struct eip_list_struct *));
 
-- 
2.1.4


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

[PATCH 3/8] tools/xenalyze: Handle fstat errors properly

[George Dunlap]

They're pretty unlikely to fail, but doesn't hurt to check.

CID 1311502
CID 1311501

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/xentrace/mread.c    | 9 ++++++++-
 tools/xentrace/xenalyze.c | 5 ++++-
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/tools/xentrace/mread.c b/tools/xentrace/mread.c
index a22c4ea..c6f6ec6 100644
--- a/tools/xentrace/mread.c
+++ b/tools/xentrace/mread.c
@@ -24,9 +24,16 @@ mread_handle_t mread_init(int fd)
 
     h->fd = fd;
 
-    fstat(fd, &s);
+    if ( fstat(fd, &s) ) {
+        perror("fstat");
+        free(h);
+        h = NULL;
+        goto out;
+    }
+    
     h->file_size = s.st_size;
 
+out:
     return h;
 }
 
diff --git a/tools/xentrace/xenalyze.c b/tools/xentrace/xenalyze.c
index 33f8129..9f8c065 100644
--- a/tools/xentrace/xenalyze.c
+++ b/tools/xentrace/xenalyze.c
@@ -10348,7 +10348,10 @@ int main(int argc, char *argv[]) {
         error(ERR_SYSTEM, NULL);
     } else {
         struct stat s;
-        fstat(G.fd, &s);
+        if ( fstat(G.fd, &s) ) {
+            perror("fstat");
+            error(ERR_SYSTEM, NULL);
+        }
         G.file_size = s.st_size;
     }
 
-- 
2.1.4


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

[PATCH 4/8] tools/xenalyze: Mark unreachable code as unreachable

[George Dunlap]

...so that coverity knows it's unreachable.

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/xentrace/xenalyze.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/xentrace/xenalyze.c b/tools/xentrace/xenalyze.c
index 9f8c065..123030a 100644
--- a/tools/xentrace/xenalyze.c
+++ b/tools/xentrace/xenalyze.c
@@ -7306,6 +7306,7 @@ void sched_runstate_process(struct pcpu_info *p)
             }
             goto update;
         }
+        __builtin_unreachable();
         fprintf(stderr, "FATAL: Logic hole in %s\n", __func__);
         error(ERR_ASSERT, NULL);
     }
-- 
2.1.4


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

[PATCH 5/8] tools/xenalyze: Fix check for error return value

[George Dunlap]

fdopen returns NULL on failure, not a negative integer.

CID 1306863
CID 1306858

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
Strangely, coverity counted this as two bugs: one for comparing a
pointer with a negative integer, one for the comparison never possibly
being true (thus making the if condition dead code).

CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/xentrace/xenalyze.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/xentrace/xenalyze.c b/tools/xentrace/xenalyze.c
index 123030a..249bebd 100644
--- a/tools/xentrace/xenalyze.c
+++ b/tools/xentrace/xenalyze.c
@@ -9029,7 +9029,7 @@ void progress_init(void) {
         opt.progress = 0;
     }
 
-    if( (G.progress.out = fdopen(G.progress.pipe[1], "w")) < 0 ) {
+    if( (G.progress.out = fdopen(G.progress.pipe[1], "w")) == NULL ) {
         fprintf(stderr, "%s: could not fdopen pipe: %s, disabling progress bar\n",
                 __func__, strerror(errno));
         opt.progress = 0;
-- 
2.1.4


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

[PATCH 6/8] tools/xenalyze: Fix off-by-one in MAX_CPUS range checks

[George Dunlap]

Skip action / throw error if cpu/vcpu >= MAX_CPUS  rather than >.

Also add an assertion to vcpu_find, to make future errors of this kind
not out-of-bounds.

CID 1306871
CID 1306870
CID 1306869
CID 1306867

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/xentrace/xenalyze.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/tools/xentrace/xenalyze.c b/tools/xentrace/xenalyze.c
index 249bebd..3e26a4c 100644
--- a/tools/xentrace/xenalyze.c
+++ b/tools/xentrace/xenalyze.c
@@ -6860,6 +6860,13 @@ struct vcpu_data * vcpu_find(int did, int vid)
     struct domain_data *d;
     struct vcpu_data *v;
 
+    /* "Graceful" handling of vid >= MAX_CPUS should be handled elsewhere */
+    if ( vid >= MAX_CPUS ) {
+        fprintf(stderr, "%s: vcpu %d exceeds MAX_CPUS %d!\n",
+                __func__, vid, MAX_CPUS);
+        error(ERR_ASSERT, NULL);
+    }
+
     d = domain_find(did);
 
     v = d->vcpu[vid];
@@ -7131,7 +7138,7 @@ void sched_runstate_process(struct pcpu_info *p)
         }
     }
 
-    if(r->vcpu > MAX_CPUS)
+    if(r->vcpu >= MAX_CPUS)
     {
         fprintf(warn, "%s: vcpu %u > MAX_VCPUS %d!\n",
                 __func__, r->vcpu, MAX_CPUS);
@@ -7441,14 +7448,14 @@ void sched_switch_process(struct pcpu_info *p)
                r->prev_dom, r->prev_vcpu,
                r->next_dom, r->next_vcpu);
 
-    if(r->prev_vcpu > MAX_CPUS)
+    if(r->prev_vcpu >= MAX_CPUS)
     {
         fprintf(warn, "%s: prev_vcpu %u > MAX_VCPUS %d!\n",
                 __func__, r->prev_vcpu, MAX_CPUS);
         return;
     }
 
-    if(r->next_vcpu > MAX_CPUS)
+    if(r->next_vcpu >= MAX_CPUS)
     {
         fprintf(warn, "%s: next_vcpu %u > MAX_VCPUS %d!\n",
                 __func__, r->next_vcpu, MAX_CPUS);
@@ -8518,7 +8525,7 @@ off_t scan_for_new_pcpu(off_t offset) {
 
     cd = (typeof(cd))rec.u.notsc.data;
 
-    if ( cd->cpu > MAX_CPUS )
+    if ( cd->cpu >= MAX_CPUS )
     {
         fprintf(stderr, "%s: cpu %d exceeds MAX_CPU %d!\n",
                 __func__, cd->cpu, MAX_CPUS);
@@ -8738,7 +8745,7 @@ void process_cpu_change(struct pcpu_info *p) {
                 (unsigned long long)p->file_offset);
     }
 
-    if(r->cpu > MAX_CPUS)
+    if(r->cpu >= MAX_CPUS)
     {
         fprintf(stderr, "FATAL: cpu %d > MAX_CPUS %d.\n",
                 r->cpu, MAX_CPUS);
-- 
2.1.4


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

[PATCH 7/8] tools/xenalyze: Fix multiple instances of *HYPERCALL_MAX

[George Dunlap]

We HYPERCALL_MAX defined as the maximum enumerated hypercall, and we
have PV_HYPERCALL_MAX defined as some other number (presumably based
on experience with actual hypercalls).  Both are used to size arrays
(hypercall_name[] and pv_data.hypercall_count[], respectively).

Rename PV_HYPERCALL_MAX to HYPERCALL_MAX, and use HYPERCALL_MAX to
size (and iterate over) all arrays.

While we're at it, print "(unknown)" for values not present in the
hypercall_name[] array.

CID 1306868

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/xentrace/xenalyze.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/tools/xentrace/xenalyze.c b/tools/xentrace/xenalyze.c
index 3e26a4c..4ae50b8 100644
--- a/tools/xentrace/xenalyze.c
+++ b/tools/xentrace/xenalyze.c
@@ -1068,9 +1068,10 @@ enum {
     HYPERCALL_sysctl,
     HYPERCALL_domctl,
     HYPERCALL_kexec_op,
-    HYPERCALL_MAX
 };
 
+#define HYPERCALL_MAX 38
+
 char *hypercall_name[HYPERCALL_MAX] = {
     [HYPERCALL_set_trap_table]="set_trap_table",
     [HYPERCALL_mmu_update]="mmu_update",
@@ -1509,13 +1510,12 @@ char *pv_name[PV_MAX] = {
     [PV_HYPERCALL_SUBCALL]="hypercall (subcall)",
 };
 
-#define PV_HYPERCALL_MAX 56
 #define PV_TRAP_MAX 20
 
 struct pv_data {
     unsigned summary_info:1;
     int count[PV_MAX];
-    int hypercall_count[PV_HYPERCALL_MAX];
+    int hypercall_count[HYPERCALL_MAX];
     int trap_count[PV_TRAP_MAX];
 };
 
@@ -6405,7 +6405,7 @@ void pv_hypercall_process(struct record_info *ri, struct pv_data *pv) {
     }
 
     if(opt.summary_info) {
-        if(eax < PV_HYPERCALL_MAX)
+        if(eax < HYPERCALL_MAX)
             pv->hypercall_count[eax]++;
     }
 
@@ -6566,10 +6566,10 @@ void pv_summary(struct pv_data *pv) {
         switch(i) {
         case PV_HYPERCALL:
         case PV_HYPERCALL_V2:
-            for(j=0; j<PV_HYPERCALL_MAX; j++) {
+            for(j=0; j<HYPERCALL_MAX; j++) {
                 if(pv->hypercall_count[j])
                     printf("    %-29s[%2d]: %6d\n",
-                           hypercall_name[j],
+                           hypercall_name[j]?:"(unknown)",
                            j,
                            pv->hypercall_count[j]);
             }
@@ -6677,7 +6677,7 @@ void pv_hypercall_v2_process(struct record_info *ri, struct pv_data *pv,
     int op = pv_hypercall_op(ri);
 
     if(opt.summary_info) {
-        if(op < PV_HYPERCALL_MAX)
+        if(op < HYPERCALL_MAX)
             pv->hypercall_count[op]++;
     }
 
-- 
2.1.4


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

[PATCH 8/8] tools/xenalyze: Actually handle case where number of ipi vectors exceeds static max

[George Dunlap]

find_vec() is supposed to find the vector in the list if it exists,
choose an empty slot if it doesn't exist, and return null if all slots
are full.

However, coverity noticed that although the callers of find_vec() handle
the last condition, find_vec() itself didn't.

Check to see if we actually found an empty slot before attempting to
initialize it.

CID 1306864

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/xentrace/xenalyze.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/xentrace/xenalyze.c b/tools/xentrace/xenalyze.c
index 4ae50b8..47fef36 100644
--- a/tools/xentrace/xenalyze.c
+++ b/tools/xentrace/xenalyze.c
@@ -3547,7 +3547,7 @@ struct outstanding_ipi *find_vec(struct vlapic_struct *vla, int vec)
             o = vla->outstanding.list + i;
     }
 
-    if(!o->valid) {
+    if(o && !o->valid) {
         o->vec = vec;
         o->valid = 1;
     }
-- 
2.1.4


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

Re: [PATCH 4/8] tools/xenalyze: Mark unreachable code as unreachable

[Ian Campbell]

On Thu, 2016-02-25 at 14:48 +0000, George Dunlap wrote:
> ...so that coverity knows it's unreachable.

I would not be surprised if Coverity starts complaining about the dead code
once this is in place. fprintf + abort is probably what would be wanted to
placate it in this case.

Ian.

> 
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> ---
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/xentrace/xenalyze.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tools/xentrace/xenalyze.c b/tools/xentrace/xenalyze.c
> index 9f8c065..123030a 100644
> --- a/tools/xentrace/xenalyze.c
> +++ b/tools/xentrace/xenalyze.c
> @@ -7306,6 +7306,7 @@ void sched_runstate_process(struct pcpu_info *p)
>              }
>              goto update;
>          }
> +        __builtin_unreachable();
>          fprintf(stderr, "FATAL: Logic hole in %s\n", __func__);
>          error(ERR_ASSERT, NULL);
>      }

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

Re: [PATCH 4/8] tools/xenalyze: Mark unreachable code as unreachable

[George Dunlap]

On 25/02/16 15:03, Ian Campbell wrote:
> On Thu, 2016-02-25 at 14:48 +0000, George Dunlap wrote:
>> ...so that coverity knows it's unreachable.
> 
> I would not be surprised if Coverity starts complaining about the dead code
> once this is in place. fprintf + abort is probably what would be wanted to
> placate it in this case.

Hrm -- it would be nice to have a way to figure out what coverity likes
without having to actually check something into the tree...

 -George


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

Re: [PATCH 4/8] tools/xenalyze: Mark unreachable code as unreachable

[Ian Campbell]

On Thu, 2016-02-25 at 15:09 +0000, George Dunlap wrote:
> On 25/02/16 15:03, Ian Campbell wrote:
> > On Thu, 2016-02-25 at 14:48 +0000, George Dunlap wrote:
> > > ...so that coverity knows it's unreachable.
> > 
> > I would not be surprised if Coverity starts complaining about the dead
> > code
> > once this is in place. fprintf + abort is probably what would be wanted
> > to
> > placate it in this case.
> 
> Hrm -- it would be nice to have a way to figure out what coverity likes
> without having to actually check something into the tree...

If this code is truly unreachable (i.e. it is after a while(1) with no
breaks etc) then you should just drop the logging since it will never be
reached, then the __builtin_unreachable() is appropriate.

If, as the log message implies, this is code which _should_ be unreachable
by design but would be reached in the case of a logic error in the
preceding code then what you want is either fprintf()+abort() or maybe
assert().

But Coverity seems to have disproven this possibility, correctly AFAICT
because all of the preceeding cases of the if chain end with a goto, this
removing the logging and leaving the __builtin_unreachable() is the way to
go.

I don't think this is really about what would keep Coverity happy, more to
do with the intended semantics of execution reaching this point.

BTW in my simple test case actually trying to execute
__builtin_unreachable() results in a SEGV, so that logging really isn't
doing anything useful with your patch.

Ian.

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

Re: [PATCH 4/8] tools/xenalyze: Mark unreachable code as unreachable

[George Dunlap]

On 25/02/16 15:28, Ian Campbell wrote:
> On Thu, 2016-02-25 at 15:09 +0000, George Dunlap wrote:
>> On 25/02/16 15:03, Ian Campbell wrote:
>>> On Thu, 2016-02-25 at 14:48 +0000, George Dunlap wrote:
>>>> ...so that coverity knows it's unreachable.
>>>
>>> I would not be surprised if Coverity starts complaining about the dead
>>> code
>>> once this is in place. fprintf + abort is probably what would be wanted
>>> to
>>> placate it in this case.
>>
>> Hrm -- it would be nice to have a way to figure out what coverity likes
>> without having to actually check something into the tree...
> 
> If this code is truly unreachable (i.e. it is after a while(1) with no
> breaks etc) then you should just drop the logging since it will never be
> reached, then the __builtin_unreachable() is appropriate.
> 
> If, as the log message implies, this is code which _should_ be unreachable
> by design but would be reached in the case of a logic error in the
> preceding code then what you want is either fprintf()+abort() or maybe
> assert().

Right -- well basically error(ASSERT,...) is a custom abort().  But in
the current case it isn't actually doing anything more than an abort()
would, so perhaps I should use that instead (since coverity knows about
abort() and assert() but not my custom function).

> But Coverity seems to have disproven this possibility, correctly AFAICT
> because all of the preceeding cases of the if chain end with a goto, this
> removing the logging and leaving the __builtin_unreachable() is the way to
> go.
> 
> I don't think this is really about what would keep Coverity happy, more to
> do with the intended semantics of execution reaching this point.

It's already doing what I want mostly; so maybe I should just close the
bug as "intentional" (or "needs modelling" or something).

 -George


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

Re: [PATCH 4/8] tools/xenalyze: Mark unreachable code as unreachable

[Ian Campbell]

On Thu, 2016-02-25 at 15:43 +0000, George Dunlap wrote:
> On 25/02/16 15:28, Ian Campbell wrote:
> > On Thu, 2016-02-25 at 15:09 +0000, George Dunlap wrote:
> > > On 25/02/16 15:03, Ian Campbell wrote:
> > > > On Thu, 2016-02-25 at 14:48 +0000, George Dunlap wrote:
> > > > > ...so that coverity knows it's unreachable.
> > > > 
> > > > I would not be surprised if Coverity starts complaining about the
> > > > dead
> > > > code
> > > > once this is in place. fprintf + abort is probably what would be
> > > > wanted
> > > > to
> > > > placate it in this case.
> > > 
> > > Hrm -- it would be nice to have a way to figure out what coverity
> > > likes
> > > without having to actually check something into the tree...
> > 
> > If this code is truly unreachable (i.e. it is after a while(1) with no
> > breaks etc) then you should just drop the logging since it will never
> > be
> > reached, then the __builtin_unreachable() is appropriate.
> > 
> > If, as the log message implies, this is code which _should_ be
> > unreachable
> > by design but would be reached in the case of a logic error in the
> > preceding code then what you want is either fprintf()+abort() or maybe
> > assert().
> 
> Right -- well basically error(ASSERT,...) is a custom abort().  But in
> the current case it isn't actually doing anything more than an abort()
> would, so perhaps I should use that instead (since coverity knows about
> abort() and assert() but not my custom function).

Personally this is what I would do in this case.

> > But Coverity seems to have disproven this possibility, correctly AFAICT
> > because all of the preceeding cases of the if chain end with a goto,
> > this
> > removing the logging and leaving the __builtin_unreachable() is the way
> > to
> > go.
> > 
> > I don't think this is really about what would keep Coverity happy, more
> > to
> > do with the intended semantics of execution reaching this point.
> 
> It's already doing what I want mostly; so maybe I should just close the
> bug as "intentional" (or "needs modelling" or something).

This is also a valid thing to do.

Remember, the goal of coverity is to provide a list of places where there
might be opportunities for improvements to be made to the code, not to
provide a list of places to change just to make coverity shut up. If the
code isn't actually improved by fixing whatever coverity is complaining
about then "intentional" or "needs modelling" is the right response.

Ian.

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

Re: [PATCH 1/8] tools/xenalyze: Close symbol_file after reading it

[Ian Jackson]

George Dunlap writes ("[PATCH 1/8] tools/xenalyze: Close symbol_file after reading it"):
> ...to avoid leaking the FD and associated memory.
> 
> CID 1306872
> 
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

> Not sure if this will actually make coverity happy, or if we need to
> actually set symbol_file to NULL.

I don't see why you'd need to assign to the symbol_file local (which
is about to go out of scope).

Ian.

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

Re: [PATCH 2/8] tools/xenalyze: Avoid redundant check

[Ian Jackson]

George Dunlap writes ("[PATCH 2/8] tools/xenalyze: Avoid redundant check"):
> Coverity notices that if !head is true, that !N can never be true.
> 
> Don't bother checking N, since we know it has to be at least one.
> 
> CID 1354243
> 
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

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

Re: [PATCH 3/8] tools/xenalyze: Handle fstat errors properly

[Ian Jackson]

George Dunlap writes ("[PATCH 3/8] tools/xenalyze: Handle fstat errors properly"):
> They're pretty unlikely to fail, but doesn't hurt to check.
...
> -    fstat(fd, &s);
> +    if ( fstat(fd, &s) ) {
> +        perror("fstat");
> +        free(h);
> +        h = NULL;
> +        goto out;
> +    }

Is there some reason why you're not calling exit ?  mread64 already
does so on mmap failure.

For that matter, is error() not available in this file ?

>          struct stat s;
> -        fstat(G.fd, &s);
> +        if ( fstat(G.fd, &s) ) {
> +            perror("fstat");
> +            error(ERR_SYSTEM, NULL);
> +        }
>          G.file_size = s.st_size;

This hunk LGTM.

Ian.

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

Re: [PATCH 4/8] tools/xenalyze: Mark unreachable code as unreachable

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH 4/8] tools/xenalyze: Mark unreachable code as unreachable"):
> On Thu, 2016-02-25 at 15:43 +0000, George Dunlap wrote:
> > Right -- well basically error(ASSERT,...) is a custom abort().  But in
> > the current case it isn't actually doing anything more than an abort()
> > would, so perhaps I should use that instead (since coverity knows about
> > abort() and assert() but not my custom function).
> 
> Personally this is what I would do in this case.

Indeed.  I would replace the fprintf/error with

   assert(!"logic hole in this function");

This is a standard idiom for `abort()' when you don't like to just
call abort because it doesn't produce a message.

Ian.

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

Re: [PATCH 5/8] tools/xenalyze: Fix check for error return value

[Ian Jackson]

George Dunlap writes ("[PATCH 5/8] tools/xenalyze: Fix check for error return value"):
> fdopen returns NULL on failure, not a negative integer.
> 
> CID 1306863
> CID 1306858
> 
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> ---
> Strangely, coverity counted this as two bugs: one for comparing a
> pointer with a negative integer, one for the comparison never possibly
> being true (thus making the if condition dead code).

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

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

Re: [PATCH 6/8] tools/xenalyze: Fix off-by-one in MAX_CPUS range checks

[Ian Jackson]

George Dunlap writes ("[PATCH 6/8] tools/xenalyze: Fix off-by-one in MAX_CPUS range checks"):
> Skip action / throw error if cpu/vcpu >= MAX_CPUS  rather than >.
> 
> Also add an assertion to vcpu_find, to make future errors of this kind
> not out-of-bounds.
...
> +    /* "Graceful" handling of vid >= MAX_CPUS should be handled elsewhere */
> +    if ( vid >= MAX_CPUS ) {
> +        fprintf(stderr, "%s: vcpu %d exceeds MAX_CPUS %d!\n",
> +                __func__, vid, MAX_CPUS);
> +        error(ERR_ASSERT, NULL);
> +    }

I'm not convinced by the existence of error(ERR_ASSERT,...).  What is
wrong with assert() ?

If you agree that ERR_ASSERT should be got rid of, then you could
start here...

But:

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

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

Re: [PATCH 7/8] tools/xenalyze: Fix multiple instances of *HYPERCALL_MAX

[Ian Jackson]

George Dunlap writes ("[PATCH 7/8] tools/xenalyze: Fix multiple instances of *HYPERCALL_MAX"):
> We HYPERCALL_MAX defined as the maximum enumerated hypercall, and we
    ^ missing word `have' ?

> have PV_HYPERCALL_MAX defined as some other number (presumably based
> on experience with actual hypercalls).  Both are used to size arrays
> (hypercall_name[] and pv_data.hypercall_count[], respectively).
> 
> Rename PV_HYPERCALL_MAX to HYPERCALL_MAX, and use HYPERCALL_MAX to
> size (and iterate over) all arrays.
...
> diff --git a/tools/xentrace/xenalyze.c b/tools/xentrace/xenalyze.c
> index 3e26a4c..4ae50b8 100644
> --- a/tools/xentrace/xenalyze.c
> +++ b/tools/xentrace/xenalyze.c
> @@ -1068,9 +1068,10 @@ enum {
>      HYPERCALL_sysctl,
>      HYPERCALL_domctl,
>      HYPERCALL_kexec_op,
> -    HYPERCALL_MAX
>  };
>  
> +#define HYPERCALL_MAX 38
> +
>  char *hypercall_name[HYPERCALL_MAX] = {
>      [HYPERCALL_set_trap_table]="set_trap_table",
>      [HYPERCALL_mmu_update]="mmu_update",
> @@ -1509,13 +1510,12 @@ char *pv_name[PV_MAX] = {
>      [PV_HYPERCALL_SUBCALL]="hypercall (subcall)",

Does this produce a build error if HYPERCALL_MAX is too small ?

Ian.

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

Re: [PATCH 8/8] tools/xenalyze: Actually handle case where number of ipi vectors exceeds static max

[Ian Jackson]

George Dunlap writes ("[PATCH 8/8] tools/xenalyze: Actually handle case where number of ipi vectors exceeds static max"):
> find_vec() is supposed to find the vector in the list if it exists,
> choose an empty slot if it doesn't exist, and return null if all slots
> are full.
> 
> However, coverity noticed that although the callers of find_vec() handle
> the last condition, find_vec() itself didn't.
> 
> Check to see if we actually found an empty slot before attempting to
> initialize it.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

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

Re: [PATCH 1/8] tools/xenalyze: Close symbol_file after reading it

[Konrad Rzeszutek Wilk]

On Fri, Feb 26, 2016 at 12:22:39PM +0000, Ian Jackson wrote:
> George Dunlap writes ("[PATCH 1/8] tools/xenalyze: Close symbol_file after reading it"):
> > ...to avoid leaking the FD and associated memory.
> > 
> > CID 1306872
> > 
> > Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

And applied.
> 
> > Not sure if this will actually make coverity happy, or if we need to
> > actually set symbol_file to NULL.
> 
> I don't see why you'd need to assign to the symbol_file local (which
> is about to go out of scope).
> 
> Ian.
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

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

Re: [PATCH 2/8] tools/xenalyze: Avoid redundant check

[Konrad Rzeszutek Wilk]

On Fri, Feb 26, 2016 at 12:23:15PM +0000, Ian Jackson wrote:
> George Dunlap writes ("[PATCH 2/8] tools/xenalyze: Avoid redundant check"):
> > Coverity notices that if !head is true, that !N can never be true.
> > 
> > Don't bother checking N, since we know it has to be at least one.
> > 
> > CID 1354243
> > 
> > Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Applied.
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

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

Re: [PATCH 5/8] tools/xenalyze: Fix check for error return value

[Konrad Rzeszutek Wilk]

On Fri, Feb 26, 2016 at 12:29:02PM +0000, Ian Jackson wrote:
> George Dunlap writes ("[PATCH 5/8] tools/xenalyze: Fix check for error return value"):
> > fdopen returns NULL on failure, not a negative integer.
> > 
> > CID 1306863
> > CID 1306858
> > 
> > Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> > ---
> > Strangely, coverity counted this as two bugs: one for comparing a
> > pointer with a negative integer, one for the comparison never possibly
> > being true (thus making the if condition dead code).
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

applied
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

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

Re: [PATCH 8/8] tools/xenalyze: Actually handle case where number of ipi vectors exceeds static max

[Konrad Rzeszutek Wilk]

On Fri, Feb 26, 2016 at 12:34:56PM +0000, Ian Jackson wrote:
> George Dunlap writes ("[PATCH 8/8] tools/xenalyze: Actually handle case where number of ipi vectors exceeds static max"):
> > find_vec() is supposed to find the vector in the list if it exists,
> > choose an empty slot if it doesn't exist, and return null if all slots
> > are full.
> > 
> > However, coverity noticed that although the callers of find_vec() handle
> > the last condition, find_vec() itself didn't.
> > 
> > Check to see if we actually found an empty slot before attempting to
> > initialize it.
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

applied.
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

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

Re: [PATCH 6/8] tools/xenalyze: Fix off-by-one in MAX_CPUS range checks

[George Dunlap]

On 26/02/16 12:30, Ian Jackson wrote:
> George Dunlap writes ("[PATCH 6/8] tools/xenalyze: Fix off-by-one in MAX_CPUS range checks"):
>> Skip action / throw error if cpu/vcpu >= MAX_CPUS  rather than >.
>>
>> Also add an assertion to vcpu_find, to make future errors of this kind
>> not out-of-bounds.
> ...
>> +    /* "Graceful" handling of vid >= MAX_CPUS should be handled elsewhere */
>> +    if ( vid >= MAX_CPUS ) {
>> +        fprintf(stderr, "%s: vcpu %d exceeds MAX_CPUS %d!\n",
>> +                __func__, vid, MAX_CPUS);
>> +        error(ERR_ASSERT, NULL);
>> +    }
> 
> I'm not convinced by the existence of error(ERR_ASSERT,...).  What is
> wrong with assert() ?

Well one half of the reason for error() in general is to print out the
record which caused (or was involved in) the error before dying.  And
I'm guessing that once I decided I'd have error(ERR_ASSERT, xxx), that
for consistency I just decided to use error(ERR_ASSERT,...) everywhere.

But at least at this point, no instance of error(ERR_ASSERT...) actually
takes a pointer to a record, so that probably is something that could
just go away.

I'll send a new series with this updated.

 -George

> 
> If you agree that ERR_ASSERT should be got rid of, then you could
> start here...
> 
> But:
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 


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

Re: [PATCH 7/8] tools/xenalyze: Fix multiple instances of *HYPERCALL_MAX

[George Dunlap]

On 26/02/16 12:33, Ian Jackson wrote:
> George Dunlap writes ("[PATCH 7/8] tools/xenalyze: Fix multiple instances of *HYPERCALL_MAX"):
>> We HYPERCALL_MAX defined as the maximum enumerated hypercall, and we
>     ^ missing word `have' ?
> 
>> have PV_HYPERCALL_MAX defined as some other number (presumably based
>> on experience with actual hypercalls).  Both are used to size arrays
>> (hypercall_name[] and pv_data.hypercall_count[], respectively).
>>
>> Rename PV_HYPERCALL_MAX to HYPERCALL_MAX, and use HYPERCALL_MAX to
>> size (and iterate over) all arrays.
> ...
>> diff --git a/tools/xentrace/xenalyze.c b/tools/xentrace/xenalyze.c
>> index 3e26a4c..4ae50b8 100644
>> --- a/tools/xentrace/xenalyze.c
>> +++ b/tools/xentrace/xenalyze.c
>> @@ -1068,9 +1068,10 @@ enum {
>>      HYPERCALL_sysctl,
>>      HYPERCALL_domctl,
>>      HYPERCALL_kexec_op,
>> -    HYPERCALL_MAX
>>  };
>>  
>> +#define HYPERCALL_MAX 38
>> +
>>  char *hypercall_name[HYPERCALL_MAX] = {
>>      [HYPERCALL_set_trap_table]="set_trap_table",
>>      [HYPERCALL_mmu_update]="mmu_update",
>> @@ -1509,13 +1510,12 @@ char *pv_name[PV_MAX] = {
>>      [PV_HYPERCALL_SUBCALL]="hypercall (subcall)",
> 
> Does this produce a build error if HYPERCALL_MAX is too small ?

You mean, if it's smaller than at least one of the indexes in the array
initialization immediately following?  Yes. (I just tested it to be sure.)

 -G


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

Re: [PATCH 7/8] tools/xenalyze: Fix multiple instances of *HYPERCALL_MAX

[Ian Jackson]

George Dunlap writes ("Re: [PATCH 7/8] tools/xenalyze: Fix multiple instances of *HYPERCALL_MAX"):
> On 26/02/16 12:33, Ian Jackson wrote:
> > Does this produce a build error if HYPERCALL_MAX is too small ?
> 
> You mean, if it's smaller than at least one of the indexes in the array
> initialization immediately following?  Yes. (I just tested it to be sure.)

Right.  Jolly good.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

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

Re: [PATCH 3/8] tools/xenalyze: Handle fstat errors properly

[George Dunlap]

On 26/02/16 12:25, Ian Jackson wrote:
> George Dunlap writes ("[PATCH 3/8] tools/xenalyze: Handle fstat errors properly"):
>> They're pretty unlikely to fail, but doesn't hurt to check.
> ...
>> -    fstat(fd, &s);
>> +    if ( fstat(fd, &s) ) {
>> +        perror("fstat");
>> +        free(h);
>> +        h = NULL;
>> +        goto out;
>> +    }
> 
> Is there some reason why you're not calling exit ?  mread64 already
> does so on mmap failure.
> 
> For that matter, is error() not available in this file ?

I originally meant this to be library-like (a faster implementation of
pread actually).  So it doesn't have error(), and for consistency
probably shouldn't be calling exit() on mmap() failure.

Is there a reason not to just return null and let the caller figure out
what to do?

 -George


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

Re: [PATCH 6/8] tools/xenalyze: Fix off-by-one in MAX_CPUS range checks

[George Dunlap]

On 29/02/16 16:58, George Dunlap wrote:
> On 26/02/16 12:30, Ian Jackson wrote:
>> George Dunlap writes ("[PATCH 6/8] tools/xenalyze: Fix off-by-one in MAX_CPUS range checks"):
>>> Skip action / throw error if cpu/vcpu >= MAX_CPUS  rather than >.
>>>
>>> Also add an assertion to vcpu_find, to make future errors of this kind
>>> not out-of-bounds.
>> ...
>>> +    /* "Graceful" handling of vid >= MAX_CPUS should be handled elsewhere */
>>> +    if ( vid >= MAX_CPUS ) {
>>> +        fprintf(stderr, "%s: vcpu %d exceeds MAX_CPUS %d!\n",
>>> +                __func__, vid, MAX_CPUS);
>>> +        error(ERR_ASSERT, NULL);
>>> +    }
>>
>> I'm not convinced by the existence of error(ERR_ASSERT,...).  What is
>> wrong with assert() ?
> 
> Well one half of the reason for error() in general is to print out the
> record which caused (or was involved in) the error before dying.  And
> I'm guessing that once I decided I'd have error(ERR_ASSERT, xxx), that
> for consistency I just decided to use error(ERR_ASSERT,...) everywhere.

Oh, actually -- going through and implementing this change, I *think*
that the problem I had was actually that assert() doesn't flush stdout
before calling abort().  In dump mode every single trace record is
printed to stdout, which makes it fairly easy to figure out how you go
to the point of the assertion -- as long as it's actually printed out.

In fact in one location I had commented out an assert() and replaced it
with an if() {fprintf(...) error(...)}, presumably for exactly that reason.

In the case of xenalyze, all the recent trace records after an error
message is actually a lot more useful for forensics than having the
stack trace (which is what abort() gives you).

 -George


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