From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34653) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XED5o-0000E2-MU for qemu-devel@nongnu.org; Mon, 04 Aug 2014 03:54:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XED5j-0005yC-SI for qemu-devel@nongnu.org; Mon, 04 Aug 2014 03:54:04 -0400 Received: from mx1.redhat.com ([209.132.183.28]:61041) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XED5j-0005xA-Kj for qemu-devel@nongnu.org; Mon, 04 Aug 2014 03:53:59 -0400 From: Markus Armbruster References: <1406800053-6480-1-git-send-email-arei.gonglei@huawei.com> <1406800053-6480-2-git-send-email-arei.gonglei@huawei.com> <20140801133618.GA27316@thinpad.lan.raisama.net> <20140801094016.1bb90a30@redhat.com> <33183CC9F5247A488A2544077AF1902086C24C8C@SZXEMA503-MBS.china.huawei.com> Date: Mon, 04 Aug 2014 09:53:20 +0200 In-Reply-To: <33183CC9F5247A488A2544077AF1902086C24C8C@SZXEMA503-MBS.china.huawei.com> (Gonglei's message of "Mon, 4 Aug 2014 07:02:27 +0000") Message-ID: <87wqaobsi7.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v4 1/8] bootindex: add modify_boot_device_path function List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Gonglei (Arei)" Cc: "chenliang (T)" , "Huangweidong (C)" , "mst@redhat.com" , "aik@ozlabs.ru" , "hutao@cn.fujitsu.com" , "qemu-devel@nongnu.org" , Luiz Capitulino , "kraxel@redhat.com" , "akong@redhat.com" , "agraf@suse.de" , "aliguori@amazon.com" , "gaowanlong@cn.fujitsu.com" , Eduardo Habkost , Luonengjun , "Huangpeng (Peter)" , "hani@linux.com" , "stefanha@redhat.com" , "pbonzini@redhat.com" , "kwolf@redhat.com" , "peter.crosthwaite@xilinx.com" , "imammedo@redhat.com" , "afaerber@suse.de" "Gonglei (Arei)" writes: > Hi, > >> >> > On Thu, Jul 31, 2014 at 05:47:26PM +0800, arei.gonglei@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. 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.