Re: [Qemu-devel] [PATCH 1/3] backup: QEMU Backup Tool

[Stefan Hajnoczi <stefanha@gmail.com>]

On Wed, Aug 23, 2017 at 06:34:38PM +0530, Ishani Chugh wrote:
> +    def write_config(self):
> +        """
> +        Writes configuration to ini file.
> +        """
> +        config_file = open(self.config_file+".tmp", 'w')

Please use whitespace around arithmetic operators:

  self.config_file + ".tmp"

> +        self.config.write(config_file)
> +        config_file.flush()
> +        os.fsync(config_file.fileno())
> +        config_file.close()
> +        os.rename(self.config_file+".tmp", self.config_file)

self.config_file + ".tmp"

> +
> +    def get_socket_address(self, socket_address):
> +        """
> +        Return Socket address in form of string or tuple
> +        """
> +        if socket_address.startswith('tcp'):
> +            return (socket_address.split(':')[1],
> +                    int(socket_address.split(':')[2]))
> +        return socket_address.split(':', 2)[1]
> +
> +    def _full_backup(self, guest_name):
> +        """
> +        Performs full backup of guest
> +        """
> +        if guest_name not in self.config.sections():
> +            print("Cannot find specified guest", file=sys.stderr)
> +            sys.exit(1)
> +
> +        self.verify_guest_running(guest_name)
> +        connection = QEMUMonitorProtocol(
> +                                         self.get_socket_address(
> +                                             self.config[guest_name]['qmp']))
> +        connection.connect()
> +        cmd = {"execute": "transaction", "arguments": {"actions": []}}
> +        drive_list = []
> +        for key in self.config[guest_name]:
> +            if key.startswith("drive_"):
> +                drive = key[len('drive_'):]
> +                drive_list.append(drive)
> +                target = self.config[guest_name][key]
> +                sub_cmd = {"type": "drive-backup", "data": {"device": drive,
> +                                                            "target": target,
> +                                                            "sync": "full"}}
> +                cmd['arguments']['actions'].append(sub_cmd)
> +        qmp_return = connection.cmd_obj(cmd)
> +        if 'error' in qmp_return:
> +            print(qmp_return['error']['desc'], file=sys.stderr)
> +            sys.exit(1)
> +        print("Backup Started")
> +        while len(drive_list) != 0:

PEP8 says:

  For sequences, (strings, lists, tuples), use the fact that empty sequences are false.

  Yes: if not seq:
       if seq:

  No: if len(seq):
      if not len(seq):

This statement should be:

  while drive_list:

> +            event = connection.pull_event(wait=True)
> +            print(event)

Please remove debugging or add a --verbose command-line option that
makes the print statement conditional:

  if verbose:
      print(event)

> +            if event['event'] == 'SHUTDOWN':
> +                print("The guest was SHUT DOWN", file=sys.stderr)
> +                sys.exit(1)
> +
> +            if event['event'] == 'RESET':
> +                print("The guest was Rebooted", file=sys.stderr)
> +                sys.exit(1)

I think you found this is a soft reset and the blockjob is still
running.  There is no need to exit.  You can ignore this event.

> +
> +            if event['event'] == 'BLOCK_JOB_COMPLETED':
> +                if event['data']['device'] in drive_list and \
> +                        event['data']['type'] == 'backup':
> +                        print("*"+event['data']['device'])

Please use whitespace around arithmetic operators:

  print("*" + event['data']['device'])

> +                        drive_list.remove(event['data']['device'])
> +
> +            if event['event'] == 'BLOCK_JOB_ERROR':
> +                if event['data']['device'] in drive_list and \
> +                        event['data']['type'] == 'backup':
> +                        print(event['data']['device']+" Backup Failed", file=sys.stderr)

The drive was not removed from drive_list so the loop never terminates.

sys.exit(1)?