[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] QEMU Backup Tool
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH] QEMU Backup Tool |
Date: |
Thu, 10 Aug 2017 14:09:06 +0100 |
User-agent: |
Mutt/1.8.3 (2017-05-23) |
On Thu, Aug 10, 2017 at 01:06:27AM +0530, Ishani Chugh wrote:
> qemu-backup will be a command-line tool for performing full and
> incremental disk backups on running VMs. It is intended as a
> reference implementation for management stack and backup developers
> to see QEMU's backup features in action. The tool writes details of
> guest in a configuration file and the data is retrieved from the file
> while creating a backup. The location of config file can be set as an
> environment variable QEMU_BACKUP_CONFIG. The usage is as follows:
>
> Add a guest
> python qemu-backup.py guest add --guest <guest_name> --qmp <socket_path>
>
> Add a drive for backup in a specified guest
> python qemu-backup.py drive add --guest <guest_name> --id <drive_id>
> [--target <target_file_path>]
>
> Create backup of the added drives:
> python qemu-backup.py backup --guest <guest_name>
>
> List all guest configs in configuration file:
> python qemu-backup.py guest list
>
> Restore operation
> python qemu-backup.py restore --guest <guest-name>
>
> Remove a guest
> python qemu-backup.py guest remove --guest <guest_name>
>
>
> Signed-off-by: Ishani Chugh <address@hidden>
> ---
> contrib/backup/qemu-backup.py | 217
> +++++++++++++++++++++++++++---------------
> 1 file changed, 141 insertions(+), 76 deletions(-)
Hi Ishani,
This patch is a diff that is based on an existing qemu-backup.py file.
The file doesn't exist in qemu.git/master yet so this patch cannot be
applied without the missing file.
Did you mean to send a new patch series consisting of patches for:
1. qemu-backup.py
2. man page
3. test case
?
I suggest using "git rebase -i origin/master" to move your patches onto
the latest qemu.git/master and reorder/squash them into a series of
logical code changes.
> diff --git a/contrib/backup/qemu-backup.py b/contrib/backup/qemu-backup.py
> index 9c3dc53..9bbbdb7 100644
> --- a/contrib/backup/qemu-backup.py
> +++ b/contrib/backup/qemu-backup.py
> @@ -1,22 +1,54 @@
> #!/usr/bin/python
> # -*- coding: utf-8 -*-
> +#
> +# Copyright (C) 2013 Red Hat, Inc.
Feel free to add your copyright:
Copyright (C) 2017 Ishani Chugh <address@hidden>
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program. If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> """
> This file is an implementation of backup tool
> """
> +from __future__ import print_function
> from argparse import ArgumentParser
> import os
> import errno
> from socket import error as socket_error
> -import configparser
> +try:
> + import configparser
> +except ImportError:
> + import ConfigParser as configparser
> import sys
> -sys.path.append('../../scripts/qmp')
> +sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..',
> + 'scripts', 'qmp'))
> from qmp import QEMUMonitorProtocol
>
>
> class BackupTool(object):
> """BackupTool Class"""
> - def __init__(self, config_file='backup.ini'):
> - self.config_file = config_file
> + def __init__(self,
> + config_file=os.path.expanduser('~')+'/.qemu/backup/config'):
Please use os.path.join() instead of appending strings.
You could consider using a variable to avoid repeating this particular
path since it is used several times in the code:
DEFAULT_CONFIG_FILE = os.path.join(os.path.expanduser('~'),
'.qemu', 'backup', 'config')
The XDG Base Directory Specification would use ~/.config/qemu instead of
~/.qemu:
https://standards.freedesktop.org/basedir-spec/basedir-spec-latest.html
Modern applications tend to follow this spec.
> + if "QEMU_BACKUP_CONFIG" in os.environ:
> + self.config_file = os.environ["QEMU_BACKUP_CONFIG"]
> + else:
> + self.config_file = config_file
> + try:
> + if not
> os.path.isdir(os.path.expanduser('~')+'/.qemu/backup'):
os.path.dirname(DEFAULT_CONFIG_FILE)
> + os.makedirs(os.path.expanduser('~')+'/.qemu/backup')
os.path.dirname(DEFAULT_CONFIG_FILE)
> + except:
> + print("Cannot find the config file", file=sys.stderr)
This error message doesn't match the try-catch block's purpose. The
issue was that the config directory couldn't be created.
> + exit(1)
> self.config = configparser.ConfigParser()
> self.config.read(self.config_file)
>
> @@ -24,66 +56,70 @@ class BackupTool(object):
> """
> Writes configuration to ini file.
> """
> - with open(self.config_file, 'w') as config_file:
> - self.config.write(config_file)
> + config_file = open(self.config_file+".tmp", 'w')
> + 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)
>
> - def get_socket_path(self, socket_path, tcp):
> + def get_socket_address(self, socket_address):
> """
> Return Socket address in form of string or tuple
> """
> - if tcp is False:
> - return os.path.abspath(socket_path)
> - return (socket_path.split(':')[0], int(socket_path.split(':')[1]))
> + 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):
> + 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:
> - return
> + print ("Cannot find specified guest", file=sys.stderr)
print() is a function, there shouldn't be a space before the parentheses:
print("message")
> + exit(1)
> +
> + self.verify_guest_running(guest_name)
> connection = QEMUMonitorProtocol(
> - self.get_socket_path(
> - self.config[guest_name]['qmp'],
> - self.config[guest_name]['tcp']))
> + self.get_socket_address(
> + self.config[guest_name]['qmp']))
> connection.connect()
> cmd = {"execute": "transaction", "arguments": {"actions": []}}
> for key in self.config[guest_name]:
> if key.startswith("drive_"):
> - drive = key[key.index('_')+1:]
> + drive = key[len('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)
> - print (connection.cmd_obj(cmd))
> + connection.cmd_obj(cmd)
> + if connection.pull_event(wait=True)['event'] ==
> 'BLOCK_JOB_COMPLETED':
> + print("Backup Complete")
> + else:
> + print("Cannot complete backup", file=sys.stderr)
A loop is needed here because innocent QMP events can occur like a VNC
client connection. BLOCK_JOB_ERROR is an interesting event to report.
Perhaps SHUTDOWN is interesting too. Other than that we need to loop
waiting for events and must not exit early.
signature.asc
Description: PGP signature