Nir Soffer has posted comments on this change.

Change subject: qemuimg: Explicit run semantics for QemuImgOperation

Patch Set 5: Code-Review-1

File lib/vdsm/

Line 276
Line 277
Line 278
Line 279
Line 280
This will break now if called before the command was run.

Can be fixed by:

   return self._command and self._command.poll() is not None

Line 278
Line 279
Line 280
Line 281
Line 282
We need to make this private, other code should not call this.

Line 300
Line 301
Line 302
Line 303
Line 304
Lets document that this is the only method that may be called from another 

Line 301
Line 302
Line 303
Line 304
Line 305
This will raise AttributeError if called before the command was run (e.g. abort 
 called in the same time job is run.

Line 234:             with_nice=utils.NICENESS.HIGH,
Line 235:             with_ioclass=utils.IOCLASS.IDLE)
Line 236:         self.cwd = cwd
Line 237:         self._command = None
Line 238:         self._stream = None
When you create the operation, self._command is None...
Line 239: 
Line 240:     def _recvstderr(self, buffer):
Line 241:         self._stderr += buffer
Line 242: 

Line 303:     def _start(self):
Line 304:         # Needed for tests.  Should not be used directly.
Line 305:         _log.debug(cmdutils.command_log_line(self.cmd, cwd=self.cwd))
Line 306:         self._command = CPopen(self.cmd, cwd=self.cwd,
Line 307:                                deathSignal=signal.SIGKILL)
Before we start the process we must make sure that we were no aborted, and we 
must make sure that we are not aborted after we check but before we start the 
command and assign to self._command.

This is very similar to what we do in copy_data.Job.

I guess we need:

    def _start(self):
        with self._lock:
            if self._aborted:
                raise ActionStopped
            self._command = ...
        self._stream = ...

Now we switch from pending  to running state without races.

And we must take the same lock in abort:

    def abort(self):
        with self._lock:
            self._aborted = True
            if self.command and self.command.poll() is None:

terminate is async, invoke kill(2), and we are waiting in run().
Line 308:         self._stream = utils.CommandStream(
Line 309:             self._command, self._recvstdout, self._recvstderr)
Line 310: 
Line 311:     def abort(self):
File tests/

Line 313
Line 314
Line 315
Line 316
Line 317
Why not use run() for this test? then we can avoid calling private methods.

Line 318:         self.assertRaises(qemuimg.QImgError, p.poll)
Line 319: 
Line 320:     def test_progress_simple(self):
Line 321:         p = qemuimg.QemuImgOperation(['true'])
Line 322:         p._start()
This test is bad, we should realy test that output received from a process is 
handled properly instead using private methods. This test make it harder to 
change the code instead of helping.

But this is not related to your patch so lets improve this later.
Line 323: 
Line 324:         for progress in self._progress_iterator():
Line 325:             p._recvstdout(self.PROGRESS_FORMAT % progress)
Line 326:             self.assertEquals(p.progress, progress)

Line 437:             top = os.path.join(tmpdir, "top.img")
Line 438:             make_image(top, size, qemuimg.FORMAT.QCOW2, 1, "1.1", 
Line 439: 
Line 440:             op = qemuimg.commit(top, topFormat=qemuimg.FORMAT.QCOW2)
Line 441:   
Much nicer.
Line 442:             self.assertEquals(100, op.progress)
Line 443: 
Line 444: 
Line 445: def make_image(path, size, format, index, qcow2_compat, backing=None):
File vdsm/storage/

Line 130
Line 131
Line 132
Line 133
Line 134
I think we should rename this to _run_qemuimg_operation. Can be done later if 
you want.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I8e427c909a8dffc3ba2a5c838c7d1e7ce7cce55d
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <>
Gerrit-Reviewer: gerrit-hooks <>
Gerrit-HasComments: Yes
vdsm-patches mailing list --
To unsubscribe send an email to

Reply via email to