From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55465) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YIZEd-0006KU-4Z for qemu-devel@nongnu.org; Tue, 03 Feb 2015 03:53:28 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YIZEZ-0006ZE-BL for qemu-devel@nongnu.org; Tue, 03 Feb 2015 03:53:27 -0500 Received: from szxga02-in.huawei.com ([119.145.14.65]:6571) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YIZEY-0006YB-Nk for qemu-devel@nongnu.org; Tue, 03 Feb 2015 03:53:23 -0500 Message-ID: <54D08C69.4050801@huawei.com> Date: Tue, 3 Feb 2015 16:52:57 +0800 From: Gonglei MIME-Version: 1.0 References: <1422538193-13648-1-git-send-email-arei.gonglei@huawei.com> <1422538193-13648-3-git-send-email-arei.gonglei@huawei.com> <54CA59EB.5040005@suse.de> <54CAD48A.5020709@huawei.com> <87386syamf.fsf@blackfin.pond.sub.org> <54CB3ED0.2070209@huawei.com> <878ugkpjdp.fsf@blackfin.pond.sub.org> <54CB74BD.80305@huawei.com> <87r3ucmoth.fsf@blackfin.pond.sub.org> <54CB7C77.4050400@huawei.com> <87egq8irhy.fsf@blackfin.pond.sub.org> <54D0289F.2080601@huawei.com> <878ugfe8nu.fsf@blackfin.pond.sub.org> In-Reply-To: <878ugfe8nu.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset="GB2312" Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/2] bootdevice: add check in restore_boot_order() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: "qemu-devel@nongnu.org" , "dvaleev@suse.de" , "Huangpeng (Peter)" , Alexander Graf On 2015/2/3 15:49, Markus Armbruster wrote: > You're right. pc.c's set_boot_dev() fails when its boot order argument > is invalid. > > The boot order interface is crap, because it makes detecting > configuration errors early hard. Two solutions: > > A. It may be hard, but not too hard for the determined > > 1. If "once" is given, register reset handler to restore boot order. > > 2. Pass the normal boot order to machine creation. Should fail when > the normal boot order is invalid. > > 3. If "once" is given, set it with qemu_boot_set(). Fails when the > once boot order is invalid. > > 4. Start the machine. > > 5. On reset, the reset handler calls qemu_boot_set() to restore boot > order. Should never fail. > What about the below patch? diff --git a/vl.c b/vl.c index 983259b..7d37191 100644 --- a/vl.c +++ b/vl.c @@ -126,6 +126,7 @@ int main(int argc, char **argv) @@ -126,6 +126,7 @@ int main(int argc, char **argv) --- a/vl.c +++ b/vl.c @@ -126,6 +126,7 @@ int main(int argc, char **argv) static const char *data_dir[16]; static int data_dir_idx; +const char *once = NULL; const char *bios_name = NULL; enum vga_retrace_method vga_retrace_method = VGA_RETRACE_DUMB; DisplayType display_type = DT_DEFAULT; @@ -4046,7 +4047,7 @@ int main(int argc, char **argv, char **envp) opts = qemu_opts_find(qemu_find_opts("boot-opts"), NULL); if (opts) { char *normal_boot_order; - const char *order, *once; + const char *order; Error *local_err = NULL; order = qemu_opt_get(opts, "order"); @@ -4067,7 +4068,6 @@ int main(int argc, char **argv, char **envp) exit(1); } normal_boot_order = g_strdup(boot_order); - boot_order = once; qemu_register_reset(restore_boot_order, normal_boot_order); } @@ -4246,6 +4246,15 @@ int main(int argc, char **argv, char **envp) net_check_clients(); + if (once) { + Error *local_err = NULL; + qemu_boot_set(once, &local_err); + if (local_err) { + error_report("%s", error_get_pretty(local_err)); + exit(1); + } + } + Regards, -Gonglei > B. Fix the crappy interface > > Separate parameter validation from the actual action. Only > validation may fail. Validate before starting the guest. > >>> * validate_bootdevices() fails >>> >>> Should never happen, because we've called it in main() already, >>> treating failure as fatal error. >> >> Yes. >> >>> >> >>> * boot_set_handler is null >>> >>> MachineClass method init() may set this. main() could *easily* test >>> whether it did! If it didn't, and -boot once is given, error out. >>> Similar checks exist already, e.g. drive_check_orphaned(), >>> net_check_clients(). They only warn, but that's detail. >> >> I agree, just need to report the error message. >> >> Regards, >> -Gonglei