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)?