On Sun, Jul 16, 2017 at 02:13:21AM +0530, Ishani Chugh wrote: > + def write_config(self): > + """ > + Writes configuration to ini file. > + """ > + with open(self.config_file, 'w') as config_file: > + self.config.write(config_file)
Please update the config file atomically. That means the file must be consistent at all points in time. Things to consider: 1. open(self.config_file, 'w') truncates the destination file. If the program crashes or is paused before self.config.write() then the config file will appear empty (its contents are lost)! 2. Data is written to the file at the end of the 'with' statement (because that flushes the buffer and closes the file), but there is no guarantee that data is safely stored on disk. Please use https://docs.python.org/3/library/os.html#os.fsync to prevent data loss in case of power failure. The usual pattern is: 1. Create a temporary file next to the file you wish to modify. "Next to" is important because if you create it on another file system (like tmpfs) it may not be possible to atomically replace the old file. 2. Write data 3. file.flush() + os.fsync() to safely store data in the new file 4. os.rename() to atomically replace the old file For an example, see: https://stackoverflow.com/questions/2333872/atomic-writing-to-file-with-python > + > + def get_socket_path(self, socket_path, tcp): > + """ > + Return Socket address in form of string or tuple Please rename this function get_socket_address(). The return value is an 'address', not a 'path'. > + """ > + if tcp is False: PEP8 says: Don't compare boolean values to True or False using == . Yes: if greeting: No: if greeting == True: Worse: if greeting is True: So this should be: if not tcp: > + return os.path.abspath(socket_path) > + return (socket_path.split(':')[0], int(socket_path.split(':')[1])) The str.split(delim, maxsplit) argument can be used but feel free to keep your version if you prefer: return tuple(socket_path.split(':', 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") > + return > + if self.is_guest_running(guest_name, self.config[guest_name]['qmp'], > + self.config[guest_name]['tcp']) is False: This is clearer if self.is_guest_running() looks up the necessary information in self.config[] itself: if not self.is_guest_running(guest_name): ... Since is_guest_running() is a method it has access to self.config[] and there's no need for every caller to fetch 'qmp'/'tcp'. > + return > + connection = QEMUMonitorProtocol( > + self.get_socket_path( > + self.config[guest_name]['qmp'], > + self.config[guest_name]['tcp'])) > + connection.connect() > + cmd = {"execute": "transaction", "arguments": {"actions": []}} > + for key in self.config[guest_name]: > + if key.startswith("drive_"): > + drive = key[key.index('_')+1:] len('drive_') is a little clearer than key.index('_')+1. > + target = self.config[guest_name][key] > + sub_cmd = {"type": "drive-backup", "data": {"device": drive, > + "target": target, > + "sync": "full"}} > + cmd['arguments']['actions'].append(sub_cmd) > + print (connection.cmd_obj(cmd)) The non-RFC version of this patch should not print raw qmp.py objects. That's useful for debugging but not user-friendly. Either there could be no output and a 0 exit code (indicating success), or there could be a message like: Starting full backup of drives: * drive0 * drive1 Backup complete! > + > + def __drive_add(self, drive_id, guest_name, target=None): > + """ > + Adds drive for backup > + """ > + if target is None: > + target = os.path.abspath(drive_id) + ".img" Not sure if this is really useful. Many image files have a non-.img file extension like .qcow2. Guessing the target filename is probably just confusing because the user experience will be inconsistent (sometimes it works, sometimes it doesn't, depending on the file extension and the current working directory). > + > + if guest_name not in self.config.sections(): > + print ("Cannot find specified guest") > + return > + > + if "drive_"+drive_id in self.config[guest_name]: > + print ("Drive already marked for backup") > + return > + > + if self.is_guest_running(guest_name, self.config[guest_name]['qmp'], > + self.config[guest_name]['tcp']) is False: > + return Silently exiting is confusing behavior. There should be an error message and the process should terminate with a failure exit code (1). By the way, it may be best to avoid repetition by writing methods for these checks: def verify_guest_exists(self, guest_name): if guest_name not in self.config.sections(): print('Cannot find specified guest', file=sys.stderr) sys.exit(1) def verify_guest_running(self, guest_name): if not self.is_guest_running(guest_name): print('Unable to connect to guest (is the guest running?)', file=sys.stderr) sys.exit(1) def a_command(self, guest_name): self.verify_guest_exists(guest_name) self.verify_guest_running(guest_name) > + > + connection = QEMUMonitorProtocol( > + self.get_socket_path( > + self.config[guest_name]['qmp'], > + self.config[guest_name]['tcp'])) > + connection.connect() > + cmd = {'execute': 'query-block'} > + returned_json = connection.cmd_obj(cmd) > + device_present = False > + for device in returned_json['return']: > + if device['device'] == drive_id: > + device_present = True > + break > + > + if device_present is False: > + print ("No such drive in guest") > + return > + > + drive_id = "drive_" + drive_id > + for id in self.config[guest_name]: > + if self.config[guest_name][id] == target: > + print ("Please choose different target") > + return Please make it clear from the error message that this is a duplicate target. "Please choose different target" doesn't explain why there is a problem and users would be confused. > + self.config.set(guest_name, drive_id, target) > + self.write_config() > + print("Successfully Added Drive") > + > + def is_guest_running(self, guest_name, socket_path, tcp): It is not necessary to distinguish between UNIX domain socket paths and TCP <host, port> pairs here or in the config file: >>> c.add_section('a') >>> c.set('a', 'b', ('hello', 'world')) >>> c.get('a', 'b') ('hello', 'world') >>> c.set('a', 'b', '/hello/world') >>> c.get('a', 'b') '/hello/world' If you treat the value as an opaque address type it doesn't matter whether it's a TCP <host, port> pair or a UNIX domain socket path. You can pass the return value of c.get() directly to the QEMUMonitorProtocol constructor: QEMUMonitorProtocol(self.config.get(guest_name, 'qmp')) You still need to parse the address in 'qemu-backup guest add' though. > + """ > + Checks whether specified guest is running or not > + """ > + try: > + connection = QEMUMonitorProtocol( > + self.get_socket_path( > + socket_path, tcp)) > + connection.connect() > + except socket_error: > + if socket_error.errno != errno.ECONNREFUSED: > + print ("Connection to guest refused") What is the purpose of this if statement?
signature.asc
Description: PGP signature
