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

2017-08-10 Thread 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  --qmp 
> 
> Add a drive for backup in a specified guest
> python qemu-backup.py drive add --guest  --id  
> [--target ]
> 
> Create backup of the added drives:
> python qemu-backup.py backup --guest 
> 
> List all guest configs in configuration file:
> python qemu-backup.py guest list
> 
> Restore operation
> python qemu-backup.py restore --guest 
> 
> Remove a guest
> python qemu-backup.py guest remove --guest 
> 
> 
> Signed-off-by: Ishani Chugh 
> ---
>  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 

> +#
> +# 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 .
> +#
> +
>  """
>  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 

[Qemu-devel] [PATCH] QEMU Backup Tool

2017-08-09 Thread Ishani Chugh
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  --qmp 

Add a drive for backup in a specified guest
python qemu-backup.py drive add --guest  --id  [--target 
]

Create backup of the added drives:
python qemu-backup.py backup --guest 

List all guest configs in configuration file:
python qemu-backup.py guest list

Restore operation
python qemu-backup.py restore --guest 

Remove a guest
python qemu-backup.py guest remove --guest 


Signed-off-by: Ishani Chugh 
---
 contrib/backup/qemu-backup.py | 217 +++---
 1 file changed, 141 insertions(+), 76 deletions(-)

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.
+#
+# 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 .
+#
+
 """
 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'):
+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.makedirs(os.path.expanduser('~')+'/.qemu/backup')
+except:
+print("Cannot find the config file", file=sys.stderr)
+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)
+exit(1)
+
+self.verify_guest_running(guest_name)
 connection =