Re: [Qemu-devel] [PATCH V3 6/7] qcow2: print message for error path in snapshot creation

[Stefan Hajnoczi <stefanha@gmail.com>]

On Mon, Sep 30, 2013 at 04:08:53PM -0600, Eric Blake wrote:
> On 09/08/2013 08:58 PM, Wenchao Xia wrote:
> > The message will be print out with a macro enabled, which can
> 
> s/print/printed/
> 
> > be used to check which error path is taken.
> > 
> > Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> > ---
> >  block/qcow2-snapshot.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 46 insertions(+), 0 deletions(-)
> > 
> 
> > @@ -381,12 +413,20 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
> >      ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_DEFAULT,
> >              sn->l1_table_offset, s->l1_size * sizeof(uint64_t));
> >      if (ret < 0) {
> > +#ifdef QCOW2_SNAPSHOT_PRINT_ERROR_PATH
> > +        printf("qcow2: Failed in overlap check before update L1 table for "
> > +               "snapshot\n");
> > +#endif
> >          goto dealloc_cluster;
> >      }
> >  
> > +    BLKDBG_EVENT(bs->file, BLKDBG_SNAPSHOT_L1_UPDATE);
> 
> Should this BLKDBG be part of patch 5?
> 
> In general, the move to avoid fprintf except under recompilation seems
> okay; but it seems odd to be removing the diagnosis altogether.  If you
> had gone one step further and refactored the code to wire in Error*
> support, then you could change fprintf to passing the Error back up the
> stack to the caller rather than losing it except during a debug build.

I agree with Eric.  Use Error* and make snapshot commands print a
detailed error to the monitor.

When diagnostics are compiled out we can't help troubleshoot user
problems.

Stefan