[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v12 1/8] error: New macro ERRP_AUTO_PROPAGATE()


  • To: Markus Armbruster <armbru@xxxxxxxxxx>, qemu-devel@xxxxxxxxxx
  • From: Vladimir Sementsov-Ogievskiy <vsementsov@xxxxxxxxxxxxx>
  • Date: Tue, 7 Jul 2020 23:08:19 +0300
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=virtuozzo.com; dmarc=pass action=none header.from=virtuozzo.com; dkim=pass header.d=virtuozzo.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=1Y0d7pH2Kl9fMsIHQ/bgSh645mak3V/leis4tLusQVY=; b=E46U6tQ3uSgPrSa4gK1Jd+Xr7rsqx0rA1uxiz5nwbghMEbOaDKdIxg2tbETJeeMueHXCrfbBoeXhmLN2Z0ZOF89PvOLt/SlzbVBsbJVnIwn9x/UkwUc+Sz2qjPol3OZdft2PCWxQnvGW51/fSYIX+twv5yv1H+CK1SruGpSrxGd8WBPmKKSdlOlCewYCuAiM3QUz79NH/hED/Uk2BGLU4UilE3WT2YJyqjLDK/zabhQZlKXaR9zqPopdNv1PDI0VHk4Xmym7AITY4jpQghxQ6Dcq0ghL0h6RBEhl9DM8316fGAdGqioorCUnUw0356azpqtUjYEZ0qPxmoG3t7ioig==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=XiDrAuSpVmz6ZLc4y1YU5F0PypfliKCbPMiN4V2i7DJjHWGhVLGnK/pPFk8rrYPWJFZQ71eQ/BiXQCV75NVJjNwYj+IofTHQ1dKRkcAf6NLeIjotX/bJAIE5OuL+5WIr9fBEQ7ZGLYLzqS2BQb845BvfLgSSsiCFK9M0XpUhmb8kGq3G0L5WDhLemrm/LHfcTjxtmwZZWCxp6ig3sK0aSCmelUU5Gp9jDjEK1goPKZY28Ue38J9twrBVhpmiem203eL5Wc2yygIamDCclcYYQDpqlLwPv4wAUzilAmDhbi4NmXET9my2EqII3t8Mm5uT3ZTVTX76oXObP7BQ41GoUA==
  • Authentication-results: redhat.com; dkim=none (message not signed) header.d=none;redhat.com; dmarc=none action=none header.from=virtuozzo.com;
  • Cc: Kevin Wolf <kwolf@xxxxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Michael Roth <mdroth@xxxxxxxxxxxxxxxxxx>, qemu-block@xxxxxxxxxx, Paul Durrant <paul@xxxxxxx>, Laszlo Ersek <lersek@xxxxxxxxxx>, Christian Schoenebeck <qemu_oss@xxxxxxxxxxxxx>, groug@xxxxxxxx, Eric Blake <eblake@xxxxxxxxxx>, Max Reitz <mreitz@xxxxxxxxxx>, Gerd Hoffmann <kraxel@xxxxxxxxxx>, Stefan Hajnoczi <stefanha@xxxxxxxxxx>, Anthony Perard <anthony.perard@xxxxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx, Philippe Mathieu-Daudé <philmd@xxxxxxxxxx>
  • Delivery-date: Tue, 07 Jul 2020 20:08:33 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

07.07.2020 19:50, Markus Armbruster wrote:
From: Vladimir Sementsov-Ogievskiy<vsementsov@xxxxxxxxxxxxx>

Introduce a new ERRP_AUTO_PROPAGATE macro, to be used at start of
functions with an errp OUT parameter.

It has three goals:

1. Fix issue with error_fatal and error_prepend/error_append_hint: user
can't see this additional information, because exit() happens in
error_setg earlier than information is added. [Reported by Greg Kurz]

