On 26.3.2013 15:22, Eric Blake wrote:
On 03/22/2013 07:16 AM, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrd...@redhat.com>
---
block.c | 22 ++++++++++++++++------
block/qcow2-snapshot.c | 9 ++++++++-
block/qcow2.h | 4 +++-
block/rbd.c | 8 ++++++--
block/sheepdog.c | 17 +++++++++--------
include/block/block.h | 3 ++-
include/block/block_int.h | 3 ++-
qemu-img.c | 2 +-
savevm.c | 2 +-
9 files changed, 48 insertions(+), 22 deletions(-)
int bdrv_snapshot_create(BlockDriverState *bs,
- QEMUSnapshotInfo *sn_info)
+ QEMUSnapshotInfo *sn_info,
+ Error **errp)
{
BlockDriver *drv = bs->drv;
- if (!drv)
+
+ if (!drv) {
+ error_setg(errp, "Device '%s' has no medium.",
In general, error_setg() should not print a trailing '.' (only two
offenders to git grep 'error_setg.*\."'). I think we also tend to start
messages with lower case, although that's not as obvious (49 cases of
'error_setg[^"*"[A-Z]' vs. 121 of error_setg[^"]*"[a-z]').
I don't know that there is any recommendation for that. I'll fix that in
next series.
+
+ error_setg(errp, "Snapshot is not supported for '%s'.",
And again, and probably throughout your series (although I'll quit
pointing it out).
@@ -830,16 +832,18 @@ static int qemu_rbd_snap_create(BlockDriverState *bs,
*/
if (sn_info->id_str[0] != '\0' &&
strcmp(sn_info->id_str, sn_info->name) != 0) {
+ error_setg(errp, "ID and name have to be equal.");
return -EINVAL;
}
if (strlen(sn_info->name) >= sizeof(sn_info->id_str)) {
+ error_setg(errp, "Parameter 'name' has to be shorter that 127 chars.");
s/that/than/
return -ERANGE;
}
r = rbd_snap_create(s->image, sn_info->name);
if (r < 0) {
- error_report("failed to create snap: %s", strerror(-r));
+ error_setg(errp, "Failed to create snapshot: '%s'.", strerror(-r));
Use error_setg_errno(errp, -r, "failed to create snapshot").
@@ -1779,9 +1781,8 @@ static int sd_snapshot_create(BlockDriverState *bs,
QEMUSnapshotInfo *sn_info)
s->name, sn_info->vm_state_size, s->is_snapshot);
if (s->is_snapshot) {
- error_report("You can't create a snapshot of a snapshot VDI, "
- "%s (%" PRIu32 ").", s->name, s->inode.vdi_id);
-
+ error_setg(errp, "You can't create a snapshot '%s' of a VDI snapshot.",
+ sn_info->name);
Why are you losing information from the previous version of this error
message?
if (ret < 0) {
- error_report("failed to create inode for snapshot. %s",
- strerror(errno));
+ error_setg(errp, "Failed to create inode for snapshot.");
Another case of losing information; error_setg_errno would be better
here, and anywhere else the old message included a strerror() call.
Thanks for pointing this out. I've probably missed that during the
rebase. I'll also check all other patches before I'll send another series.
Pavel
