[PATCH v2] ioreq_broadcast(): accept partial broadcast success

[PATCH v2] ioreq_broadcast(): accept partial broadcast success

[Per Bilse]

Avoid incorrectly triggering an error when a broadcast buffered ioreq
is not handled by all registered clients, as long as the failure is
strictly because the client doesn't handle buffered ioreqs.

Signed-off-by: Per Bilse <per.bilse@citrix.com>
---
v2: Complete rethink with better information. A lot of simplicity was added.
---
 xen/common/ioreq.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/common/ioreq.c b/xen/common/ioreq.c
index 4617aef29b..568e7aea91 100644
--- a/xen/common/ioreq.c
+++ b/xen/common/ioreq.c
@@ -1317,7 +1317,8 @@ unsigned int ioreq_broadcast(ioreq_t *p, bool buffered)
 
     FOR_EACH_IOREQ_SERVER(d, id, s)
     {
-        if ( !s->enabled )
+        if ( !s->enabled || (buffered &&
+                    s->bufioreq_handling == HVM_IOREQSRV_BUFIOREQ_OFF) )
             continue;
 
         if ( ioreq_send(s, p, buffered) == IOREQ_STATUS_UNHANDLED )
-- 
2.31.1

Re: [PATCH v2] ioreq_broadcast(): accept partial broadcast success

[Paul Durrant]

On 06/12/2022 17:52, Per Bilse wrote:
> Avoid incorrectly triggering an error when a broadcast buffered ioreq
> is not handled by all registered clients, as long as the failure is
> strictly because the client doesn't handle buffered ioreqs.
> 
> Signed-off-by: Per Bilse <per.bilse@citrix.com>
> ---
> v2: Complete rethink with better information. A lot of simplicity was added.
> ---

Reviewed-by: Paul Durrant <paul@xen.org>

Re: [PATCH v2] ioreq_broadcast(): accept partial broadcast success

[Jan Beulich]

On 06.12.2022 18:52, Per Bilse wrote:
> --- a/xen/common/ioreq.c
> +++ b/xen/common/ioreq.c
> @@ -1317,7 +1317,8 @@ unsigned int ioreq_broadcast(ioreq_t *p, bool buffered)
>  
>      FOR_EACH_IOREQ_SERVER(d, id, s)
>      {
> -        if ( !s->enabled )
> +        if ( !s->enabled || (buffered &&
> +                    s->bufioreq_handling == HVM_IOREQSRV_BUFIOREQ_OFF) )

Nit: Bad indentation. Since inserting the missing blanks would make
the line too long, the expression wants re-wrapping. Can certainly
be done while committing.

Jan

Re: [PATCH v2] ioreq_broadcast(): accept partial broadcast success

[Jan Beulich]

(Cc to xen-devel@ re-added; I don't see why it was dropped)

On 07.12.2022 11:25, Per Bilse (3P) wrote:
> On 07/12/2022 09:05, Jan Beulich wrote:
>> On 06.12.2022 18:52, Per Bilse wrote:
>>> +        if ( !s->enabled || (buffered &&
>>> +                    s->bufioreq_handling == HVM_IOREQSRV_BUFIOREQ_OFF) )
>>
>> Nit: Bad indentation. Since inserting the missing blanks would make
>> the line too long, the expression wants re-wrapping. Can certainly
>> be done while committing.
> 
> Thanks for the feedback, but what should the indentation be (and where 
> would I find the information)?  I checked the coding style and it simply 
> says "Long line should be split at sensible places and the trailing 
> portions indented."

You'll find ample examples throughout the tree. Indentation of pieces
of expressions should be such that related pieces align. Hence in the
case here it could be

        if ( !s->enabled || (buffered &&
                             s->bufioreq_handling == HVM_IOREQSRV_BUFIOREQ_OFF) )

but since that's too long a line it'll want to be e.g.

        if ( !s->enabled ||
             (buffered && s->bufioreq_handling == HVM_IOREQSRV_BUFIOREQ_OFF) )

or

        if ( !s->enabled ||
             (buffered &&
              s->bufioreq_handling == HVM_IOREQSRV_BUFIOREQ_OFF) )

(of which I prefer the former, and that's what I'll convert to if I
end up committing this change).

Jan