Taking on a Xen development project

Taking on a Xen development project

[jtotto]

Hi,

We're a team of three fourth-year undergraduate software engineering
students at the University of Waterloo in Canada.  We're in the process
of planning for our capstone design project, and are interested in
contributing to Xen.  Ideally, we'd like to take on a hypervisor/kernel
hacking project with roughly the same scope as a Google Summer of Code
project (like the hypervisor or domain support projects described at
[0]), following a similar timeline (roughly May to August 2016).  We're
all broadly interested in systems programming in C, and have each had
relevant academic and internship experiences.

Each of projects [1-3] currently on the Wiki look interesting, though
we'd be completely open to others as well.  In particular, we'd be open
to picking up Ben Catterall's work on HVM x86 deprivileged mode [4].  Do
any of these projects seem like a good fit in terms of usefulness to the
community and our timeline?  If so, we'd love to communicate more with
any maintainers with projects in mind!  We were also hoping to
familiarize ourselves with the project by addressing some Coverity
issues, if any are open at the moment.

Thanks!

Harley Armstrong, Chester Lin, Joshua Otto

[0] http://wiki.xenproject.org/wiki/GSoC_2015
[1]  
<http://wiki.xenproject.org/wiki/Xen_Development_Projects#Utilize_Intel_QuickPath_on_network_and_block_path.>
[2]  
<http://wiki.xenproject.org/wiki/Xen_Development_Projects#Introducing_PowerClamp-like_driver_for_Xen>
[3]  
<http://wiki.xenproject.org/wiki/Xen_Development_Projects#Allowing_guests_to_boot_with_a_passed-through_GPU_as_the_primary_display>
[4]  
<http://lists.xenproject.org/archives/html/xen-devel/2015-09/msg01550.html>

Re: Taking on a Xen development project

[Wei Liu]

Hello

On Fri, Dec 04, 2015 at 03:26:00PM -0500, jtotto@uwaterloo.ca wrote:
> Hi,
> 
> We're a team of three fourth-year undergraduate software engineering
> students at the University of Waterloo in Canada.  We're in the process
> of planning for our capstone design project, and are interested in
> contributing to Xen.  Ideally, we'd like to take on a hypervisor/kernel
> hacking project with roughly the same scope as a Google Summer of Code
> project (like the hypervisor or domain support projects described at
> [0]), following a similar timeline (roughly May to August 2016).  We're
> all broadly interested in systems programming in C, and have each had
> relevant academic and internship experiences.
> 
> Each of projects [1-3] currently on the Wiki look interesting, though
> we'd be completely open to others as well.  In particular, we'd be open
> to picking up Ben Catterall's work on HVM x86 deprivileged mode [4].  Do
> any of these projects seem like a good fit in terms of usefulness to the
> community and our timeline?  If so, we'd love to communicate more with
> any maintainers with projects in mind!  We were also hoping to
> familiarize ourselves with the project by addressing some Coverity
> issues, if any are open at the moment.
> 
> Thanks!
> 
> Harley Armstrong, Chester Lin, Joshua Otto
> 
> [0] http://wiki.xenproject.org/wiki/GSoC_2015
> [1] <http://wiki.xenproject.org/wiki/Xen_Development_Projects#Utilize_Intel_QuickPath_on_network_and_block_path.>
> [2] <http://wiki.xenproject.org/wiki/Xen_Development_Projects#Introducing_PowerClamp-like_driver_for_Xen>
> [3] <http://wiki.xenproject.org/wiki/Xen_Development_Projects#Allowing_guests_to_boot_with_a_passed-through_GPU_as_the_primary_display>
> [4]
> <http://lists.xenproject.org/archives/html/xen-devel/2015-09/msg01550.html>
> 

The powerclamp project has been taken by our Outreachy intern and the
x86 depriveleged project has been taken by Anthony Perard.

I've CC the mentors of the other two projects for you.

Wei.

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

Re: Taking on a Xen development project

[Andrew Cooper]

On 04/12/15 20:26, jtotto@uwaterloo.ca wrote:
> Hi,
>
> We're a team of three fourth-year undergraduate software engineering
> students at the University of Waterloo in Canada.  We're in the process
> of planning for our capstone design project, and are interested in
> contributing to Xen.  Ideally, we'd like to take on a hypervisor/kernel
> hacking project with roughly the same scope as a Google Summer of Code
> project (like the hypervisor or domain support projects described at
> [0]), following a similar timeline (roughly May to August 2016).  We're
> all broadly interested in systems programming in C, and have each had
> relevant academic and internship experiences.
>
> Each of projects [1-3] currently on the Wiki look interesting, though
> we'd be completely open to others as well.  In particular, we'd be open
> to picking up Ben Catterall's work on HVM x86 deprivileged mode [4].  Do
> any of these projects seem like a good fit in terms of usefulness to the
> community and our timeline?  If so, we'd love to communicate more with
> any maintainers with projects in mind!  We were also hoping to
> familiarize ourselves with the project by addressing some Coverity
> issues, if any are open at the moment.
>
> Thanks!
>
> Harley Armstrong, Chester Lin, Joshua Otto

Hello - thankyou for your interest.

One area to look at might be the parameters to the live migration
looping.  As part of the migration v2 rework I did in the 4.6 dev
period, I left all of that alone, and it is in a working but poor state.

In the past, there have been several research investigations into
improving the live migration algorithm, such as tracking the rate of
dirtying of memory, or attempting to resume the domain on the far side
and fault the final memory across.

If you are interested in perusing this, start with reading
docs/features/migration.pandoc in the Xen tree.

~Andrew

Re: Taking on a Xen development project

[Ian Campbell]

On Fri, 2015-12-04 at 15:26 -0500, jtotto@uwaterloo.ca wrote:
> We're a team of three fourth-year undergraduate software engineering
> students at the University of Waterloo in Canada.  We're in the process
> of planning for our capstone design project, and are interested in
> contributing to Xen.

Cool! Just to be clear, you are looking for one project for the 3 of you to
work on as a group (vs 3 individual projects), is that right?

> We were also hoping to
> familiarize ourselves with the project by addressing some Coverity
> issues, if any are open at the moment.

It's been a while since there has been a scan run, I did one yesterday but
it is taking an unusually long time to get the results back. Hopefully
we'll have an up to date set of defects early next week and I can have a
scrobble around for some interesting ones for you guys to take a look at.

Ian.

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

Re: Taking on a Xen development project

[Yang Hongyang]

On 2015年12月11日 01:23, Andrew Cooper wrote:
> On 04/12/15 20:26, jtotto@uwaterloo.ca wrote:
>> Hi,
>>
>> We're a team of three fourth-year undergraduate software engineering
>> students at the University of Waterloo in Canada.  We're in the process
>> of planning for our capstone design project, and are interested in
>> contributing to Xen.  Ideally, we'd like to take on a hypervisor/kernel
>> hacking project with roughly the same scope as a Google Summer of Code
>> project (like the hypervisor or domain support projects described at
>> [0]), following a similar timeline (roughly May to August 2016).  We're
>> all broadly interested in systems programming in C, and have each had
>> relevant academic and internship experiences.
>>
>> Each of projects [1-3] currently on the Wiki look interesting, though
>> we'd be completely open to others as well.  In particular, we'd be open
>> to picking up Ben Catterall's work on HVM x86 deprivileged mode [4].  Do
>> any of these projects seem like a good fit in terms of usefulness to the
>> community and our timeline?  If so, we'd love to communicate more with
>> any maintainers with projects in mind!  We were also hoping to
>> familiarize ourselves with the project by addressing some Coverity
>> issues, if any are open at the moment.
>>
>> Thanks!
>>
>> Harley Armstrong, Chester Lin, Joshua Otto
>
> Hello - thankyou for your interest.
>
> One area to look at might be the parameters to the live migration
> looping.  As part of the migration v2 rework I did in the 4.6 dev
> period, I left all of that alone, and it is in a working but poor state.
>
> In the past, there have been several research investigations into
> improving the live migration algorithm, such as tracking the rate of
> dirtying of memory, or attempting to resume the domain on the far side
> and fault the final memory across.

I think you mean postcopy here? The hypervisor then needs to maintain
a dirty page bitmap and generate pagefault when a page is not yet
tranferred to the far end.
This feature already merged into QEMU2.5(kvm patch which generates
pagefault also been merged into linux kernel mainline), if you want a
reference, you can take a look at those patches.
This surely is a great aera to work on.

>
> If you are interested in perusing this, start with reading
> docs/features/migration.pandoc in the Xen tree.
>
> ~Andrew
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>

-- 
Thanks,
Yang

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

Re: Taking on a Xen development project

[Joshua Otto]

On Fri, Dec 11, 2015 at 01:52:41PM +0000, Ian Campbell wrote:
> Cool! Just to be clear, you are looking for one project for the 3 of you to
> work on as a group (vs 3 individual projects), is that right?

Yes, that's right.

> It's been a while since there has been a scan run, I did one yesterday but
> it is taking an unusually long time to get the results back. Hopefully
> we'll have an up to date set of defects early next week and I can have a
> scrobble around for some interesting ones for you guys to take a look at.

That would be perfect, thanks!

Josh

Re: Taking on a Xen development project

[Joshua Otto]

On Sat, Dec 12, 2015 at 10:19:31AM +0800, Yang Hongyang wrote:
> On 2015年12月11日 01:23, Andrew Cooper wrote:
> >
> >Hello - thankyou for your interest.
> >
> >One area to look at might be the parameters to the live migration
> >looping.  As part of the migration v2 rework I did in the 4.6 dev
> >period, I left all of that alone, and it is in a working but poor state.
> >
> >In the past, there have been several research investigations into
> >improving the live migration algorithm, such as tracking the rate of
> >dirtying of memory, or attempting to resume the domain on the far side
> >and fault the final memory across.
> 
> I think you mean postcopy here? The hypervisor then needs to maintain
> a dirty page bitmap and generate pagefault when a page is not yet
> tranferred to the far end.
> This feature already merged into QEMU2.5(kvm patch which generates
> pagefault also been merged into linux kernel mainline), if you want a
> reference, you can take a look at those patches.
> This surely is a great aera to work on.
> 
> >
> >If you are interested in perusing this, start with reading
> >docs/features/migration.pandoc in the Xen tree.

We'd definitely be interested in working on live migration!  The feature
is essentially an implementation of the approach described in Section
5.1 of [0], right?

Would the focus of the project be to implement and evaluate postcopy
live migration in Xen, then, or to more generally build on previous
research efforts? (either way sounds like fun!)

Thanks!

Josh

[0] <http://www.cl.cam.ac.uk/research/srg/netos/papers/2005-migration-nsdi-pre.pdf>

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

Re: Taking on a Xen development project

[Andrew Cooper]

On 12/12/2015 22:30, Joshua Otto wrote:
> On Sat, Dec 12, 2015 at 10:19:31AM +0800, Yang Hongyang wrote:
>> On 2015年12月11日 01:23, Andrew Cooper wrote:
>>> Hello - thankyou for your interest.
>>>
>>> One area to look at might be the parameters to the live migration
>>> looping.  As part of the migration v2 rework I did in the 4.6 dev
>>> period, I left all of that alone, and it is in a working but poor state.
>>>
>>> In the past, there have been several research investigations into
>>> improving the live migration algorithm, such as tracking the rate of
>>> dirtying of memory, or attempting to resume the domain on the far side
>>> and fault the final memory across.
>> I think you mean postcopy here? The hypervisor then needs to maintain
>> a dirty page bitmap and generate pagefault when a page is not yet
>> tranferred to the far end.
>> This feature already merged into QEMU2.5(kvm patch which generates
>> pagefault also been merged into linux kernel mainline), if you want a
>> reference, you can take a look at those patches.
>> This surely is a great aera to work on.
>>
>>> If you are interested in perusing this, start with reading
>>> docs/features/migration.pandoc in the Xen tree.
> We'd definitely be interested in working on live migration!  The feature
> is essentially an implementation of the approach described in Section
> 5.1 of [0], right?

