Re: [Qemu-devel] [PATCH v4 1/8] bootindex: add modify_boot_device_path function

[Gonglei (Arei)]

Hi,

> >>
> >> > On Thu, Jul 31, 2014 at 05:47:26PM +0800, address@hidden
> 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