Hi Jose,
> > > diff --git a/lib/sdp.c b/lib/sdp.c
> > > index 822ec1a..ddf2d1a 100644
> > > --- a/lib/sdp.c
> > > +++ b/lib/sdp.c
> > > @@ -4621,3 +4621,113 @@ uint16_t sdp_gen_tid(sdp_session_t *session)
> > > {
> > > return session->tid++;
> > > }
> > > +
> > > +/*
> > > + * Set the supported features
> > > + */
> > > +int sdp_set_supp_feat(sdp_record_t *rec, const sdp_list_t *sf)
> > > +{
> > > + const sdp_list_t *p, *r;
> > > + sdp_data_t *feat, *seq_feat;
> > > + int seqlen, i, err;
> > > + void **seqDTDs, **seqVals;
> > > +
> > > + err = -1;
> >
> > what is this err = -1 business. That is pointless. See below.
>
> I use this err variable because i need to distinguish between the error and no
> error cases. I explained better below.
>
> >
> > > + i = 0;
> >
> > Move this to the place where it is actually used.
> >
> > > + seqlen = sdp_list_len(sf);
> > > + seqDTDs = malloc(seqlen * sizeof(void *));
> > > + if (!seqDTDs)
> > > + return err;
> >
> > This is return -1;
> >
> > > + seqVals = malloc(seqlen * sizeof(void *));
> > > + if (!seqVals) {
> > > + free(seqDTDs);
> > > + return err;
> >
> > And this, too. Just use return -1;
> >
> > > + }
> > > +
> > > + for (p = sf; p; p = p->next) {
> > > + int plen, j;
> > > + void **dtds, **vals;
> > > +
> > > + plen = sdp_list_len(p->data);
> > > + dtds = malloc(plen * sizeof(void *));
> > > + if (!dtds)
> > > + goto set_sup_feat_exit;
> > > + vals = malloc(plen * sizeof(void *));
> > > + if (!vals) {
> > > + free(dtds);
> > > + goto set_sup_feat_exit;
> > > + }
> > > + j = 0;
> > > + for (r = p->data; r; r = r->next) {
> > > + sdp_data_t *data = (sdp_data_t*)r->data;
> > > + dtds[j] = &data->dtd;
> > > + vals[j] = &data->val;
> > > + j++;
> > > + }
> > > + feat = sdp_seq_alloc(dtds, vals, plen);
> > > + free(dtds);
> > > + free(vals);
> > > + if (!feat)
> > > + goto set_sup_feat_exit;
> > > + seqDTDs[i] = &feat->dtd;
> > > + seqVals[i] = feat;
> > > + i++;
> > > + }
> > > + seq_feat = sdp_seq_alloc(seqDTDs, seqVals, seqlen);
> > > + if (!seq_feat)
> > > + goto set_sup_feat_exit;
> > > + sdp_attr_replace(rec, SDP_ATTR_SUPPORTED_FEATURES_LIST, seq_feat);
> > > +
> > > + err = 0;
> >
> > Replace this with return 0;
> >
> > > +
> > > +set_sup_feat_exit:
> >
> > Call this label "fail" to be consistent across the code.
>
> This part should be executed on error and on no error, because seqVals and
> seqDTDs must be freed in all the cases. Because of that I called it exit:
> instead of fail. May be it is better to repeat this two statements (I mean
> free statements) in order to keep readability and use return as you suggested,
> without any variable.
I see your point here. However this doesn't make the code more readable.
It actually does complicates the flow through it. A simple goto fail and
return -1 is easier to understand then trying to remember the err value
across such a complex function.
So just after sdp_attr_replace duplicated the two free calls and then
return 0. And the label as a pure failure case. While you have two extra
free calls, you have on less variable and a lot simple code path here.
Remember that if the code makes my brain hurt, it is too complex ;)
Regards
Marcel
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
