Re: [Qemu-devel] [RFC] RFC on Backup tool

[Alex Bennée]

Ishani <address@hidden> writes:

> Thanks for the review.
>
> ----- On Jul 17, 2017, at 12:48 PM, Fam Zheng address@hidden wrote:
>
>> On Sun, 07/16 02:13, Ishani Chugh wrote:
>>> This is a Request For Comments patch for qemu backup tool. As an
>>> Outreachy intern, I am assigned to the project for creating a backup
>>> tool. qemu-backup will be a command-line tool for performing full and
>>> incremental disk backups on running VMs.
>>
>> Only full backup is implemented in this patch, is the plan to add incremental
>> backup on top?  I'm curious because you have target file path set during 
>> drive
>> add, but in the incremental case, it should be possible that each backup 
>> creates
>> a new target file that chains up with earlier ones, so I think the target 
>> file
>> should be an option for "backup" command as well.
>
> Yes. Incremental backup is to be added. I am still in learning phase with
> respect to incremental backups. I will modify the arguments and workflow 
> accordingly.
>
>>> 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 usage is as follows:
>>> Add a guest
>>> python qemu-backup.py guest add --guest <guest_name> --qmp <socket_path> 
>>> [--tcp]
>>>
>>> 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
>>
>> Please put these examples into the help output of the command, this way users
>> can read it as well.
>
> I have created a manpage documentation for the tool.
> http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg05241.html
> It is to be updated continuously as the development progresses.
>
>>>
>>> I will be obliged by any feedback.
>>
>> Thanks for doing this!
>>
>>>
>>> Signed-off-by: Ishani Chugh <address@hidden>
>>> ---
>>>  contrib/backup/qemu-backup.py | 244 
>>> ++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 244 insertions(+)
>>>  create mode 100644 contrib/backup/qemu-backup.py
>>>
>>> diff --git a/contrib/backup/qemu-backup.py b/contrib/backup/qemu-backup.py
>>> new file mode 100644
>>> index 0000000..9c3dc53
>>> --- /dev/null
>>> +++ b/contrib/backup/qemu-backup.py
>>> @@ -0,0 +1,244 @@
>>> +#!/usr/bin/python
>>> +# -*- coding: utf-8 -*-
>>
>> You need a copyright header here (the default choice for QEMU is GPLv2 but 
>> there
>> is no strict restrictions for scripts). See examples in other *.py files.
>
> Thanks. Will update in next revision.
>
>>> +"""
>>> +This file is an implementation of backup tool
>>> +"""
>>> +from argparse import ArgumentParser
>>> +import os
>>> +import errno
>>> +from socket import error as socket_error
>>> +import configparser
>>
>> Python2 has ConfigParser while python3 has configparser. Please be specific
>> about the python compatibility level of this script - my system (Fedora) has
>> python2 as /usr/bin/python, so the shebang and your example command in the
>> commit message don't really work. "six" module can handle python 2/3
>> differentiations, or you can use '#!/usr/bin/env python2' to specify a python
>> version explicitly.
>
> The script is supposed to be both python2/3 compatible. I will take reference
> from Stefan's review and fix it in next revision.
>
>>> +import sys
>>> +sys.path.append('../../scripts/qmp')
>>
>> This expects the script to be invoked from its local directory? It's better 
>> to
>> use the __file__ trick to allow arbitrary workdir:
>>
>> $ git grep __file__
>> scripts/device-crash-test:sys.path.append(os.path.join(os.path.dirname(__file__),
>> '..', 'scripts'))
>> scripts/qemu-gdb.py:sys.path.append(os.path.dirname(__file__))
>> tests/migration/guestperf/engine.py:sys.path.append(os.path.join(os.path.dirname(__file__),
>> '..', '..', '..', 'scripts'))
>> tests/qemu-iotests/iotests.py:sys.path.append(os.path.join(os.path.dirname(__file__),
>> '..', '..', 'scripts'))
>
> Thanks. Will fix it in next revision.
>
>>> +from qmp import QEMUMonitorProtocol
>>> +
>>> +
>>> +class BackupTool(object):
>>> +    """BackupTool Class"""
>>> +    def __init__(self, config_file='backup.ini'):
>>
>> Is it better to put this in a more predictable place such as
>> "$HOME/.qemu-backup.ini" and/or make it a command line option?
>
> It is planned to be an optional command-line option, if not
> provided, the default location suggested should be taken.

Can it be $HOME/.config/qemu/backup.ini please as per:

  https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html

Cheers,

--
Alex Bennée