On Sun, Oct 13, 2024 at 01:03:45PM +0530, Umang Jain wrote:
> > > + spin_unlock(&service->state->bulk_waiter_spinlock);
> > > +
> > > + status = 0;
> > This just looks odd here. If it weren't for this I'd have probably been
> > fine with the initialisation of status
> >
> > > + } else if (bulk->mode == VCHIQ_BULK_MODE_CALLBACK) {
> > > + enum vchiq_reason reason = get_bulk_reason(bulk);
> > > + status = make_service_callback(service, reason, NULL,
> > > + bulk->userdata);
> > I think I would probably just drop the int status altogether and make this
> >
> > return make_service_callback(service, reason, NULL,
> > bulk->userdata);
> >
> > > + }
> > > +
> > > + return status;
> > And return 0 here. Then we get rid of the awkward initialisation and
> > usages above.
>
> I usually have the tendency to minimise return statements in a routine and
> ideally target for single return statement at the end.
I feel like the "one return per function" style rule is an anti-pattern. I
feel like it's better to handle errors right away. Then the code which is
indented one tab is the success path and the code which is indented more is
the edge cases.
>
> But I do agree on the awkward initialisation of status = 0
I sent my email and then I thought. Actually the solution here is to do:
status = make_service_callback(service, reason, NULL,
bulk->userdata);
if (status)
return status;
}
return 0;
This handles the error right away and avoids mixing the error paths with the
success paths. Plus I like a big "return 0;" at the end of my functions.
I like Kieran's approach as well.
But, I see now that I have misread the function. I'm not sure what is the most
readable way to write it. Maybe:
int status = 0;
if (bulk->mode == VCHIQ_BULK_MODE_BLOCKING) {
...
} else if (bulk->mode == VCHIQ_BULK_MODE_CALLBACK) {
...
status = make_service_callback();
} else {
status = -EINVAL;
}
return status;
Probably whatever you decide is fine. You care more about this code than I do
for sure.
regards,
dan carpenter