2. Fix issue with error_abort and error_propagate: when we wrap
error_abort by local_err+error_propagate, the resulting coredump will
refer to error_propagate and not to the place where error happened.
(the macro itself doesn't fix the issue, but it allows us to [3.] drop
the local_err+error_propagate pattern, which will definitely fix the
issue) [Reported by Kevin Wolf]

3. Drop local_err+error_propagate pattern, which is used to workaround
void functions with errp parameter, when caller wants to know resulting
status. (Note: actually these functions could be merely updated to
return int error code).

To achieve these goals, later patches will add invocations
of this macro at the start of functions with either use
error_prepend/error_append_hint (solving 1) or which use
local_err+error_propagate to check errors, switching those
functions to use *errp instead (solving 2 and 3).

Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@xxxxxxxxxxxxx>
Reviewed-by: Paul Durrant<paul@xxxxxxx>
Reviewed-by: Greg Kurz<groug@xxxxxxxx>
Reviewed-by: Eric Blake<eblake@xxxxxxxxxx>
[Comments merged properly with recent commit "error: Document Error
API usage rules", and edited for clarity.  Put ERRP_AUTO_PROPAGATE()
before its helpers, and touch up style.  Commit message tweaked.]
Signed-off-by: Markus Armbruster<armbru@xxxxxxxxxx>

Ok, I see you have mostly rewritten the big comment and not only in this patch, 
so, I go and read the whole comment on top of these series.

=================================

   * Pass an existing error to the caller with the message modified:
   *     error_propagate_prepend(errp, err,
   *                             "Could not frobnicate '%s': ", name);
   * This is more concise than
   *     error_propagate(errp, err); // don't do this
   *     error_prepend(errp, "Could not frobnicate '%s': ", name);
   * and works even when @errp is &error_fatal.

- the latter doesn't consider ERRP_AUTO_PROPAGATE: as we know, that 
ERRP_AUTO_PROPAGATE should be used when we use error_prepend, the latter should 
look like


ERRP_AUTO_PROPAGATE();
...
error_propagate(errp, err); // don't do this
error_prepend(errp, "Could not frobnicate '%s': ", name);

- and it works even when @errp is &error_fatal, so the error_propagate_prepend 
now is just a shortcut, not the only correct way.


Still, the text is formally correct as is, and may be improved later.

=================================

   * 2. Replace &err by errp, and err by *errp.  Delete local variable
   *    @err.

- hmm a bit not obvious,, It can be local_err. It can be (in some rare cases) 
still needed to handle the error locally, not passing to the caller..

may be just something like "Assume local Error *err variable is used to get errors 
from called functions and than propagated to caller's errp" before paragraph [2.] 
will help.


   *
   * 3. Delete error_propagate(errp, *errp), replace
   *    error_propagate_prepend(errp, *errp, ...) by error_prepend(errp, ...),
   *
   * 4. Ensure @errp is valid at return: when you destroy *errp, set
   *    errp = NULL.

=================================


May be good to add note about ERRP_AUTO_PROPAGATE() into comment above 
error_append_hint (and error_(v)prepend)).



=================================

  /*
   * Make @errp parameter easier to use regardless of argument value

may be s/argument/its/

   *
   * This macro is for use right at the beginning of a function that
   * takes an Error **errp parameter to pass errors to its caller.  The
   * parameter must be named @errp.
   *
   * It must be used when the function dereferences @errp or passes
   * @errp to error_prepend(), error_vprepend(), or error_append_hint().
   * It is safe to use even when it's not needed, but please avoid
   * cluttering the source with useless code.
   *
   * If @errp is NULL or &error_fatal, rewrite it to point to a local
   * Error variable, which will be automatically propagated to the
   * original @errp on function exit.
   *
   * Note: &error_abort is not rewritten, because that would move the
   * abort from the place where the error is created to the place where
   * it's propagated.
   */

=================================


All these are minor, the documentation is good as is, thank you!

--
Best regards,
Vladimir



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.