Hi Frank, Frank Batschulat (Home) wrote: > On Sat, 23 May 2009 20:12:05 +0200, Dai Ngo <Dai.Ngo at sun.com> wrote: > > >> I'd like to have code review for a minor change to fix 6676419. >> >> The panic was caused by dereferencing a NULL pointer when the >> server returned an error for the getattr op in the compound request. >> >> The fix is to add a check, proposed in the "Suggested Fix" section >> of the CR, of the status of the op before accessing its reply result. >> >> The webrev is here: >> http://cr.opensolaris.org/~dain/6676419/ >> > > looks sane to me, but I do wonder why a simple, preceding > > if (res.status == NFS4_OK) { > 3114 /* getattr lease_time res */ > 3115 if (res.array_len >= 2) { > 3117 garp = &res.array[1].nfs_resop4_u.opgetattr.ga_res; > .... > > is not being used instead. for the sake of nfs4setclientid_otw() > it does not matter to know if the OP_PUTROOTFH _or_ the OP_GETATTR failed in > detail, > in either case we do not want to look at possibly existing GETATTR results. > ie. if PUTROOTFH already failed, there's shouldn't be anything to expect from > the > following GETATTR at least for my understanding. > The reason I did not do this way because np->s_lease_time and mi->mi_lease_period need to be updated even when OP_SETCLIENTID failed but OP_GETATTR is OK.
Thanks, -Dai > I'd favor fixing it this way. > > --- > frankB > > > >
