On Mon, May 25, 2009 at 2:47 PM, Michael Goldish <mgoldish@xxxxxxxxxx> wrote:
>
> ----- "sudhir kumar" <smalikphy@xxxxxxxxx> wrote:
>
>> On Sun, May 24, 2009 at 9:16 PM, Michael Goldish <mgoldish@xxxxxxxxxx>
>> wrote:
>> > VM.create() does a few things (such as finding free ports) which are
>> not safe
>> > to execute in parallel. Use a lock file to make sure this doesn't
>> happen. The
>> > lock is released only after the VM is started or fails to start.
>> >
>> > Signed-off-by: Michael Goldish <mgoldish@xxxxxxxxxx>
>> > ---
>> > client/tests/kvm_runtest_2/kvm_vm.py | 85
>> +++++++++++++++++++---------------
>> > 1 files changed, 48 insertions(+), 37 deletions(-)
>> >
>> > diff --git a/client/tests/kvm_runtest_2/kvm_vm.py
>> b/client/tests/kvm_runtest_2/kvm_vm.py
>> > index 3ce2003..af06693 100644
>> > --- a/client/tests/kvm_runtest_2/kvm_vm.py
>> > +++ b/client/tests/kvm_runtest_2/kvm_vm.py
>> > @@ -3,6 +3,7 @@
>> > import time
>> > import socket
>> > import os
>> > +import fcntl
>> >
>> > import kvm_utils
>> > import kvm_log
>> > @@ -289,48 +290,58 @@ class VM:
>> > kvm_log.error("Actual MD5 sum differs from
>> expected one")
>> > return False
>> >
>> > - # Handle port redirections
>> > - redir_names = kvm_utils.get_sub_dict_names(params,
>> "redirs")
>> > - host_ports = kvm_utils.find_free_ports(5000, 6000,
>> len(redir_names))
>> > - self.redirs = {}
>> > - for i in range(len(redir_names)):
>> > - redir_params = kvm_utils.get_sub_dict(params,
>> redir_names[i])
>> > - guest_port = int(redir_params.get("guest_port"))
>> > - self.redirs[guest_port] = host_ports[i]
>> > -
>> > - # Find available VNC port, if needed
>> > - if params.get("display") == "vnc":
>> > - self.vnc_port = kvm_utils.find_free_port(5900, 6000)
>> > -
>> > - # Make qemu command
>> > - qemu_command = self.make_qemu_command()
>> > + # Make sure the following code is not executed by more than
>> one thread
>> > + # at the same time
>> > + lockfile = open("/tmp/kvm-autotest-vm-create.lock", "w+")
>> How do you handle an open failure?
>
> Obviously I don't, but I don't think there should be a reason for this to
> fail, unless you deliberately create a file with such a name and with no
> write permission. The mode "w+" creates the file if it doesn't exist.
> In the unlikely event of an open failure the test will fail with some
> Python exception.
>
> It may be a good idea to create the file in the test's local directory
> (test.bindir) instead of /tmp to prevent a symlink attack.
Agree.
>
>> > + fcntl.lockf(lockfile, fcntl.LOCK_EX)
>> What if other instance has locked the file at the moment. Definitely
>> you would not like to fail. You may want to wait for a while and try
>> agian.
>
> This is what fcntl.lockf() does -- it blocks until it can acquire the
> lock (it doesn't fail).
>
>> I feel the default behaviour should be a blocking one but
>> still you want to print the debug message
>> kvm_log.debug("Trying to acquire lock for port selection")
>> before getting the lock.
>
> This could be a good idea, but I'm not sure it's necessary, because while
> one process waits, another process is busy doing stuff and printing
> stuff, so the user never gets bored. Also, the time duration to wait is
> typically around 1 sec.
>
> In any case it shouldn't hurt too much to print debugging info, so I'll
> either resend this patch, or add to it in a future patch.
thanks.
>
>> >
>> > - # Is this VM supposed to accept incoming migrations?
>> > - if for_migration:
>> > - # Find available migration port
>> > - self.migration_port = kvm_utils.find_free_port(5200,
>> 6000)
>> > - # Add -incoming option to the qemu command
>> > - qemu_command += " -incoming tcp:0:%d" %
>> self.migration_port
>> > + try:
>> > + # Handle port redirections
>> > + redir_names = kvm_utils.get_sub_dict_names(params,
>> "redirs")
>> > + host_ports = kvm_utils.find_free_ports(5000, 6000,
>> len(redir_names))
>> > + self.redirs = {}
>> > + for i in range(len(redir_names)):
>> > + redir_params = kvm_utils.get_sub_dict(params,
>> redir_names[i])
>> > + guest_port = int(redir_params.get("guest_port"))
>> > + self.redirs[guest_port] = host_ports[i]
>> > +
>> > + # Find available VNC port, if needed
>> > + if params.get("display") == "vnc":
>> > + self.vnc_port = kvm_utils.find_free_port(5900,
>> 6000)
>> > +
>> > + # Make qemu command
>> > + qemu_command = self.make_qemu_command()
>> > +
>> > + # Is this VM supposed to accept incoming migrations?
>> > + if for_migration:
>> > + # Find available migration port
>> > + self.migration_port =
>> kvm_utils.find_free_port(5200, 6000)
>> > + # Add -incoming option to the qemu command
>> > + qemu_command += " -incoming tcp:0:%d" %
>> self.migration_port
>> > +
>> > + kvm_log.debug("Running qemu command:\n%s" %
>> qemu_command)
>> > + (status, pid, output) = kvm_utils.run_bg(qemu_command,
>> None, kvm_log.debug, "(qemu) ")
>> > +
>> > + if status:
>> > + kvm_log.debug("qemu exited with status %d" %
>> status)
>> > + kvm_log.error("VM could not be created -- qemu
>> command failed:\n%s" % qemu_command)
>> > + return False
>> >
>> > - kvm_log.debug("Running qemu command:\n%s" % qemu_command)
>> > - (status, pid, output) = kvm_utils.run_bg(qemu_command,
>> None, kvm_log.debug, "(qemu) ")
>> > + self.pid = pid
>> >
>> > - if status:
>> > - kvm_log.debug("qemu exited with status %d" % status)
>> > - kvm_log.error("VM could not be created -- qemu command
>> failed:\n%s" % qemu_command)
>> > - return False
>> > -
>> > - self.pid = pid
>> > + if not kvm_utils.wait_for(self.is_alive, timeout, 0,
>> 1):
>> > + kvm_log.debug("VM is not alive for some reason")
>> > + kvm_log.error("VM could not be created with
>> command:\n%s" % qemu_command)
>> > + self.destroy()
>> > + return False
>> >
>> > - if not kvm_utils.wait_for(self.is_alive, timeout, 0, 1):
>> > - kvm_log.debug("VM is not alive for some reason")
>> > - kvm_log.error("VM could not be created with
>> command:\n%s" % qemu_command)
>> > - self.destroy()
>> > - return False
>> > + kvm_log.debug("VM appears to be alive with PID %d" %
>> self.pid)
>> >
>> > - kvm_log.debug("VM appears to be alive with PID %d" %
>> self.pid)
>> > + return True
>> >
>> > - return True
>> > + finally:
>> > + fcntl.lockf(lockfile, fcntl.LOCK_UN)
>> > + lockfile.close()
>> >
>> > def send_monitor_cmd(self, command, block=True, timeout=20.0):
>> > """Send command to the QEMU monitor.
>> > --
>> > 1.5.4.1
>> >
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe kvm" in
>> > the body of a message to majordomo@xxxxxxxxxxxxxxx
>> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>> >
>>
>>
>>
>> --
>> Sudhir Kumar
>> --
>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Sudhir Kumar
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
