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 <xiaw...@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