On Tue, 8 Mar 2022 10:39:09 -0500 "Jason J. Herne" <jjherne@xxxxxxxxxxxxx> wrote: > On 3/7/22 07:31, Tony Krowiak wrote: > >>> + * If the input apm and aqm belong to the matrix_mdev's matrix, > >>> + * then move on to the next. > >>> + */ > >>> + if (mdev_apm == matrix_mdev->matrix.apm && > >>> + mdev_aqm == matrix_mdev->matrix.aqm) > >>> continue; > >> > >> We may have a problem here. This check seems like it exists to stop you from > >> comparing an mdev's apm/aqm with itself. Obviously comparing an mdev's newly > >> updated apm/aqm with itself would cause a false positive sharing check, right? > >> If this is the case, I think the comment should be changed to reflect that. > > > > You are correct, this check is performed to prevent comparing an mdev to > > itself, I'll improve the comment. > > > >> > >> Aside from the comment, what stops this particular series of if statements from > >> allowing us to configure a second mdev with the exact same apm/aqm values as an > >> existing mdev? If we do, then this check's continue will short circuit the rest > >> of the function thereby allowing that 2nd mdev even though it should be a > >> sharing violation. > > > > I don't see how this is possible. > > You are correct. I missed the fact that you are comparing pointers here, and not > values. Apologies. Now that I understand the code, I agree that this is fine as is. > I believe clarifying the 'belongs to' vs 'is a part of' stuff is still worthwhile, because 'belongs to' does beg the question you asked. Thus IMHO it is good that you raised the question. Regards, Halil
