| Message ID | 58205668020000780011C95E@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 07.11.16 at 10:24, <JBeulich@suse.com> wrote: > --- a/xen/drivers/passthrough/io.c > +++ b/xen/drivers/passthrough/io.c > @@ -165,7 +165,11 @@ static void pt_irq_time_out(void *data) > spin_lock(&irq_map->dom->event_lock); > > dpci = domain_get_irq_dpci(irq_map->dom); > - ASSERT(dpci); > + if ( unlikely(!dpci) ) > + { > + ASSERT_UNREACHABLE(); > + return; > + } > list_for_each_entry ( digl, &irq_map->digl_list, list ) > { > unsigned int guest_gsi = hvm_pci_intx_gsi(digl->device, digl->intx); > @@ -793,7 +797,11 @@ void hvm_dpci_msi_eoi(struct domain *d, > > static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci) > { > - ASSERT(d->arch.hvm_domain.irq.dpci); > + if ( unlikely(!d->arch.hvm_domain.irq.dpci) ) > + { > + ASSERT_UNREACHABLE(); > + return; > + } I wonder, btw, whether we shouldn't ease these by making a macro along the lines of #define ASSERT_BAIL(cond, retval...) do { \ if ( unlikely(!(cond)) ) { ASSERT_UNREACHABLE(); return retval; } \ } while (0) Opinions? Jan
On 07/11/16 09:24, Jan Beulich wrote: > Avoid NULL derefs on non-debug builds. > > Coverity ID: 1055650 > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> > > --- a/xen/drivers/passthrough/io.c > +++ b/xen/drivers/passthrough/io.c > @@ -165,7 +165,11 @@ static void pt_irq_time_out(void *data) > spin_lock(&irq_map->dom->event_lock); > > dpci = domain_get_irq_dpci(irq_map->dom); > - ASSERT(dpci); > + if ( unlikely(!dpci) ) > + { > + ASSERT_UNREACHABLE(); > + return; > + } > list_for_each_entry ( digl, &irq_map->digl_list, list ) > { > unsigned int guest_gsi = hvm_pci_intx_gsi(digl->device, digl->intx); > @@ -793,7 +797,11 @@ void hvm_dpci_msi_eoi(struct domain *d, > > static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci) > { > - ASSERT(d->arch.hvm_domain.irq.dpci); > + if ( unlikely(!d->arch.hvm_domain.irq.dpci) ) > + { > + ASSERT_UNREACHABLE(); > + return; > + } > > spin_lock(&d->event_lock); > if ( test_and_clear_bool(pirq_dpci->masked) ) > > >
On Mon, Nov 07, 2016 at 10:30:37AM +0000, Andrew Cooper wrote: > On 07/11/16 09:24, Jan Beulich wrote: > > Avoid NULL derefs on non-debug builds. > > > > Coverity ID: 1055650 > > > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> > Release-acked-by: Wei Liu <wei.liu2@citrix.com>
On 07/11/16 09:53, Jan Beulich wrote: >>>> On 07.11.16 at 10:24, <JBeulich@suse.com> wrote: >> --- a/xen/drivers/passthrough/io.c >> +++ b/xen/drivers/passthrough/io.c >> @@ -165,7 +165,11 @@ static void pt_irq_time_out(void *data) >> spin_lock(&irq_map->dom->event_lock); >> >> dpci = domain_get_irq_dpci(irq_map->dom); >> - ASSERT(dpci); >> + if ( unlikely(!dpci) ) >> + { >> + ASSERT_UNREACHABLE(); >> + return; >> + } >> list_for_each_entry ( digl, &irq_map->digl_list, list ) >> { >> unsigned int guest_gsi = hvm_pci_intx_gsi(digl->device, digl->intx); >> @@ -793,7 +797,11 @@ void hvm_dpci_msi_eoi(struct domain *d, >> >> static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci) >> { >> - ASSERT(d->arch.hvm_domain.irq.dpci); >> + if ( unlikely(!d->arch.hvm_domain.irq.dpci) ) >> + { >> + ASSERT_UNREACHABLE(); >> + return; >> + } > I wonder, btw, whether we shouldn't ease these by making a macro > along the lines of > > #define ASSERT_BAIL(cond, retval...) do { \ > if ( unlikely(!(cond)) ) { ASSERT_UNREACHABLE(); return retval; } \ > } while (0) > > Opinions? On the one hand, this is becoming a common pattern. On the other, I really dislike hiding control flow in a macro like this. It might be ok if named ASSERT_UNREACHABLE_RETURN() to both highlight that it is an ASSERT_UNREACHABLE() rather than an ASSERT() of the condition passed. Perhaps ASSERT_UNREACHABLE_RETURN_IF() to avoid mixing up the types of assertion? ~Andrew
>>> On 07.11.16 at 11:43, <andrew.cooper3@citrix.com> wrote: > On 07/11/16 09:53, Jan Beulich wrote: >>>>> On 07.11.16 at 10:24, <JBeulich@suse.com> wrote: >>> --- a/xen/drivers/passthrough/io.c >>> +++ b/xen/drivers/passthrough/io.c >>> @@ -165,7 +165,11 @@ static void pt_irq_time_out(void *data) >>> spin_lock(&irq_map->dom->event_lock); >>> >>> dpci = domain_get_irq_dpci(irq_map->dom); >>> - ASSERT(dpci); >>> + if ( unlikely(!dpci) ) >>> + { >>> + ASSERT_UNREACHABLE(); >>> + return; >>> + } >>> list_for_each_entry ( digl, &irq_map->digl_list, list ) >>> { >>> unsigned int guest_gsi = hvm_pci_intx_gsi(digl->device, digl->intx); >>> @@ -793,7 +797,11 @@ void hvm_dpci_msi_eoi(struct domain *d, >>> >>> static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci) >>> { >>> - ASSERT(d->arch.hvm_domain.irq.dpci); >>> + if ( unlikely(!d->arch.hvm_domain.irq.dpci) ) >>> + { >>> + ASSERT_UNREACHABLE(); >>> + return; >>> + } >> I wonder, btw, whether we shouldn't ease these by making a macro >> along the lines of >> >> #define ASSERT_BAIL(cond, retval...) do { \ >> if ( unlikely(!(cond)) ) { ASSERT_UNREACHABLE(); return retval; } \ >> } while (0) >> >> Opinions? > > On the one hand, this is becoming a common pattern. On the other, I > really dislike hiding control flow in a macro like this. > > It might be ok if named ASSERT_UNREACHABLE_RETURN() to both highlight > that it is an ASSERT_UNREACHABLE() rather than an ASSERT() of the > condition passed. Perhaps ASSERT_UNREACHABLE_RETURN_IF() to avoid > mixing up the types of assertion? That would end up being (taking one of the examples above) static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci) { ASSERT_UNREACHABLE_RETURN_IF(d->arch.hvm_domain.irq.dpci); ... or static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci) { ASSERT_UNREACHABLE_RETURN_IF(!d->arch.hvm_domain.irq.dpci); ... No matter which way I take it, I find it confusing: Is the condition meant to be ASSERT()-like or if()-like? If hiding control flow keywords in a macro looks bad to you, how about #define ASSERT_BAIL(cond, stmt...) do { \ if ( unlikely(!(cond)) ) { ASSERT_UNREACHABLE(); stmt; } \ } while (0) i.e. forcing the "return" to be visible at the macro invocation site (and at once allowing e.g. using "break" or "goto" there too)? It might even be worthwhile calling it ASSERT_CLEANUP() or some such, making it more logical to use even non-control-flow statements there (like setting a variable to a certain value). Jan
On Mon, Nov 07, 2016 at 10:30:37AM +0000, Andrew Cooper wrote: > On 07/11/16 09:24, Jan Beulich wrote: > > Avoid NULL derefs on non-debug builds. > > > > Coverity ID: 1055650 > > > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> This introduces a bug: > > > > > --- a/xen/drivers/passthrough/io.c > > +++ b/xen/drivers/passthrough/io.c > > @@ -165,7 +165,11 @@ static void pt_irq_time_out(void *data) > > spin_lock(&irq_map->dom->event_lock); > > > > dpci = domain_get_irq_dpci(irq_map->dom); > > - ASSERT(dpci); > > + if ( unlikely(!dpci) ) > > + { > > + ASSERT_UNREACHABLE(); > > + return; As this does not unlock the 'event_lock' spinlock. But with the ASSERT_UNREACHABLE() we just spin around. Perhaps a better option would be just to add an 'unlock' label and then goto to it? (And ditch the ASSERT altogether?) > > + } > > list_for_each_entry ( digl, &irq_map->digl_list, list ) > > { > > unsigned int guest_gsi = hvm_pci_intx_gsi(digl->device, digl->intx); > > @@ -793,7 +797,11 @@ void hvm_dpci_msi_eoi(struct domain *d, > > > > static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci) > > { > > - ASSERT(d->arch.hvm_domain.irq.dpci); > > + if ( unlikely(!d->arch.hvm_domain.irq.dpci) ) > > + { > > + ASSERT_UNREACHABLE(); > > + return; > > + } > > > > spin_lock(&d->event_lock); > > if ( test_and_clear_bool(pirq_dpci->masked) ) > > > > > > >
--- a/xen/drivers/passthrough/io.c +++ b/xen/drivers/passthrough/io.c @@ -165,7 +165,11 @@ static void pt_irq_time_out(void *data) spin_lock(&irq_map->dom->event_lock); dpci = domain_get_irq_dpci(irq_map->dom); - ASSERT(dpci); + if ( unlikely(!dpci) ) + { + ASSERT_UNREACHABLE(); + return; + } list_for_each_entry ( digl, &irq_map->digl_list, list ) { unsigned int guest_gsi = hvm_pci_intx_gsi(digl->device, digl->intx); @@ -793,7 +797,11 @@ void hvm_dpci_msi_eoi(struct domain *d, static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci) { - ASSERT(d->arch.hvm_domain.irq.dpci); + if ( unlikely(!d->arch.hvm_domain.irq.dpci) ) + { + ASSERT_UNREACHABLE(); + return; + } spin_lock(&d->event_lock); if ( test_and_clear_bool(pirq_dpci->masked) )