From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48306) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aBrOb-0001Vp-SS for qemu-devel@nongnu.org; Wed, 23 Dec 2015 16:56:35 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aBrOX-00069Q-Nl for qemu-devel@nongnu.org; Wed, 23 Dec 2015 16:56:33 -0500 Received: from mx1.redhat.com ([209.132.183.28]:56899) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aBrOX-00069L-GK for qemu-devel@nongnu.org; Wed, 23 Dec 2015 16:56:29 -0500 References: <1449240275-26196-1-git-send-email-den@openvz.org> <1449240275-26196-5-git-send-email-den@openvz.org> From: Eric Blake Message-ID: <567B188B.40108@redhat.com> Date: Wed, 23 Dec 2015 14:56:27 -0700 MIME-Version: 1.0 In-Reply-To: <1449240275-26196-5-git-send-email-den@openvz.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="4aFJSBvgtHQ6jEBhcfhASRIbmRLcbd8Hp" Subject: Re: [Qemu-devel] [PATCH 4/5] migration: improve error reporting for load_vmstate List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Denis V. Lunev" Cc: Amit Shah , Markus Armbruster , qemu-devel@nongnu.org, quintela@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --4aFJSBvgtHQ6jEBhcfhASRIbmRLcbd8Hp Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 12/04/2015 07:44 AM, Denis V. Lunev wrote: > The patch adds Error ** parameter to load_vmstate call and fills error > inside. The caller after that properly reports error either through > monitor or via local stderr facility during VM start. >=20 > This helper will be useful too for qmp_loadvm implementation. >=20 > Signed-off-by: Denis V. Lunev > CC: Juan Quintela > CC: Amit Shah > CC: Markus Armbruster > CC: Eric Blake > --- > include/sysemu/sysemu.h | 2 +- > migration/savevm.c | 28 ++++++++++++++++------------ > monitor.c | 6 +++++- > vl.c | 4 +++- > 4 files changed, 25 insertions(+), 15 deletions(-) >=20 > -int load_vmstate(const char *name) > +int load_vmstate(const char *name, Error **errp) > { > BlockDriverState *bs, *bs_vm_state; > QEMUSnapshotInfo sn; > @@ -2035,20 +2035,22 @@ int load_vmstate(const char *name) > AioContext *aio_context; > =20 > if (!bdrv_all_can_snapshot(&bs)) { > - error_report("Device '%s' is writable but does not support sna= pshots.", > - bdrv_get_device_name(bs)); > + error_setg(errp, > + "Device '%s' is writable but does not support snaps= hots.", No trailing '.' > @@ -2058,10 +2060,11 @@ int load_vmstate(const char *name) > ret =3D bdrv_snapshot_find(bs_vm_state, &sn, name); > aio_context_release(aio_context); > if (ret < 0) { > + error_setg_errno(errp, ret, "Snapshot '%s' not found", name); You need '-ret', since error_setg_errno() expects positive errno values. [maybe, for convenience, we should teach error_setg_errno() to handle both -EINVAL and EINVAL identically - but it would need good justification and would touch a lot of the tree, so if we do it, it would be a separate series] > return ret; > } else if (sn.vm_state_size =3D=3D 0) { > - error_report("This is a disk-only snapshot. Revert to it offli= ne " > - "using qemu-img."); > + error_setg(errp, "This is a disk-only snapshot. Revert to it o= ffline " > + "using qemu-img."); According to Markus' recent cleanups, error_setg() should be a single phrase, not two sentences. You'll want the second sentence to be added with error_append_hint(): https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg03508.html > return -EINVAL; > } > =20 > @@ -2070,15 +2073,16 @@ int load_vmstate(const char *name) > =20 > ret =3D bdrv_all_goto_snapshot(name, &bs); > if (ret < 0) { > - error_report("Error %d while activating snapshot '%s' on '%s'"= , > - ret, name, bdrv_get_device_name(bs)); > + error_setg_errno(errp, ret, -ret again > + "Error while activating snapshot '%s' on '%s'= ", > + name, bdrv_get_device_name(bs)); but thanks for getting rid of the weird 'error -5 while...' in the message :) > @@ -2092,7 +2096,7 @@ int load_vmstate(const char *name) > =20 > migration_incoming_state_destroy(); > if (ret < 0) { > - error_report("Error %d while loading VM state", ret); > + error_setg_errno(errp, ret, "Error while loading VM state"); -ret again > +++ b/monitor.c > @@ -1739,10 +1739,14 @@ static void hmp_loadvm(Monitor *mon, const QDic= t *qdict) > { > int saved_vm_running =3D runstate_is_running(); > const char *name =3D qdict_get_str(qdict, "name"); > + Error *local_err =3D NULL; > =20 > vm_stop(RUN_STATE_RESTORE_VM); > =20 > - if (load_vmstate(name) =3D=3D 0 && saved_vm_running) { > + if (load_vmstate(name, &local_err) < 0) { > + error_report_err(local_err); > + } > + if (saved_vm_running) { > vm_start(); Logic bug. You blindly fall through, and could end up attempting vm_start() even after an error, where previously it was not possible. You probably meant '} else if (saved_vm_running) {'. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --4aFJSBvgtHQ6jEBhcfhASRIbmRLcbd8Hp Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJWexiMAAoJEKeha0olJ0Nq0hkH/0CfG2SIBRx5tQ2WvUyeSlo6 4yRaA25V9npmpKS9xGxyJGEC5g9W+8Q9OU7F+8r00/0omutzMSqgwSzPRPKyzSal 0e7hhgholOO/LHzQIvaH2sZyjPxB4HU2dZReiFX9QXExNmrvDlY+ziQ9HJIFGqvY w05JdlwPKzixtfEJ1adJZtFvZXV32CGdV0htPM/o/XhJVJwFpCvGaB1y/JEkhzby bLfAh2befQYEQDGEObTDJxBBJctRGgqijnjOblHluRUCnIkCzZo4aGbvMohg40yC Rl3z1sLi/RQYb/CwG1iOWRyIMicIIbp40BC/RlrgBrO5Dg97goNmXNFrXOCKMuk= =2mZ9 -----END PGP SIGNATURE----- --4aFJSBvgtHQ6jEBhcfhASRIbmRLcbd8Hp--