Yes - Section 5.1 is quite a good general description of live migration
(even after my white-room redesign from first principles), although be
aware that some of the more technical details are now out of date.

>
> Would the focus of the project be to implement and evaluate postcopy
> live migration in Xen, then, or to more generally build on previous
> research efforts? (either way sounds like fun!)

I hadn't really though that far ahead.  I was more suggestion that the
general area of live migration has a lot in the way to offer for
projects, be it implementing someone else's research, or researching a
new area yourself.

There are definitely areas where improvements can be made, and some of
these would be an easier introduction to the codebase than to start with
a full postcopy implementation.  (Not that I wish to put you off
postcopy, but it does come with a number of non-trivial problems to
solve as part of getting the scaffolding in place.)

I guess it depends on what you are looking to get out of a project like
this, and what timescale you have.

~Andrew

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

Re: Taking on a Xen development project

[Ian Campbell]

On Sat, 2015-12-12 at 17:07 -0500, Joshua Otto wrote:
> On Fri, Dec 11, 2015 at 01:52:41PM +0000, Ian Campbell wrote:
> > Cool! Just to be clear, you are looking for one project for the 3 of
> > you to
> > work on as a group (vs 3 individual projects), is that right?
> 
> Yes, that's right.
> 
> > It's been a while since there has been a scan run, I did one yesterday but
> > it is taking an unusually long time to get the results back. Hopefully
> > we'll have an up to date set of defects early next week and I can have a
> > scrobble around for some interesting ones for you guys to take a look at.
> 
> That would be perfect, thanks!

Results are in. I've cherry-picked a few of the new issues below. I've not
checked carefully for false +ves.

Not a great deal of massive thrills in there, but some one liners etc to
dip your toes in I guess.

Ian.
________________________________________________________________________________________________________
*** CID 1343310:  Code maintainability issues  (UNUSED_VALUE)
/xen/arch/x86/hvm/svm/intr.c: 95 in svm_enable_intr_window()
89                 struct vmcb_struct *gvmcb = nv->nv_vvmcx;
90     
91                 /* check if l1 guest injects interrupt into l2 guest via vintr.
92                  * return here or l2 guest looses interrupts, otherwise.
93                  */
94                 ASSERT(gvmcb != NULL);
>>>     CID 1343310:  Code maintainability issues  (UNUSED_VALUE)
>>>     Assigning value from "vmcb_get_vintr(gvmcb)" to "intr" here, but that stored value is overwritten before it can be used.
95                 intr = vmcb_get_vintr(gvmcb);
96                 if ( intr.fields.irq )
97                     return;
98             }
99         }
100     
________________________________________________________________________________________________________
*** CID 1343309:  Control flow issues  (UNREACHABLE)
/tools/libxl/libxl.c: 5575 in libxl_get_scheduler()
5569     {
5570         libxl_scheduler sched, ret;
5571         GC_INIT(ctx);
5572         if ((ret = xc_sched_id(ctx->xch, (int *)&sched)) != 0) {
5573             LOGE(ERROR, "getting domain info list");
5574             return ERROR_FAIL;
>>>     CID 1343309:  Control flow issues  (UNREACHABLE)
>>>     This code cannot be reached: "libxl__free_all(gc);".
5575             GC_FREE;
5576         }
5577         GC_FREE;
5578         return sched;
5579     }
5580     

As well as putting GC_FREE in the right place this function could be
reworked to follow the recommendations in tools/libxl/CODING_STYLE.

** CID 1343307:    (RESOURCE_LEAK)
/tools/libxl/libxl_dm.c: 746 in libxl__dm_runas_helper()
/tools/libxl/libxl_dm.c: 748 in libxl__dm_runas_helper()
/tools/libxl/libxl_dm.c: 749 in libxl__dm_runas_helper()


________________________________________________________________________________________________________
*** CID 1343307:    (RESOURCE_LEAK)
/tools/libxl/libxl_dm.c: 746 in libxl__dm_runas_helper()
740             ret = getpwnam_r(username, &pwd, buf, buf_size, &user);
741             if (ret == ERANGE) {
742                 buf_size += 128;
743                 continue;
744             }
745             if (ret != 0)
>>>     CID 1343307:    (RESOURCE_LEAK)
>>>     Variable "buf" going out of scope leaks the storage it points to.
746                 return ERROR_FAIL;
747             if (user != NULL)
748                 return 1;
749             return 0;
750         }
751     }
/tools/libxl/libxl_dm.c: 748 in libxl__dm_runas_helper()
742                 buf_size += 128;
743                 continue;
744             }
745             if (ret != 0)
746                 return ERROR_FAIL;
747             if (user != NULL)
>>>     CID 1343307:    (RESOURCE_LEAK)
>>>     Variable "buf" going out of scope leaks the storage it points to.
748                 return 1;
749             return 0;
750         }
751     }
752     
753     static int libxl__build_device_model_args_new(libxl__gc *gc,
/tools/libxl/libxl_dm.c: 749 in libxl__dm_runas_helper()
743                 continue;
744             }
745             if (ret != 0)
746                 return ERROR_FAIL;
747             if (user != NULL)
748                 return 1;
>>>     CID 1343307:    (RESOURCE_LEAK)
>>>     Variable "buf" going out of scope leaks the storage it points to.
749             return 0;
750         }
751     }
752     
753     static int libxl__build_device_model_args_new(libxl__gc *gc,
754                                             const char *dm, int guest_domid,

*** CID 1343302:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
/xen/drivers/char/ns16550.c: 916 in pci_uart_config()
910     
911                             p = uart_config[i].param;
912                             /*
913                              * Force length of mmio region to be at least
914                              * 8 bytes times (1 << reg_shift)
915                              */
>>>     CID 1343302:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
>>>     Potentially overflowing expression "1 << uart_param[p].reg_shift" with type "int" (32 bits, signed) is evaluated using 32-bit arithmetic, and then used in a context that expects an expression of type "u64" (64 bits, unsigned).
916                             if ( size < (0x8 * (1 << uart_param[p].reg_shift)) )
917                                 continue;
918     
919                             if ( bar_idx >= uart_param[p].max_bars )
920                                 continue;
921     
________________________________________________________________________________________________________
*** CID 1343301:  Incorrect expression  (NO_EFFECT)
/xen/common/sched_credit.c: 1795 in csched_dump_pcpu()
1789     csched_dump_pcpu(const struct scheduler *ops, int cpu)
1790     {
1791         struct list_head *runq, *iter;
1792         struct csched_private *prv = CSCHED_PRIV(ops);
1793         struct csched_pcpu *spc;
1794         struct csched_vcpu *svc;
>>>     CID 1343301:  Incorrect expression  (NO_EFFECT)
>>>     Assignment operation "lock = lock" has no effect.
1795         spinlock_t *lock = lock;
1796         unsigned long flags;
1797         int loop;
1798     #define cpustr keyhandler_scratch
1799     
1800         /*

________________________________________________________________________________________________________
*** CID 1343299:  Incorrect expression  (MIXED_ENUMS)
/tools/libxl/libxl_psr.c: 313 in libxl_psr_cat_set_cbm()
307             goto out;
308         }
309     
310         libxl_for_each_set_bit(socketid, *target_map) {
311             if (socketid >= nr_sockets)
312                 break;
>>>     CID 1343299:  Incorrect expression  (MIXED_ENUMS)
>>>     Mixing enum types "enum libxl_psr_cbm_type" and "enum xc_psr_cat_type" for "type".
313             if (xc_psr_cat_set_domain_data(ctx->xch, domid, type, socketid, cbm)) {
314                 libxl__psr_cat_log_err_msg(gc, errno);
315                 rc = ERROR_FAIL;
316             }
317         }
318     
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: Taking on a Xen development project

[Joshua Otto]

On Sat, Dec 12, 2015 at 11:02:50PM +0000, Andrew Cooper wrote:
> On 12/12/2015 22:30, Joshua Otto wrote:
> > On Sat, Dec 12, 2015 at 10:19:31AM +0800, Yang Hongyang wrote:
> >> On 2015年12月11日 01:23, Andrew Cooper wrote:
> >>> Hello - thankyou for your interest.
> >>>
> >>> One area to look at might be the parameters to the live migration
> >>> looping.  As part of the migration v2 rework I did in the 4.6 dev
> >>> period, I left all of that alone, and it is in a working but poor state.
> >>>
> >>> In the past, there have been several research investigations into
> >>> improving the live migration algorithm, such as tracking the rate of
> >>> dirtying of memory, or attempting to resume the domain on the far side
> >>> and fault the final memory across.
> >> I think you mean postcopy here? The hypervisor then needs to maintain
> >> a dirty page bitmap and generate pagefault when a page is not yet
> >> tranferred to the far end.
> >> This feature already merged into QEMU2.5(kvm patch which generates
> >> pagefault also been merged into linux kernel mainline), if you want a
> >> reference, you can take a look at those patches.
> >> This surely is a great aera to work on.
> >>
> >>> If you are interested in perusing this, start with reading
> >>> docs/features/migration.pandoc in the Xen tree.
> > We'd definitely be interested in working on live migration!  The feature
> > is essentially an implementation of the approach described in Section
> > 5.1 of [0], right?
> 
> Yes - Section 5.1 is quite a good general description of live migration
> (even after my white-room redesign from first principles), although be
> aware that some of the more technical details are now out of date.
> 
> >
> > Would the focus of the project be to implement and evaluate postcopy
> > live migration in Xen, then, or to more generally build on previous
> > research efforts? (either way sounds like fun!)
> 
> I hadn't really though that far ahead.  I was more suggestion that the
> general area of live migration has a lot in the way to offer for
> projects, be it implementing someone else's research, or researching a
> new area yourself.
> 
> There are definitely areas where improvements can be made, and some of
> these would be an easier introduction to the codebase than to start with
> a full postcopy implementation.  (Not that I wish to put you off
> postcopy, but it does come with a number of non-trivial problems to
> solve as part of getting the scaffolding in place.)

Ah, okay.  Would our best path forward then be to gain enough technical
familiarity with the feature and past research to come back and propose
a more specific project on the list?

Thank you for all of your help!

Josh

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

Re: Taking on a Xen development project

[Joshua Otto]

On Mon, Dec 14, 2015 at 11:08:43AM +0000, Ian Campbell wrote:
> On Sat, 2015-12-12 at 17:07 -0500, Joshua Otto wrote:
> > On Fri, Dec 11, 2015 at 01:52:41PM +0000, Ian Campbell wrote:
> > > Cool! Just to be clear, you are looking for one project for the 3 of
> > > you to
> > > work on as a group (vs 3 individual projects), is that right?
> > 
> > Yes, that's right.
> > 
> > > It's been a while since there has been a scan run, I did one yesterday but
> > > it is taking an unusually long time to get the results back. Hopefully
> > > we'll have an up to date set of defects early next week and I can have a
> > > scrobble around for some interesting ones for you guys to take a look at.
> > 
> > That would be perfect, thanks!
> 
> Results are in. I've cherry-picked a few of the new issues below. I've not
> checked carefully for false +ves.
> 
> Not a great deal of massive thrills in there, but some one liners etc to
> dip your toes in I guess.

Thanks!  Hopefully we'll have patches to submit sometime next week.

Josh

Re: Taking on a Xen development project

[Ian Campbell]

On Mon, 2015-12-14 at 17:59 -0500, Joshua Otto wrote:
> On Mon, Dec 14, 2015 at 11:08:43AM +0000, Ian Campbell wrote:
> > On Sat, 2015-12-12 at 17:07 -0500, Joshua Otto wrote:
> > > On Fri, Dec 11, 2015 at 01:52:41PM +0000, Ian Campbell wrote:
> > > > Cool! Just to be clear, you are looking for one project for the 3 of
> > > > you to
> > > > work on as a group (vs 3 individual projects), is that right?
> > > 
> > > Yes, that's right.
> > > 
> > > > It's been a while since there has been a scan run, I did one yesterday but
> > > > it is taking an unusually long time to get the results back. Hopefully
> > > > we'll have an up to date set of defects early next week and I can have a
> > > > scrobble around for some interesting ones for you guys to take a look at.
> > > 
> > > That would be perfect, thanks!
> > 
> > Results are in. I've cherry-picked a few of the new issues below. I've not
> > checked carefully for false +ves.
> > 
> > Not a great deal of massive thrills in there, but some one liners etc to
> > dip your toes in I guess.
> 
> Thanks!  Hopefully we'll have patches to submit sometime next week.

Great!

Be aware that maintainer presence on list etc might be a bit spotty from
next week until the new year.

Ian.


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

Coverity tidying

[Joshua Otto]

On Mon, Dec 14, 2015 at 11:08:43AM +0000, Ian Campbell wrote:
> On Sat, 2015-12-12 at 17:07 -0500, Joshua Otto wrote:
> > On Fri, Dec 11, 2015 at 01:52:41PM +0000, Ian Campbell wrote:
> > > Cool! Just to be clear, you are looking for one project for the 3 of
> > > you to
> > > work on as a group (vs 3 individual projects), is that right?
> > 
> > Yes, that's right.
> > 
> > > It's been a while since there has been a scan run, I did one yesterday but
> > > it is taking an unusually long time to get the results back. Hopefully
> > > we'll have an up to date set of defects early next week and I can have a
> > > scrobble around for some interesting ones for you guys to take a look at.
> > 
> > That would be perfect, thanks!
> 
> Results are in. I've cherry-picked a few of the new issues below. I've not
> checked carefully for false +ves.
> 
> Not a great deal of massive thrills in there, but some one liners etc to
> dip your toes in I guess.

These patches address the Coverity scan issues identified below that appear to
be actual problems.  For issues that we believe to be false positives, we
briefly explain why.

We've attempted to CC maintainers according to get_maintainer.pl.

> ________________________________________________________________________________________________________
> *** CID 1343310:  Code maintainability issues  (UNUSED_VALUE)
> /xen/arch/x86/hvm/svm/intr.c: 95 in svm_enable_intr_window()
> 89                 struct vmcb_struct *gvmcb = nv->nv_vvmcx;
> 90     
> 91                 /* check if l1 guest injects interrupt into l2 guest via vintr.
> 92                  * return here or l2 guest looses interrupts, otherwise.
> 93                  */
> 94                 ASSERT(gvmcb != NULL);
> >>>     CID 1343310:  Code maintainability issues  (UNUSED_VALUE)
> >>>     Assigning value from "vmcb_get_vintr(gvmcb)" to "intr" here, but that stored value is overwritten before it can be used.
> 95                 intr = vmcb_get_vintr(gvmcb);
> 96                 if ( intr.fields.irq )
> 97                     return;
> 98             }
> 99         }
> 100     

