On Mon, Aug 17, 2020 at 18:02:04 +0200, Andrea Bolognani wrote:
> On Mon, 2020-08-17 at 17:28 +0200, Peter Krempa wrote:
> > On Mon, Aug 17, 2020 at 16:26:55 +0200, Michal Privoznik wrote:
> > > - if ((controlFD = virDMOpen()) < 0)
> > > + if ((controlFD = virDMOpen()) < 0) {
> > > + if (controlFD == -2) {
> > > + /* Devmapper was available but now it isn't. Somebody
> > > + * must have removed the module. Reset the major
> > > + * number we remember. */
> > > + virDMMajor = 0;
> >
> > This overwrites 'virDMMajor' without taking the mutex. 'unsigned int' on
> > x86_64 will be atomic, we shouldn't use a knowingly broken code pattern
> > without at least a proper explanation why it's okay.
>
> IIUC you're saying that you'd be okay with adding a comment that
> explains why this will work fine on x86_64. If so, I'd like to point
> out that libvirt supports many other architectures... Is the same
> pattern safe on those as well?
It would really depend on how the arguments in the explanation persuade
me that the non-obvious/weird/wrong-looking code is justified in that
particular scenario. If the justification doesn't include that it's safe
in every scenario I'd protest it anyways.
What I wanted to say is that I know that this works on x86 but it goes
against established patterns of lock usage. That was to prevent any
discussion in the direction "but it's safe to do in this case" from
happening without moving towards a better code pattern or explanation
why it's necessary in this case.
