Hi, > >> > >> > On Thu, Jul 31, 2014 at 05:47:26PM +0800, arei.gong...@huawei.com > wrote: > >> > [...] > >> > > +void modify_boot_device_path(int32_t bootindex, DeviceState *dev, > >> > > + const char *suffix) > >> > > +{ > >> > > + FWBootEntry *i, *old_entry = NULL; > >> > > + > >> > > + assert(dev != NULL || suffix != NULL); > >> > > + > >> > > + if (bootindex >= 0) { > >> > > + QTAILQ_FOREACH(i, &fw_boot_order, link) { > >> > > + if (i->bootindex == bootindex) { > >> > > + qerror_report(ERROR_CLASS_GENERIC_ERROR, > >> > > + "The bootindex %d has already > been > >> used", > >> > > + bootindex); > >> > > >> > Isn't an Error** parameter preferable here, instead of using > >> > qerror_report()? > >> > >> Good catch. I'm not following this series, but using qerror_report() is > >> almost always wrong these days. > > Yes. http://wiki.qemu.org/CodeTransitions#Error_reporting explains: > > qerror_report() is a transitional interface to help with converting > existing HMP commands to QMP. It should not be used elsewhere. Use > Error objects instead with error_propagate() and error_setg() > instead. > > > Would you give me some advice? Thanks. > > Using error_report() instead of qerror_report()? > > Depends on how the function is used. > > If you know this can only run during QEMU startup (e.g. command line > processing) or in a *human* monitor, error_report() is fine. > > If the error is propagated up the call chain to some place that reports > it via its Error ** parameter to its caller, then you should consider > passing an Error object created with error_setg() here up the call > chain. > Understood, thank you so much! error_setg() is the best choice in my case.
> Not the case right now, as your modify_boot_device_path() cannot fail. > Whether that's appropriate I can't tell without examining more of your > patch. Best regards, -Gonglei