intr is used on the next line, so this appears to be a false positive without an
obvious rephrasing that Coverity would accept.

> ________________________________________________________________________________________________________
> *** CID 1343309:  Control flow issues  (UNREACHABLE)
> /tools/libxl/libxl.c: 5575 in libxl_get_scheduler()
> 5569     {
> 5570         libxl_scheduler sched, ret;
> 5571         GC_INIT(ctx);
> 5572         if ((ret = xc_sched_id(ctx->xch, (int *)&sched)) != 0) {
> 5573             LOGE(ERROR, "getting domain info list");
> 5574             return ERROR_FAIL;
> >>>     CID 1343309:  Control flow issues  (UNREACHABLE)
> >>>     This code cannot be reached: "libxl__free_all(gc);".
> 5575             GC_FREE;
> 5576         }
> 5577         GC_FREE;
> 5578         return sched;
> 5579     }
> 5580     
>
> As well as putting GC_FREE in the right place this function could be
> reworked to follow the recommendations in tools/libxl/CODING_STYLE.

This issue is addressed by patches 1 and 2.

> ** CID 1343307:    (RESOURCE_LEAK)
> /tools/libxl/libxl_dm.c: 746 in libxl__dm_runas_helper()
> /tools/libxl/libxl_dm.c: 748 in libxl__dm_runas_helper()
> /tools/libxl/libxl_dm.c: 749 in libxl__dm_runas_helper()
>
>
> ________________________________________________________________________________________________________
> *** CID 1343307:    (RESOURCE_LEAK)
> /tools/libxl/libxl_dm.c: 746 in libxl__dm_runas_helper()
> 740             ret = getpwnam_r(username, &pwd, buf, buf_size, &user);
> 741             if (ret == ERANGE) {
> 742                 buf_size += 128;
> 743                 continue;
> 744             }
> 745             if (ret != 0)
> >>>     CID 1343307:    (RESOURCE_LEAK)
> >>>     Variable "buf" going out of scope leaks the storage it points to.
> 746                 return ERROR_FAIL;
> 747             if (user != NULL)
> 748                 return 1;
> 749             return 0;
> 750         }
> 751     }
> /tools/libxl/libxl_dm.c: 748 in libxl__dm_runas_helper()
> 742                 buf_size += 128;
> 743                 continue;
> 744             }
> 745             if (ret != 0)
> 746                 return ERROR_FAIL;
> 747             if (user != NULL)
> >>>     CID 1343307:    (RESOURCE_LEAK)
> >>>     Variable "buf" going out of scope leaks the storage it points to.
> 748                 return 1;
> 749             return 0;
> 750         }
> 751     }
> 752     
> 753     static int libxl__build_device_model_args_new(libxl__gc *gc,
> /tools/libxl/libxl_dm.c: 749 in libxl__dm_runas_helper()
> 743                 continue;
> 744             }
> 745             if (ret != 0)
> 746                 return ERROR_FAIL;
> 747             if (user != NULL)
> 748                 return 1;
> >>>     CID 1343307:    (RESOURCE_LEAK)
> >>>     Variable "buf" going out of scope leaks the storage it points to.
> 749             return 0;
> 750         }
> 751     }
> 752     
> 753     static int libxl__build_device_model_args_new(libxl__gc *gc,
> 754                                             const char *dm, int guest_domid,

This appears to be a false positive - libxl__realloc() ensures that any
new allocations are added to the gc, and that subsequent reallocations
bring the gc up to date, so exiting the function at any time should be
safe.

> *** CID 1343302:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
> /xen/drivers/char/ns16550.c: 916 in pci_uart_config()
> 910     
> 911                             p = uart_config[i].param;
> 912                             /*
> 913                              * Force length of mmio region to be at least
> 914                              * 8 bytes times (1 << reg_shift)
> 915                              */
> >>>     CID 1343302:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
> >>>     Potentially overflowing expression "1 << uart_param[p].reg_shift" with type "int" (32 bits, signed) is evaluated using 32-bit arithmetic, and then used in a context that expects an expression of type "u64" (64 bits, unsigned).
> 916                             if ( size < (0x8 * (1 << uart_param[p].reg_shift)) )
> 917                                 continue;
> 918     
> 919                             if ( bar_idx >= uart_param[p].max_bars )
> 920                                 continue;
> 921     

This should be fine - reg_shift never exceeds 2, so there's no need to
worry about overflow.  However, if it's possible that reg_shift might in the
future have a value that causes overflow it could be addressed by applying our
third patch.

> ________________________________________________________________________________________________________
> *** CID 1343301:  Incorrect expression  (NO_EFFECT)
> /xen/common/sched_credit.c: 1795 in csched_dump_pcpu()
> 1789     csched_dump_pcpu(const struct scheduler *ops, int cpu)
> 1790     {
> 1791         struct list_head *runq, *iter;
> 1792         struct csched_private *prv = CSCHED_PRIV(ops);
> 1793         struct csched_pcpu *spc;
> 1794         struct csched_vcpu *svc;
> >>>     CID 1343301:  Incorrect expression  (NO_EFFECT)
> >>>     Assignment operation "lock = lock" has no effect.
> 1795         spinlock_t *lock = lock;
> 1796         unsigned long flags;
> 1797         int loop;
> 1798     #define cpustr keyhandler_scratch
> 1799     
> 1800         /*

This is addressed by the fourth patch.

> *** CID 1343299:  Incorrect expression  (MIXED_ENUMS)
> /tools/libxl/libxl_psr.c: 313 in libxl_psr_cat_set_cbm()
> 307             goto out;
> 308         }
> 309     
> 310         libxl_for_each_set_bit(socketid, *target_map) {
> 311             if (socketid >= nr_sockets)
> 312                 break;
> >>>     CID 1343299:  Incorrect expression  (MIXED_ENUMS)
> >>>     Mixing enum types "enum libxl_psr_cbm_type" and "enum xc_psr_cat_type" for "type".
> 313             if (xc_psr_cat_set_domain_data(ctx->xch, domid, type, socketid, cbm)) {
> 314                 libxl__psr_cat_log_err_msg(gc, errno);
> 315                 rc = ERROR_FAIL;
> 316             }
> 317         }
> 318     

The enum types being mixed are identical - one lives in the libxl types
IDL and the other in libxc.  Our fifth patch introduces an explicit conversion,
in case this style is preferable.

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

[PATCH 1/5] libxl: tidy libxl_get_scheduler() according to CODING_STYLE

[Joshua Otto]

To more closely follow the guidelines in CODING_STYLE, store the result
of the libxc call in the local variable r, and the check the result of
the call in a separate statement.

Additionally, change the error log statement to more accurately reflect
the failure.  This is the only functional change introduced by this
patch.

Signed-off-by: Joshua Otto <jtotto@uwaterloo.ca>
---
 tools/libxl/libxl.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 9207621..ca4679b 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5585,10 +5585,11 @@ out:
 
 libxl_scheduler libxl_get_scheduler(libxl_ctx *ctx)
 {
-    libxl_scheduler sched, ret;
     GC_INIT(ctx);
-    if ((ret = xc_sched_id(ctx->xch, (int *)&sched)) != 0) {
-        LOGE(ERROR, "getting domain info list");
+    libxl_scheduler sched;
+    int r = xc_sched_id(ctx->xch, (int *)&sched);
+    if (r != 0) {
+        LOGE(ERROR, "getting current scheduler id");
         return ERROR_FAIL;
         GC_FREE;
     }
-- 
1.9.1

[PATCH 2/5] libxl: make GC_FREE reachable in libxl_get_scheduler()

[Joshua Otto]

Coverity CID 1343309

This patch preserves the multiple error paths in order to avoid
meaninglessly assigning the ERROR_FAIL libxl error code to the
return variable, which is of type libxl_scheduler.

Signed-off-by: Joshua Otto <jtotto@uwaterloo.ca>
---
 tools/libxl/libxl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index ca4679b..60a2509 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5590,8 +5590,8 @@ libxl_scheduler libxl_get_scheduler(libxl_ctx *ctx)
     int r = xc_sched_id(ctx->xch, (int *)&sched);
     if (r != 0) {
         LOGE(ERROR, "getting current scheduler id");
-        return ERROR_FAIL;
         GC_FREE;
+        return ERROR_FAIL;
     }
     GC_FREE;
     return sched;
-- 
1.9.1

[PATCH 3/5] ns16550: widen an integer constant for Coverity.

[Joshua Otto]

From: Harley Armstrong <hjarmstr@uwaterloo.ca>

Fix CID 1343302 by widening a constant used with uart_param.reg_shift to
avoid overflow for large values of reg_shift.

Signed-off-by: Harley Armstrong <hjarmstr@uwaterloo.ca>
---
 xen/drivers/char/ns16550.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index bc24015..546bba1 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -913,7 +913,7 @@ pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int bar_idx)
                          * Force length of mmio region to be at least
                          * 8 bytes times (1 << reg_shift)
                          */
-                        if ( size < (0x8 * (1 << uart_param[p].reg_shift)) )
+                        if ( size < (0x8 * (1ull << uart_param[p].reg_shift)) )
                             continue;
 
                         if ( bar_idx >= uart_param[p].max_bars )
-- 
1.9.1

[PATCH 4/5] credit: remove pointless local variable initialization.

[Joshua Otto]

Coverity CID 1343301

No functional changes.

Signed-off-by: Joshua Otto <jtotto@uwaterloo.ca>
---
 xen/common/sched_credit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
index 0dce790..02afddf 100644
--- a/xen/common/sched_credit.c
+++ b/xen/common/sched_credit.c
@@ -1792,7 +1792,7 @@ csched_dump_pcpu(const struct scheduler *ops, int cpu)
     struct csched_private *prv = CSCHED_PRIV(ops);
     struct csched_pcpu *spc;
     struct csched_vcpu *svc;
-    spinlock_t *lock = lock;
+    spinlock_t *lock;
     unsigned long flags;
     int loop;
 #define cpustr keyhandler_scratch
-- 
1.9.1

[PATCH 5/5] libxl: Add explicit cast to libxl_psr_cat_set_cbm

[Joshua Otto]

From: Chester Lin <czylin@uwaterloo.ca>

Fixes Coverity CID 1343299. The call to xc_psr_cat_set_domain_data()
expects type xc_psr_cat_type but is provided libxl_psr_cbm_type which
is defined in IDL. This change adds an explicit cast to fix the
Coverity warning, and tweaks the surrounding code to more closely
conform to the guidelines in CODING_STYLE.

No functional changes.

Signed-off-by: Chester Lin <czylin@uwaterloo.ca>
---
 tools/libxl/libxl_psr.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c
index 3d0dc61..1677f9c 100644
--- a/tools/libxl/libxl_psr.c
+++ b/tools/libxl/libxl_psr.c
@@ -298,7 +298,7 @@ int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t domid,
                           uint64_t cbm)
 {
     GC_INIT(ctx);
-    int rc;
+    int rc, r;
     int socketid, nr_sockets;
 
     rc = libxl__count_physical_sockets(gc, &nr_sockets);
@@ -310,7 +310,9 @@ int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t domid,
     libxl_for_each_set_bit(socketid, *target_map) {
         if (socketid >= nr_sockets)
             break;
-        if (xc_psr_cat_set_domain_data(ctx->xch, domid, type, socketid, cbm)) {
+        r = xc_psr_cat_set_domain_data(ctx->xch, domid, (xc_psr_cat_type) type,
+                                       socketid, cbm);
+        if (r) {
             libxl__psr_cat_log_err_msg(gc, errno);
             rc = ERROR_FAIL;
         }
@@ -326,11 +328,14 @@ int libxl_psr_cat_get_cbm(libxl_ctx *ctx, uint32_t domid,
                           uint64_t *cbm_r)
 {
     GC_INIT(ctx);
-    int rc = 0;
-
-    if (xc_psr_cat_get_domain_data(ctx->xch, domid, type, target, cbm_r)) {
+    int rc, r;
+    r = xc_psr_cat_get_domain_data(ctx->xch, domid, (xc_psr_cat_type) type,
+                                   target, cbm_r);
+    if (r) {
         libxl__psr_cat_log_err_msg(gc, errno);
         rc = ERROR_FAIL;
+    } else {
+        rc = 0;
     }
 
     GC_FREE;
-- 
1.9.1

Re: Coverity tidying

[Andrew Cooper]

On 28/12/15 05:16, Joshua Otto wrote:
> On Mon, Dec 14, 2015 at 11:08:43AM +0000, Ian Campbell wrote:
>> On Sat, 2015-12-12 at 17:07 -0500, Joshua Otto wrote:
>>> On Fri, Dec 11, 2015 at 01:52:41PM +0000, Ian Campbell wrote:
>>>> Cool! Just to be clear, you are looking for one project for the 3 of
>>>> you to
>>>> work on as a group (vs 3 individual projects), is that right?
>>> Yes, that's right.
>>>
>>>> It's been a while since there has been a scan run, I did one yesterday but
>>>> it is taking an unusually long time to get the results back. Hopefully
>>>> we'll have an up to date set of defects early next week and I can have a
>>>> scrobble around for some interesting ones for you guys to take a look at.
>>> That would be perfect, thanks!
>> Results are in. I've cherry-picked a few of the new issues below. I've not
>> checked carefully for false +ves.
>>
>> Not a great deal of massive thrills in there, but some one liners etc to
>> dip your toes in I guess.
> These patches address the Coverity scan issues identified below that appear to
> be actual problems.  For issues that we believe to be false positives, we
> briefly explain why.
>
> We've attempted to CC maintainers according to get_maintainer.pl.
>
>> ________________________________________________________________________________________________________
>> *** CID 1343310:  Code maintainability issues  (UNUSED_VALUE)
>> /xen/arch/x86/hvm/svm/intr.c: 95 in svm_enable_intr_window()
>> 89                 struct vmcb_struct *gvmcb = nv->nv_vvmcx;
>> 90
>> 91                 /* check if l1 guest injects interrupt into l2 guest via vintr.
>> 92                  * return here or l2 guest looses interrupts, otherwise.
>> 93                  */
>> 94                 ASSERT(gvmcb != NULL);
>>>>>       CID 1343310:  Code maintainability issues  (UNUSED_VALUE)
>>>>>       Assigning value from "vmcb_get_vintr(gvmcb)" to "intr" here, but that stored value is overwritten before it can be used.
>> 95                 intr = vmcb_get_vintr(gvmcb);
>> 96                 if ( intr.fields.irq )
>> 97                     return;
>> 98             }
>> 99         }
>> 100
> intr is used on the next line, so this appears to be a false positive without an
> obvious rephrasing that Coverity would accept.

The error message isn't fantastic, but the complaint that Coverity has 
is that we store intr here, then unilaterally store it again slightly 
lower in the function, no matter what value it had (with the early 
return presumably not being taken into account).

The error would probably be resolved if lines 95 and 96 turned into "if 
( vmcb_get_vintr(gvmcb).fields.irq )"

>
>> ________________________________________________________________________________________________________
>> *** CID 1343309:  Control flow issues  (UNREACHABLE)
>> /tools/libxl/libxl.c: 5575 in libxl_get_scheduler()
>> 5569     {
>> 5570         libxl_scheduler sched, ret;
>> 5571         GC_INIT(ctx);
>> 5572         if ((ret = xc_sched_id(ctx->xch, (int *)&sched)) != 0) {
>> 5573             LOGE(ERROR, "getting domain info list");
>> 5574             return ERROR_FAIL;
>>>>>       CID 1343309:  Control flow issues  (UNREACHABLE)
>>>>>       This code cannot be reached: "libxl__free_all(gc);".
>> 5575             GC_FREE;
>> 5576         }
>> 5577         GC_FREE;
>> 5578         return sched;
>> 5579     }
>> 5580
>>
>> As well as putting GC_FREE in the right place this function could be
>> reworked to follow the recommendations in tools/libxl/CODING_STYLE.
> This issue is addressed by patches 1 and 2.
>
>> ** CID 1343307:    (RESOURCE_LEAK)
>> /tools/libxl/libxl_dm.c: 746 in libxl__dm_runas_helper()
>> /tools/libxl/libxl_dm.c: 748 in libxl__dm_runas_helper()
>> /tools/libxl/libxl_dm.c: 749 in libxl__dm_runas_helper()
>>
>>
>> ________________________________________________________________________________________________________
>> *** CID 1343307:    (RESOURCE_LEAK)
>> /tools/libxl/libxl_dm.c: 746 in libxl__dm_runas_helper()
>> 740             ret = getpwnam_r(username, &pwd, buf, buf_size, &user);
>> 741             if (ret == ERANGE) {
>> 742                 buf_size += 128;
>> 743                 continue;
>> 744             }
>> 745             if (ret != 0)
>>>>>       CID 1343307:    (RESOURCE_LEAK)
>>>>>       Variable "buf" going out of scope leaks the storage it points to.
>> 746                 return ERROR_FAIL;
>> 747             if (user != NULL)
>> 748                 return 1;
>> 749             return 0;
>> 750         }
>> 751     }
>> /tools/libxl/libxl_dm.c: 748 in libxl__dm_runas_helper()
>> 742                 buf_size += 128;
>> 743                 continue;
>> 744             }
>> 745             if (ret != 0)
>> 746                 return ERROR_FAIL;
>> 747             if (user != NULL)
>>>>>       CID 1343307:    (RESOURCE_LEAK)
>>>>>       Variable "buf" going out of scope leaks the storage it points to.
>> 748                 return 1;
>> 749             return 0;
>> 750         }
>> 751     }
>> 752
>> 753     static int libxl__build_device_model_args_new(libxl__gc *gc,
>> /tools/libxl/libxl_dm.c: 749 in libxl__dm_runas_helper()
>> 743                 continue;
>> 744             }
>> 745             if (ret != 0)
>> 746                 return ERROR_FAIL;
>> 747             if (user != NULL)
>> 748                 return 1;
>>>>>       CID 1343307:    (RESOURCE_LEAK)
>>>>>       Variable "buf" going out of scope leaks the storage it points to.
>> 749             return 0;
>> 750         }
>> 751     }
>> 752
>> 753     static int libxl__build_device_model_args_new(libxl__gc *gc,
>> 754                                             const char *dm, int guest_domid,
> This appears to be a false positive - libxl__realloc() ensures that any
> new allocations are added to the gc, and that subsequent reallocations
> bring the gc up to date, so exiting the function at any time should be
> safe.

Correct.  Coverity is unable to track object ownership information when 
you start playing containerof() games to put it in a list.

~Andrew

[PATCH] svm: rephrase local variable use for Coverity.

[Joshua Otto]

Coverity CID 1343310

No functional changes.

Signed-off-by: Joshua Otto <jtotto@uwaterloo.ca>
---
On Mon, Dec 28, 2015 at 09:34:28AM +0000, Andrew Cooper wrote:
> The error message isn't fantastic, but the complaint that Coverity
> has is that we store intr here, then unilaterally store it again
> slightly lower in the function, no matter what value it had (with
> the early return presumably not being taken into account).
>
> The error would probably be resolved if lines 95 and 96 turned into
> "if ( vmcb_get_vintr(gvmcb).fields.irq )"

This patch implements that change - as a general rule, is maintainer
preference to resolve false positives like this by suppressing them in
the tool or through code changes like this one?

 xen/arch/x86/hvm/svm/intr.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/intr.c b/xen/arch/x86/hvm/svm/intr.c
index bd94731..240eb35 100644
--- a/xen/arch/x86/hvm/svm/intr.c
+++ b/xen/arch/x86/hvm/svm/intr.c
@@ -92,8 +92,7 @@ static void svm_enable_intr_window(struct vcpu *v, struct hvm_intack intack)
              * return here or l2 guest looses interrupts, otherwise.
              */
             ASSERT(gvmcb != NULL);
-            intr = vmcb_get_vintr(gvmcb);
-            if ( intr.fields.irq )
+            if ( vmcb_get_vintr(gvmcb).fields.irq )
                 return;
         }
     }
