Re: [PATCH 2/2] virdevmapper: Deal with unloading dm module
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.