Re: [Qemu-devel] [PATCH] QEMU Backup Tool

[Stefan Hajnoczi]

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