-- 
1.9.1

Re: [PATCH 1/5] libxl: tidy libxl_get_scheduler() according to CODING_STYLE

[Ian Campbell]

On Mon, 2015-12-28 at 00:16 -0500, Joshua Otto wrote:
> To more closely follow the guidelines in CODING_STYLE, store the result
> of the libxc call in the local variable r, and the check the result of
> the call in a separate statement.

I think a far more important aspect of this change is: 
    don't store the int return value of xc_sched_id into a variable of type
    libxl_scheduler

libxl_scheduler is an enum, and hence "int-like", but... still.

I think this is worth mentioning in the commit message, mainly because I'm
only 99% confident this is just a benign oddity rather than an actual
latent bug.

> Additionally, change the error log statement to more accurately reflect
> the failure.  This is the only functional change introduced by this
> patch.

Right.

> Signed-off-by: Joshua Otto <jtotto@uwaterloo.ca>


> ---
>  tools/libxl/libxl.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 9207621..ca4679b 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -5585,10 +5585,11 @@ out:
>  
>  libxl_scheduler libxl_get_scheduler(libxl_ctx *ctx)
>  {
> -    libxl_scheduler sched, ret;
>      GC_INIT(ctx);
> -    if ((ret = xc_sched_id(ctx->xch, (int *)&sched)) != 0) {
> -        LOGE(ERROR, "getting domain info list");
> +    libxl_scheduler sched;
> +    int r = xc_sched_id(ctx->xch, (int *)&sched);

If you were minded to make a further cleanup (i.e. this is totally
optional) then I'm not convinced this case is actually safe, since
libxl_scheduler could potentially be smaller than sizeof(int), or at least
IIRC the C standard give the compiler that option, although I don't know if
gcc will make use of it or if something else (e.g. OS calling convention on
Linux) would make it a non-issue (or I might be totally wrong...).

Safer (and cleaner looking even if I'm wrong) would be to use a temporary
int for the function call and turn it into an enum implicitly in the
return.

> +    if (r != 0) {
> +        LOGE(ERROR, "getting current scheduler id");
>          return ERROR_FAIL;
>          GC_FREE;
>      }

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

Re: [PATCH 2/5] libxl: make GC_FREE reachable in libxl_get_scheduler()

[Ian Campbell]

On Mon, 2015-12-28 at 00:16 -0500, Joshua Otto wrote:
> Coverity CID 1343309
> 
> This patch preserves the multiple error paths in order to avoid
> meaninglessly assigning the ERROR_FAIL libxl error code to the
> return variable, which is of type libxl_scheduler.

Which makes me think that the existing code is bogus to return an error
code as a libxl_scheduler too, since that is not very different to the
bogus assignment.

The function ought to return LIBXL_SCHEDLER_UNKNOWN. However that might be
considered an API breakage (since at least xl checks for errors with < 0).

A _really_ correct libxl function API would take an output libxl_scheduler
pointer and return an int error code, but that is definitely an API change.

Given that a caller really ought to be handling LIBXL_SCHEDULER_UNKNOWN as
a return value, even if it is also written to expect negative error values
as well, so I reckon we can get away with changing the return to
SCHEDULER_UNKNOWN in the error case. Either the caller already handles
that, or it was already buggy in not doing so.

That would also allow us to move to a single error path, since you'd no
longer worry about clobbering.

Wei, Ian, any thoughts?

> Signed-off-by: Joshua Otto <jtotto@uwaterloo.ca>
> ---
>  tools/libxl/libxl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index ca4679b..60a2509 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -5590,8 +5590,8 @@ libxl_scheduler libxl_get_scheduler(libxl_ctx *ctx)
>      int r = xc_sched_id(ctx->xch, (int *)&sched);
>      if (r != 0) {
>          LOGE(ERROR, "getting current scheduler id");
> -        return ERROR_FAIL;
>          GC_FREE;
> +        return ERROR_FAIL;
>      }
>      GC_FREE;
>      return sched;

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

Re: [PATCH 3/5] ns16550: widen an integer constant for Coverity.

[Ian Campbell]

On Mon, 2015-12-28 at 00:16 -0500, Joshua Otto wrote:

Please check MAINTAINERS (perhaps using ./scripts/get_maintainers.pl) to
determine the maintainers in order to CC them.

(Hrm, I see now in your cover letter you have, I wonder why Jan et al are
missing?)

> From: Harley Armstrong <hjarmstr@uwaterloo.ca>
> 
> Fix CID 1343302 by widening a constant used with uart_param.reg_shift to
> avoid overflow for large values of reg_shift.

A reg_shift large enough to actually expose this would be infeasibly large
(since it would imply a UART taking practically the entire virtual address
space of the processor).

So while Coverity is likely correct here, it is probably also a bit
misguided in the context.

I don't especially object to this change as means to quieten coverity, but
perhaps checking for some sane limit to reg_shift would also serve to
quieten coverity?

That would also avoid the need to check for overflow on the multiplication,
assuming a suitable sane limit was chosen.


> Signed-off-by: Harley Armstrong <hjarmstr@uwaterloo.ca>
> ---
>  xen/drivers/char/ns16550.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index bc24015..546bba1 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -913,7 +913,7 @@ pci_uart_config(struct ns16550 *uart, bool_t
> skip_amt, unsigned int bar_idx)
>                           * Force length of mmio region to be at least
>                           * 8 bytes times (1 << reg_shift)
>                           */
> -                        if ( size < (0x8 * (1 <<
> uart_param[p].reg_shift)) )
> +                        if ( size < (0x8 * (1ull << uart_param[p].reg_shift)) )

It looks from the surrounding code like
  ... < (0x8 * ((u64)1 << uart_param[p].reg_shift)) )

would be the preferred way of tackling this.

>                              continue;
>  
>                          if ( bar_idx >= uart_param[p].max_bars )

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

Re: [PATCH 5/5] libxl: Add explicit cast to libxl_psr_cat_set_cbm

[Ian Campbell]

On Mon, 2015-12-28 at 00:16 -0500, Joshua Otto wrote:
> From: Chester Lin <czylin@uwaterloo.ca>
> 
> Fixes Coverity CID 1343299. The call to xc_psr_cat_set_domain_data()
> expects type xc_psr_cat_type but is provided libxl_psr_cbm_type which
> is defined in IDL.

I assume the values are the same by construction in the IDL? Assuming so
then it would be worth mentioning that here I think, so we know why we
thought this was a valid change.

>  This change adds an explicit cast to fix the
> Coverity warning, and tweaks the surrounding code to more closely
> conform to the guidelines in CODING_STYLE.
> 
> No functional changes.
> 
> Signed-off-by: Chester Lin <czylin@uwaterloo.ca>
> ---
>  tools/libxl/libxl_psr.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c
> index 3d0dc61..1677f9c 100644
> --- a/tools/libxl/libxl_psr.c
> +++ b/tools/libxl/libxl_psr.c
> @@ -298,7 +298,7 @@ int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t
> domid,
>                            uint64_t cbm)
>  {
>      GC_INIT(ctx);
> -    int rc;
> +    int rc, r;
>      int socketid, nr_sockets;
>  
>      rc = libxl__count_physical_sockets(gc, &nr_sockets);
> @@ -310,7 +310,9 @@ int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t
> domid,
>      libxl_for_each_set_bit(socketid, *target_map) {
>          if (socketid >= nr_sockets)
>              break;
> -        if (xc_psr_cat_set_domain_data(ctx->xch, domid, type, socketid,
> cbm)) {
> +        r = xc_psr_cat_set_domain_data(ctx->xch, domid,
> (xc_psr_cat_type) type,
> +                                       socketid, cbm);
> +        if (r) {
>              libxl__psr_cat_log_err_msg(gc, errno);
>              rc = ERROR_FAIL;
>          }
> @@ -326,11 +328,14 @@ int libxl_psr_cat_get_cbm(libxl_ctx *ctx, uint32_t
> domid,
>                            uint64_t *cbm_r)
>  {
>      GC_INIT(ctx);
> -    int rc = 0;
> -
> -    if (xc_psr_cat_get_domain_data(ctx->xch, domid, type, target,
> cbm_r)) {
> +    int rc, r;
> +    r = xc_psr_cat_get_domain_data(ctx->xch, domid, (xc_psr_cat_type)
> type,
> +                                   target, cbm_r);
> +    if (r) {
>          libxl__psr_cat_log_err_msg(gc, errno);
>          rc = ERROR_FAIL;
> +    } else {
> +        rc = 0;
>      }
>  
>      GC_FREE;
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 1/5] libxl: tidy libxl_get_scheduler() according to CODING_STYLE

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1063 bytes --]

On Mon, 2016-01-04 at 16:23 +0000, Ian Campbell wrote:
> On Mon, 2015-12-28 at 00:16 -0500, Joshua Otto wrote:
> > 
> > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > @@ -5585,10 +5585,11 @@ out:
> >  
> >  libxl_scheduler libxl_get_scheduler(libxl_ctx *ctx)
> >  {
> > -    libxl_scheduler sched, ret;
> >      GC_INIT(ctx);
> > -    if ((ret = xc_sched_id(ctx->xch, (int *)&sched)) != 0) {
> > -        LOGE(ERROR, "getting domain info list");
> > +    libxl_scheduler sched;
> > +    int r = xc_sched_id(ctx->xch, (int *)&sched);
> 
> Safer (and cleaner looking even if I'm wrong) would be to use a
> temporary
> int for the function call and turn it into an enum implicitly in the
> return.
> 
FWIW, +1 to this

Regadrs,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH 2/5] libxl: make GC_FREE reachable in libxl_get_scheduler()

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 2370 bytes --]

On Mon, 2016-01-04 at 16:29 +0000, Ian Campbell wrote:
> On Mon, 2015-12-28 at 00:16 -0500, Joshua Otto wrote:
> > Coverity CID 1343309
> > 
> > This patch preserves the multiple error paths in order to avoid
> > meaninglessly assigning the ERROR_FAIL libxl error code to the
> > return variable, which is of type libxl_scheduler.
> 
> Which makes me think that the existing code is bogus to return an
> error
> code as a libxl_scheduler too, since that is not very different to
> the
> bogus assignment.
> 
Indeed.

> Given that a caller really ought to be handling
> LIBXL_SCHEDULER_UNKNOWN as
> a return value, even if it is also written to expect negative error
> values
> as well, so I reckon we can get away with changing the return to
> SCHEDULER_UNKNOWN in the error case. Either the caller already
> handles
> that, or it was already buggy in not doing so.
> 
Again, FWIW, I think this indeed is the proper way forward.

About callers, xl is, of course, quite easy to change.

I just quickly checked libvirt, and I think things will just continue
to work there too. In fact, libxl_get_scheduler() is used 3 times in
there, of which:
 - 2 of them, explicitly check for the result to 
   be LIBXL_SCHEDULER_CREDIT, and errors if it is not (as Credit1 is
   the only supported scheduler in libvirt for now)
 - 1 explicitly check for the result to be either _SEDF, _CREDIT, 
   _CREDIT2 or _ARINC653, and errors out if it's something else[1]

For other users, I agree that they should be handling or start to
handle LIBXL_SCHEDULER_UNKNOWN. *Maybe*, just as a "mitigation"
measure", we can redefine the enum and make "unknown"=-1... or is
something like that to be considered an API change as well?

I know, it looks really an hack, and it would remain wrong, for a
caller, to check for a libxl_scheduler object to be equal to
ERROR_FAIL, but maybe it's worth at least being considered.

Thanks and Regards,
Dario

[1] note to self, send a patch to update that (e.g., adding RTDS and
removing SEDF, when adequate, depending on Xen version)
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH 2/5] libxl: make GC_FREE reachable in libxl_get_scheduler()

[Ian Campbell]

On Tue, 2016-01-05 at 09:49 +0100, Dario Faggioli wrote:
> For other users, I agree that they should be handling or start to
> handle LIBXL_SCHEDULER_UNKNOWN. *Maybe*, just as a "mitigation"
> measure", we can redefine the enum and make "unknown"=-1... or is
> something like that to be considered an API change as well?

So in theory an enum can be signed per the spec (and is required to be if
any of the tags are assigned a -ve value) and it also seems like it is
signed in the ABIs we support (otherwise "sched < 0" would surely generate
warnings on some compiler).

However I don't think we want to change the value of SCHEDULER_UNKNOWN away
from 0 since we expect in many parts of the libxl API that things can be
checked for validity with ! (e.g. if (!sched) barf()) I haven't looked at
whether anyone does in this specific case.

I spoke with Ian J IRL and he suggested that libxl_get_scheduler should
probably return an explicit int, which can contain either the (negative)
libxl ERROR_* constants or the (positive) values of libxl_scheduler.

As far as we can tell there are no worry ABI or API considerations from
such a change (there are corner cases, such as function pointers no longer
being seen as compatible, but we think those won't be an issue in this
case).

Ian.

Re: [PATCH 3/5] ns16550: widen an integer constant for Coverity.

[Jan Beulich]

>>> On 04.01.16 at 17:36, <ian.campbell@citrix.com> wrote:
> On Mon, 2015-12-28 at 00:16 -0500, Joshua Otto wrote:
>> Fix CID 1343302 by widening a constant used with uart_param.reg_shift to
>> avoid overflow for large values of reg_shift.
> 
> A reg_shift large enough to actually expose this would be infeasibly large
> (since it would imply a UART taking practically the entire virtual address
> space of the processor).
> 
> So while Coverity is likely correct here, it is probably also a bit
> misguided in the context.
> 
> I don't especially object to this change as means to quieten coverity, but
> perhaps checking for some sane limit to reg_shift would also serve to
> quieten coverity?

Indeed I'd prefer an alternative change to be found, as 64-bit
arithmetic is quite pointless here.

>> Signed-off-by: Harley Armstrong <hjarmstr@uwaterloo.ca>
>> ---
>>  xen/drivers/char/ns16550.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
>> index bc24015..546bba1 100644
>> --- a/xen/drivers/char/ns16550.c
>> +++ b/xen/drivers/char/ns16550.c
>> @@ -913,7 +913,7 @@ pci_uart_config(struct ns16550 *uart, bool_t
>> skip_amt, unsigned int bar_idx)
>>                           * Force length of mmio region to be at least
>>                           * 8 bytes times (1 << reg_shift)
>>                          */
>> -                        if ( size < (0x8 * (1 << uart_param[p].reg_shift)) )
>> +                        if ( size < (0x8 * (1ull << uart_param[p].reg_shift)) )
> 
> It looks from the surrounding code like
>   ... < (0x8 * ((u64)1 << uart_param[p].reg_shift)) )
> 
> would be the preferred way of tackling this.

If we were to really stay with a change to this line, then the
multiplication should go away altogether, since just a shift
will always suffice.

Jan

Re: [PATCH] svm: rephrase local variable use for Coverity.

[Jan Beulich]

>>> On 01.01.16 at 04:14, <jtotto@uwaterloo.ca> wrote:
> Coverity CID 1343310
> 
> No functional changes.
> 
> Signed-off-by: Joshua Otto <jtotto@uwaterloo.ca>
> ---
> On Mon, Dec 28, 2015 at 09:34:28AM +0000, Andrew Cooper wrote:
>> The error message isn't fantastic, but the complaint that Coverity
>> has is that we store intr here, then unilaterally store it again
>> slightly lower in the function, no matter what value it had (with
>> the early return presumably not being taken into account).
>>
>> The error would probably be resolved if lines 95 and 96 turned into
>> "if ( vmcb_get_vintr(gvmcb).fields.irq )"
> 
> This patch implements that change - as a general rule, is maintainer
> preference to resolve false positives like this by suppressing them in
> the tool or through code changes like this one?

Asking such a question it would be helpful if you included the
maintainers of the code in question, since to a good part this
is a matter of taste, especially when ...

> --- a/xen/arch/x86/hvm/svm/intr.c
> +++ b/xen/arch/x86/hvm/svm/intr.c
> @@ -92,8 +92,7 @@ static void svm_enable_intr_window(struct vcpu *v, struct hvm_intack intack)
>               * return here or l2 guest looses interrupts, otherwise.
>               */
>              ASSERT(gvmcb != NULL);
> -            intr = vmcb_get_vintr(gvmcb);
> -            if ( intr.fields.irq )
> +            if ( vmcb_get_vintr(gvmcb).fields.irq )

... some people (not me) frown upon complex expressions like the
one resulting here.

Also please note that while perhaps minor here, obvious with
the quote from an earlier mail conversation, naming the person
having suggested the change would be appropriate - if you
look for them, you'll find quite a few Suggested-by: tags in the
commit history.

Jan

Re: [PATCH] svm: rephrase local variable use for Coverity.

[Boris Ostrovsky]

On 01/06/2016 08:24 AM, Jan Beulich wrote:
>>>> On 01.01.16 at 04:14, <jtotto@uwaterloo.ca> wrote:
>> Coverity CID 1343310
>>
>> No functional changes.
>>
>> Signed-off-by: Joshua Otto <jtotto@uwaterloo.ca>
>> ---
>> On Mon, Dec 28, 2015 at 09:34:28AM +0000, Andrew Cooper wrote:
>>> The error message isn't fantastic, but the complaint that Coverity
>>> has is that we store intr here, then unilaterally store it again
>>> slightly lower in the function, no matter what value it had (with
>>> the early return presumably not being taken into account).
>>>
>>> The error would probably be resolved if lines 95 and 96 turned into
>>> "if ( vmcb_get_vintr(gvmcb).fields.irq )"
>> This patch implements that change - as a general rule, is maintainer
>> preference to resolve false positives like this by suppressing them in
>> the tool or through code changes like this one?

I'd rather suppress this in the tool as I am one of those people that 
Jan refers to below ;-)

However, if it's too much of a hassle then this patch would be OK.

-boris

> Asking such a question it would be helpful if you included the
> maintainers of the code in question, since to a good part this
> is a matter of taste, especially when ...
>
>> --- a/xen/arch/x86/hvm/svm/intr.c
>> +++ b/xen/arch/x86/hvm/svm/intr.c
>> @@ -92,8 +92,7 @@ static void svm_enable_intr_window(struct vcpu *v, struct hvm_intack intack)
>>                * return here or l2 guest looses interrupts, otherwise.
>>                */
>>               ASSERT(gvmcb != NULL);
>> -            intr = vmcb_get_vintr(gvmcb);
>> -            if ( intr.fields.irq )
>> +            if ( vmcb_get_vintr(gvmcb).fields.irq )
> ... some people (not me) frown upon complex expressions like the
> one resulting here.
>
> Also please note that while perhaps minor here, obvious with
> the quote from an earlier mail conversation, naming the person
> having suggested the change would be appropriate - if you
> look for them, you'll find quite a few Suggested-by: tags in the
> commit history.
>
> Jan
>

[PATCH v2 1/5] libxl: tidy libxl_get_scheduler() according to CODING_STYLE

[Chester Lin]

To more closely follow the guidelines in CODING_STYLE, store the result
of xc_sched_id() in the local variable r, and the check the result of
the call in a separate statement.  Change the type of the output
parameter given to xc_sched_id() from libxl_scheduler to int to match
the libxc interface.

Additionally, change the error log statement to more accurately reflect
the failure.  This is the only functional change introduced by this
patch.

Suggested-by: Ian Campbell <ian.campbell@citrix.com>
Signed-off-by: Chester Lin <czylin@uwaterloo.ca>

---
Now storing the return of xc_sched_id in an int as per
  On Mon, 2016-01-04 at 16:23 +0000, Ian Campbell wrote:
  >Safer (and cleaner looking even if I'm wrong) would be to use a temporary
  >int for the function call and turn it into an enum implicitly in the return
---
 tools/libxl/libxl.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 9207621..7f28af8 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5585,10 +5585,12 @@ out:
 
 libxl_scheduler libxl_get_scheduler(libxl_ctx *ctx)
 {
-    libxl_scheduler sched, ret;
+    int r, sched;
+
     GC_INIT(ctx);
-    if ((ret = xc_sched_id(ctx->xch, (int *)&sched)) != 0) {
-        LOGE(ERROR, "getting domain info list");
+    r = xc_sched_id(ctx->xch, &sched);
+    if (r != 0) {
+        LOGE(ERROR, "getting current scheduler id");
         return ERROR_FAIL;
         GC_FREE;
     }
-- 
1.9.5.msysgit.0

[PATCH v2 2/5] libxl: make GC_FREE reachable in libxl_get_scheduler()

[Chester Lin]

Coverity CID 1343309

Make GC_FREE reachable in all cases in libxl_get_scheduler() by
eliminating the error-path return and instead storing the error code in
the returned variable.

To make this semantically consistent, change the return type of
libxl_get_scheduler() from libxl_scheduler to int, and make a note of
the interpretation of the return value in libxl.h.  N.B. This change
breaks neither the API nor the ABI of libxl.

Suggested-by: Ian Campbell <ian.campbell@citrix.com>
Signed-off-by: Chester Lin <czylin@uwaterloo.ca>

---
Changed return type of libxl_get_scheduler in order to return both negative
error constants and positive scheduler values.
---
 tools/libxl/libxl.c | 11 +++++------
 tools/libxl/libxl.h |  5 ++++-
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 7f28af8..96b6333 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5583,19 +5583,18 @@ out:
     return rc;
 }
 
-libxl_scheduler libxl_get_scheduler(libxl_ctx *ctx)
+int libxl_get_scheduler(libxl_ctx *ctx)
 {
-    int r, sched;
+    int r, ret;
 
     GC_INIT(ctx);
-    r = xc_sched_id(ctx->xch, &sched);
+    r = xc_sched_id(ctx->xch, &ret);
     if (r != 0) {
         LOGE(ERROR, "getting current scheduler id");
-        return ERROR_FAIL;
-        GC_FREE;
+        ret = ERROR_FAIL;
     }
     GC_FREE;
-    return sched;
+    return ret;
 }
 
 static int sched_arinc653_domain_set(libxl__gc *gc, uint32_t domid,
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 05606a7..6f53376 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -1702,7 +1702,10 @@ int libxl_domain_get_nodeaffinity(libxl_ctx *ctx, uint32_t domid,
                                   libxl_bitmap *nodemap);
 int libxl_set_vcpuonline(libxl_ctx *ctx, uint32_t domid, libxl_bitmap *cpumap);
 
-libxl_scheduler libxl_get_scheduler(libxl_ctx *ctx);
+/* A return value less than 0 should be interpreted as a libxl_error, while a
+ * return value greater than or equal to 0 should be interpreted as a
+ * libxl_scheduler. */
+int libxl_get_scheduler(libxl_ctx *ctx);
 
 /* Per-scheduler parameters */
 int libxl_sched_credit_params_get(libxl_ctx *ctx, uint32_t poolid,
-- 
1.9.5.msysgit.0

[PATCH v2 3/5] n16550: add sanity check for reg_shift

[Chester Lin]

Fix CID 1343302 by adding checking a check on the value of reg_shift.
This patch also rolls the multiplication by 8 into the shift.
No functional changes.

Suggested-by: Jan Beulich <JBeulich@suse.com>
Signed-off-by: Chester Lin <czylin@uwaterloo.ca>
---
 xen/drivers/char/ns16550.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index bc24015..55cfc45 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -913,7 +913,8 @@ pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int bar_idx)
                          * Force length of mmio region to be at least
                          * 8 bytes times (1 << reg_shift)
                          */
-                        if ( size < (0x8 * (1 << uart_param[p].reg_shift)) )
+                        if ( uart_param[p].reg_shift > 27 ||
+                             size < (1 << (uart_param[p].reg_shift + 3)) )
                             continue;
 
                         if ( bar_idx >= uart_param[p].max_bars )
-- 
1.9.5.msysgit.0

[PATCH v2 5/5] libxl: Add explicit cast to libxl_psr_cat_set_cbm

[Chester Lin]

Fixes Coverity CID 1343299. The call to xc_psr_cat_set_domain_data()
expects type xc_psr_cat_type but is provided libxl_psr_cbm_type which
is defined in IDL.

The two enums are deliberately identical and IDL only exists so that
libxl clients don't need to include libxc headers directly.

This change adds an explicit cast to fix the
Coverity warning, and tweaks the surrounding code to more closely
conform to the guidelines in CODING_STYLE.

No functional changes.

Signed-off-by: Chester Lin <czylin@uwaterloo.ca>
---

Changed commit message to say that the enums are identical

---
 tools/libxl/libxl_psr.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c
index 3d0dc61..1677f9c 100644
--- a/tools/libxl/libxl_psr.c
+++ b/tools/libxl/libxl_psr.c
@@ -298,7 +298,7 @@ int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t domid,
                           uint64_t cbm)
 {
     GC_INIT(ctx);
-    int rc;
+    int rc, r;
     int socketid, nr_sockets;
 
     rc = libxl__count_physical_sockets(gc, &nr_sockets);
@@ -310,7 +310,9 @@ int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t domid,
     libxl_for_each_set_bit(socketid, *target_map) {
         if (socketid >= nr_sockets)
             break;
-        if (xc_psr_cat_set_domain_data(ctx->xch, domid, type, socketid, cbm)) {
+        r = xc_psr_cat_set_domain_data(ctx->xch, domid, (xc_psr_cat_type) type,
+                                       socketid, cbm);
+        if (r) {
             libxl__psr_cat_log_err_msg(gc, errno);
             rc = ERROR_FAIL;
         }
@@ -326,11 +328,14 @@ int libxl_psr_cat_get_cbm(libxl_ctx *ctx, uint32_t domid,
                           uint64_t *cbm_r)
 {
     GC_INIT(ctx);
-    int rc = 0;
-
-    if (xc_psr_cat_get_domain_data(ctx->xch, domid, type, target, cbm_r)) {
+    int rc, r;
+    r = xc_psr_cat_get_domain_data(ctx->xch, domid, (xc_psr_cat_type) type,
+                                   target, cbm_r);
+    if (r) {
         libxl__psr_cat_log_err_msg(gc, errno);
         rc = ERROR_FAIL;
+    } else {
+        rc = 0;
     }
 
     GC_FREE;
-- 
1.9.5.msysgit.0

Re: [PATCH v2 5/5] libxl: Add explicit cast to libxl_psr_cat_set_cbm

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1546 bytes --]

On Tue, 2016-01-19 at 00:58 -0500, Chester Lin wrote:
> Fixes Coverity CID 1343299. The call to xc_psr_cat_set_domain_data()
> expects type xc_psr_cat_type but is provided libxl_psr_cbm_type which
> is defined in IDL.
> 
> The two enums are deliberately identical and IDL only exists so that
> libxl clients don't need to include libxc headers directly.
> 
I see...

> --- a/tools/libxl/libxl_psr.c
> +++ b/tools/libxl/libxl_psr.c
> @@ -310,7 +310,9 @@ int libxl_psr_cat_set_cbm(libxl_ctx *ctx,
> uint32_t domid,
>      libxl_for_each_set_bit(socketid, *target_map) {
>          if (socketid >= nr_sockets)
>              break;
> -        if (xc_psr_cat_set_domain_data(ctx->xch, domid, type,
> socketid, cbm)) {
> +        r = xc_psr_cat_set_domain_data(ctx->xch, domid,
> (xc_psr_cat_type) type,
> +                                       socketid, cbm);
> +        if (r) {
>
Is the cast in the function call better than a local variable of
xc_psr_cat_type initialized with 'type'? Or would Coverity keep
complaining in such a case?

If yes to either of the questions, this patch is:

Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH v2 2/5] libxl: make GC_FREE reachable in libxl_get_scheduler()

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1579 bytes --]

On Tue, 2016-01-19 at 00:57 -0500, Chester Lin wrote:
> Coverity CID 1343309
> 
> Make GC_FREE reachable in all cases in libxl_get_scheduler() by
> eliminating the error-path return and instead storing the error code
> in
> the returned variable.
> 
> To make this semantically consistent, change the return type of
> libxl_get_scheduler() from libxl_scheduler to int, and make a note of
> the interpretation of the return value in libxl.h.  N.B. This change
> breaks neither the API nor the ABI of libxl.
>
Not that I feel too strong about this, but I would reword this last
sentence a bit. In fact, ABI, AFAIK, we don't care. API, someone could
argue that it does actually break it, it's just the case that we don't
think it breaks it in any ways that we should care.

And maybe we should also add a note about the libxl_scheduler enum
being (and needing to continue to do so) consistent with what
xc_sched_id returns, like it's been done in another patch of this
series?

Anyway, that's all up for the tools maintainers to judge... The patch
seems to me to do what was asked during v1 review, so:

> Suggested-by: Ian Campbell <ian.campbell@citrix.com>
> Signed-off-by: Chester Lin <czylin@uwaterloo.ca>
> 
Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH v2 1/5] libxl: tidy libxl_get_scheduler() according to CODING_STYLE

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 992 bytes --]

On Tue, 2016-01-19 at 00:57 -0500, Chester Lin wrote:
> To more closely follow the guidelines in CODING_STYLE, store the
> result
> of xc_sched_id() in the local variable r, and the check the result of
> the call in a separate statement.  Change the type of the output
> parameter given to xc_sched_id() from libxl_scheduler to int to match
> the libxc interface.
> 
> Additionally, change the error log statement to more accurately
> reflect
> the failure.  This is the only functional change introduced by this
> patch.
> 
> Suggested-by: Ian Campbell <ian.campbell@citrix.com>
> Signed-off-by: Chester Lin <czylin@uwaterloo.ca>
> 
Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH v2 1/5] libxl: tidy libxl_get_scheduler() according to CODING_STYLE

[Wei Liu]

Hi Chester

What we normally do for new version of patches is to send out a new
series prefixed with "PATCH v2", instead of replying to old version of
the patches.

Could you collect Dario's Reviewed-by tags and send this series as v2.

Thanks
Wei.

Re: [PATCH v2 1/5] libxl: tidy libxl_get_scheduler() according to CODING_STYLE

[Ian Campbell]

On Tue, 2016-01-19 at 11:28 +0000, Wei Liu wrote:
> Hi Chester
> 
> What we normally do for new version of patches is to send out a new
> series prefixed with "PATCH v2", instead of replying to old version of
> the patches.
> 
> Could you collect Dario's Reviewed-by tags and send this series as v2.

Please call the resend v3 to avoid any confusion.

http://wiki.xen.org/wiki/Submitting_Xen_Patches#Review.2C_Rinse_.26_Repeat 
also has some words on this topic.

Ian.

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

Re: [PATCH v2 3/5] n16550: add sanity check for reg_shift

[Jan Beulich]

>>> On 19.01.16 at 06:57, <czylin@uwaterloo.ca> wrote:
> Fix CID 1343302 by adding checking a check on the value of reg_shift.
> This patch also rolls the multiplication by 8 into the shift.
> No functional changes.
> 
> Suggested-by: Jan Beulich <JBeulich@suse.com>

I don't think so.

> Signed-off-by: Chester Lin <czylin@uwaterloo.ca>
> ---
>  xen/drivers/char/ns16550.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index bc24015..55cfc45 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -913,7 +913,8 @@ pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int bar_idx)
>                           * Force length of mmio region to be at least
>                           * 8 bytes times (1 << reg_shift)
>                           */
> -                        if ( size < (0x8 * (1 << uart_param[p].reg_shift)) )
> +                        if ( uart_param[p].reg_shift > 27 ||
> +                             size < (1 << (uart_param[p].reg_shift + 3)) )
>                              continue;

Instead I think Coverity complaining is mad, and adding a
comparison here just clutters the code. The only thing I could
imagine I might have suggested would be to put an ASSERT()
here.

In any event should is the replacement of the multiplication
by an addition not what I think I had also mentioned before:
The expression, if changed in the first place, should use 8 as
the left operand of the shift.

Jan

Re: [PATCH v2 5/5] libxl: Add explicit cast to libxl_psr_cat_set_cbm

[Ian Jackson]

Chester Lin writes ("[PATCH v2 5/5] libxl: Add explicit cast to libxl_psr_cat_set_cbm"):
> Fixes Coverity CID 1343299. The call to xc_psr_cat_set_domain_data()
> expects type xc_psr_cat_type but is provided libxl_psr_cbm_type which
> is defined in IDL.
> 
> The two enums are deliberately identical and IDL only exists so that
> libxl clients don't need to include libxc headers directly.
> 
> This change adds an explicit cast to fix the
> Coverity warning, and tweaks the surrounding code to more closely
> conform to the guidelines in CODING_STYLE.

I can see why Coverity is complaining.  I think, overall, that the
existing situation is not really desirable.


In fact there are not two but *three* of these enums:

 * XEN_DOMCTL_PSR_CAT_OP_SET_L3_* (public/domctl.h)
 * enum xc_psr_cat_type (xenctrl.h)
 * Enumeration("psr_cbm_type",...) (libxl_types.idl)

xc_psr.c explicitly converts between the first two with a switch
statement.  libxl does no conversion.


I think our general rule is that enums from the hypervisor public
headers are OK to expose to libxl users, because that avoids a pile of
needless translation.  Ian, Wei, do you agree ?

Of course in this particular case, we shouldn't expect libxl users to
consume XEN_DOMCTL_*.  Instead, I would have expected
XEN_DOMCTL_PSR_CAT_OP_SET with a separate enum
XEN_PSR_CAT_L3_* or something.


With the current setup there is no mechanism (computer- or
human-mediated) that checks that new values added to these enums
correspond.  And there is not even a comment that the values of the
libxl enum and the libxc enum need to be kept in step.


I am not a fan of the cast as a solution.  I would rather, prefer to
regularise the situation.  If my co-maintainers agree about the
desirability of expecting libxl callers to use enum values from Xen
public headers, then I would want to:

 * Change the hypervisor interface
 * Abolish the libxc and libxl enums
 * Provide a compatibility layer in libxl for users of the old
   enum value names and the old type names (do we need to keep
   the old enum in the IDL or does our API stability guarantee apply
   to the generated C bindings?)

Ian.

Re: [PATCH v2 2/5] libxl: make GC_FREE reachable in libxl_get_scheduler()

[Ian Jackson]

Chester Lin writes ("[PATCH v2 2/5] libxl: make GC_FREE reachable in libxl_get_scheduler()"):
> To make this semantically consistent, change the return type of
> libxl_get_scheduler() from libxl_scheduler to int, and make a note of
> the interpretation of the return value in libxl.h.  N.B. This change
> breaks neither the API nor the ABI of libxl.

This is as we discussed, I think, thanks.

One nit:

> -libxl_scheduler libxl_get_scheduler(libxl_ctx *ctx)
> +int libxl_get_scheduler(libxl_ctx *ctx)
>  {
> -    int r, sched;
> +    int r, ret;

I see that CODING_STYLE doesn't discuss this directly, but I'm not
very keen on the use of `ret' for this variable name.  There is quite
a lot of code elsewhere where `ret' is used simply for libxl error
codes.

I think the name `sched' is fine and that here

> -        return ERROR_FAIL;
> -        GC_FREE;
> +        ret = ERROR_FAIL;

  +        sched = ERROR_FAIL;

would be fine.

(But this is a bikeshed style issue that others may disagree about so
please let's see what they think before you rework the patch...)

Ian.

Re: [PATCH v2 5/5] libxl: Add explicit cast to libxl_psr_cat_set_cbm

[Ian Campbell]

On Tue, 2016-01-19 at 14:06 +0000, Ian Jackson wrote:
> I am not a fan of the cast as a solution.  I would rather, prefer to
> regularise the situation.  If my co-maintainers agree about the
> desirability of expecting libxl callers to use enum values from Xen
> public headers,

libxl_shutdown_reason has the same issues, libxl_tsc_mode also might, as
might libxl_timer_mode. I'm not sure if there are others. I think we
generally handle all these the way psr is handled today (with casts and/or
explicit conversion switches).

I think part of the problem is that it is hard to expose just the desired
bits through to the user of libxl without also exposing the full xen
hypercall interface (the vast majority of which would be inappropriate to
expose to them since libxl is suppose to encapsulate such things).

If we can find a way to allow this without that downside and while
preserving API compat that would be ok.

>  then I would want to:
> 
>  * Change the hypervisor interface
>  * Abolish the libxc and libxl enums
>  * Provide a compatibility layer in libxl for users of the old
>    enum value names and the old type names (do we need to keep
>    the old enum in the IDL or does our API stability guarantee apply
>    to the generated C bindings?)

I think we do need to keep the compat, yes, the fact that bit of the C API
is autogenerated makes no difference to the end user. The compat API is
only needed for callers who define LIBXL_API_VERSION to some older version
though (I think, do check the libxl.h comments in case I'm misremembering
what we say).

As a possible alternative, we could make it such that the IDL generator
knows about the linkage and enforces the use of the same values, and
automatically provides conversion helpers (essential a cast wrapped in some
syntax) for _internal_ use.

Ian.

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

Re: [PATCH v2 5/5] libxl: Add explicit cast to libxl_psr_cat_set_cbm

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1171 bytes --]

On Tue, 2016-01-19 at 14:21 +0000, Ian Campbell wrote:
> On Tue, 2016-01-19 at 14:06 +0000, Ian Jackson wrote:
> > I am not a fan of the cast as a solution.  I would rather, prefer
> > to
> > regularise the situation.  If my co-maintainers agree about the
> > desirability of expecting libxl callers to use enum values from Xen
> > public headers,
> 
> libxl_shutdown_reason has the same issues, libxl_tsc_mode also might,
> as
> might libxl_timer_mode. I'm not sure if there are others. 
>
(FTR) libxl_scheduler too, probably?

> I think we
> generally handle all these the way psr is handled today (with casts
> and/or
> explicit conversion switches).
> 
and in fact, libxl_scheduler --touched in another patch of this
series-- is also handled in a similar way, and in that case, you seemed
to be fine with it? (and I'm not complaining, I'm just genuinely
confused :-) )

Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH v2 5/5] libxl: Add explicit cast to libxl_psr_cat_set_cbm

[George Dunlap]

On 19/01/16 14:21, Ian Campbell wrote:
> On Tue, 2016-01-19 at 14:06 +0000, Ian Jackson wrote:
>> I am not a fan of the cast as a solution.  I would rather, prefer to
>> regularise the situation.  If my co-maintainers agree about the
>> desirability of expecting libxl callers to use enum values from Xen
>> public headers,
<snip>
> I think part of the problem is that it is hard to expose just the desired
> bits through to the user of libxl without also exposing the full xen
> hypercall interface (the vast majority of which would be inappropriate to
> expose to them since libxl is suppose to encapsulate such things).

I agree with this sentiment...

> As a possible alternative, we could make it such that the IDL generator
> knows about the linkage and enforces the use of the same values, and
> automatically provides conversion helpers (essential a cast wrapped in some
> syntax) for _internal_ use.

...and using the IDL to enforce or generate values was my first thought.

 -George

Re: [PATCH v2 5/5] libxl: Add explicit cast to libxl_psr_cat_set_cbm

[Ian Campbell]

On Tue, 2016-01-19 at 14:06 +0000, Ian Jackson wrote:
> Chester Lin writes ("[PATCH v2 5/5] libxl: Add explicit cast to
> libxl_psr_cat_set_cbm"):
> > Fixes Coverity CID 1343299. The call to xc_psr_cat_set_domain_data()
> > expects type xc_psr_cat_type but is provided libxl_psr_cbm_type which
> > is defined in IDL.
> > 
> > The two enums are deliberately identical and IDL only exists so that
> > libxl clients don't need to include libxc headers directly.
> > 
> > This change adds an explicit cast to fix the
> > Coverity warning, and tweaks the surrounding code to more closely
> > conform to the guidelines in CODING_STYLE.
> 
> I can see why Coverity is complaining.  I think, overall, that the
> existing situation is not really desirable.
> 
> 
> In fact there are not two but *three* of these enums:
> 
>  * XEN_DOMCTL_PSR_CAT_OP_SET_L3_* (public/domctl.h)
>  * enum xc_psr_cat_type (xenctrl.h)
>  * Enumeration("psr_cbm_type",...) (libxl_types.idl)

Forgot to say in my other reply, but we could try and abolish at least the
xc one and have libxl internally use the domctl values.

I should also have said I think all of this is a bit much to ask Chester to
tackle given that the intro[0] explains that the Coverity stuff is just in
order to gain some familiarity before embarking on a "proper" project.

Ian.

[0] http://lists.xen.org/archives/html/xen-devel/2015-12/msg00629.html


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

Re: [PATCH v2 5/5] libxl: Add explicit cast to libxl_psr_cat_set_cbm

[Ian Jackson]

Dario Faggioli writes ("Re: [PATCH v2 5/5] libxl: Add explicit cast to libxl_psr_cat_set_cbm"):
> On Tue, 2016-01-19 at 14:21 +0000, Ian Campbell wrote:
> > libxl_shutdown_reason has the same issues, libxl_tsc_mode also might,
> > as might libxl_timer_mode. I'm not sure if there are others. 
> >
> (FTR) libxl_scheduler too, probably?

Quite possibly.  Hrm.

I see that for shutdown_reason Ian C did this deliberately in "libxl:
Remove xen/sched.h from public interface".

I am really not a fan of this approach.  I don't remember what I said
about that at the time.  The result is multiple enums with the same
values in that need to be kept in step.

However, I'm not sure I can persuade everyone to agree on an
alternative.  At the very least we should document the rules in
CODING_STYLE.

Ian.

Re: [PATCH v2 5/5] libxl: Add explicit cast to libxl_psr_cat_set_cbm

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH v2 5/5] libxl: Add explicit cast to libxl_psr_cat_set_cbm"):
> On Tue, 2016-01-19 at 14:06 +0000, Ian Jackson wrote:
> >  * XEN_DOMCTL_PSR_CAT_OP_SET_L3_* (public/domctl.h)
> >  * enum xc_psr_cat_type (xenctrl.h)
> >  * Enumeration("psr_cbm_type",...) (libxl_types.idl)
> 
> Forgot to say in my other reply, but we could try and abolish at least the
> xc one and have libxl internally use the domctl values.

Yes.

I like George's IDL suggestion.

> I should also have said I think all of this is a bit much to ask Chester to
> tackle given that the intro[0] explains that the Coverity stuff is just in
> order to gain some familiarity before embarking on a "proper" project.

Well, quite!  Sorry to Chester for having suddenly revealed this
apparently-simple bug to be a swamp.

Ian.

Re: [PATCH v2 3/5] n16550: add sanity check for reg_shift

[czylin]

Quoting Jan Beulich <JBeulich@suse.com>:

>>>> On 19.01.16 at 06:57, <czylin@uwaterloo.ca> wrote:
>> Fix CID 1343302 by adding checking a check on the value of reg_shift.
>> This patch also rolls the multiplication by 8 into the shift.
>> No functional changes.
>>
>> Suggested-by: Jan Beulich <JBeulich@suse.com>
>
> I don't think so.
>
>> Signed-off-by: Chester Lin <czylin@uwaterloo.ca>
>> ---
>>  xen/drivers/char/ns16550.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
>> index bc24015..55cfc45 100644
>> --- a/xen/drivers/char/ns16550.c
>> +++ b/xen/drivers/char/ns16550.c
>> @@ -913,7 +913,8 @@ pci_uart_config(struct ns16550 *uart, bool_t  
>> skip_amt, unsigned int bar_idx)
>>                           * Force length of mmio region to be at least
>>                           * 8 bytes times (1 << reg_shift)
>>                           */
>> -                        if ( size < (0x8 * (1 <<  
>> uart_param[p].reg_shift)) )
>> +                        if ( uart_param[p].reg_shift > 27 ||
>> +                             size < (1 << (uart_param[p].reg_shift + 3)) )
>>                              continue;
>
> Instead I think Coverity complaining is mad, and adding a
> comparison here just clutters the code. The only thing I could
> imagine I might have suggested would be to put an ASSERT()
> here.
>
> In any event should is the replacement of the multiplication
> by an addition not what I think I had also mentioned before:
> The expression, if changed in the first place, should use 8 as
> the left operand of the shift.
>
> Jan
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>

Sorry for the confusion regarding the suggested-by tag.

Thank you for reviewing our patches. We agree that it would make
more sense to suppress the error in Coverity. As such, we will
not be sending this patch for further review.

Regards,
Chester Lin

Re: [PATCH v2 5/5] libxl: Add explicit cast to libxl_psr_cat_set_cbm

[George Dunlap]

On Tue, Jan 19, 2016 at 2:35 PM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> Ian Campbell writes ("Re: [PATCH v2 5/5] libxl: Add explicit cast to libxl_psr_cat_set_cbm"):
>> On Tue, 2016-01-19 at 14:06 +0000, Ian Jackson wrote:
>> >  * XEN_DOMCTL_PSR_CAT_OP_SET_L3_* (public/domctl.h)
>> >  * enum xc_psr_cat_type (xenctrl.h)
>> >  * Enumeration("psr_cbm_type",...) (libxl_types.idl)
>>
>> Forgot to say in my other reply, but we could try and abolish at least the
>> xc one and have libxl internally use the domctl values.
>
> Yes.
>
> I like George's IDL suggestion.

Out of curiosity, did anything ever come of this?  If not it seems
like we should write it down somewhere.

 -George

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

Re: [PATCH v2 5/5] libxl: Add explicit cast to libxl_psr_cat_set_cbm

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1180 bytes --]

On Thu, 2017-01-12 at 18:08 +0000, George Dunlap wrote:
> On Tue, Jan 19, 2016 at 2:35 PM, Ian Jackson
> <Ian.Jackson@eu.citrix.com> wrote:
> > 
> > > On Tue, 2016-01-19 at 14:06 +0000, Ian Jackson wrote:
> > > > 
> > > >  * XEN_DOMCTL_PSR_CAT_OP_SET_L3_* (public/domctl.h)
> > > >  * enum xc_psr_cat_type (xenctrl.h)
> > > >  * Enumeration("psr_cbm_type",...) (libxl_types.idl)
> > > 
> > > Forgot to say in my other reply, but we could try and abolish at
> > > least the
> > > xc one and have libxl internally use the domctl values.
> > 
> > Yes.
> > 
> > I like George's IDL suggestion.
> 
> Out of curiosity, did anything ever come of this?  
>
AFAICT, no. The patch with the casts was not taken, and we still have
all the three enums/types in Xen, libxc and libxl.

> If not it seems
> like we should write it down somewhere.
> 
Indeed, especially because it was a good idea.

Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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