> From: Gerd Hoffmann [mailto:kra...@redhat.com]
>
> Hi,
>
> > +void set_bootindex(int32_t *bootindex, Visitor *v,
> > > + const char *name, Error **errp)
> > > +{
> > > + int32_t boot_index;
> > > + Error *local_err = NULL;
> > > +
> > > + visit_type_int32(v, &boot_index, name, &local_err);
> > > +
> > > + if (local_err == NULL) {
> > > + /* check the bootindex existes or not in fw_boot_order list */
> > > + check_boot_index(boot_index, &local_err);
> > > + }
> > > +
> > > + if (local_err) {
> > > + error_propagate(errp, local_err);
> > > + return;
> > > + }
> > > + /* change bootindex to a new one */
> > > + *bootindex = boot_index;
> > > +}
> > > +
> >
> > In this version, I just change devices' bootindex value, but not update
> fw_boot_order
> > immediately. When we reboot the vm, we will call add_boot_device_path to
> update
> > fw_boot_order list. Do you think it is a good method? This method will cause
> a problem
> > that when we want set a existed bootindex which has been changed, we will
> get failure.
> > Only after vm rebooting, we can set the same bootindex again.
>
> Good point. check_boot_index() doing the verification against stale
> data is pointless and may even throw an error in cases where it should
> not.
>
Yes.
> I guess we should add a add_boot_device_path() call to the
> ${device}_set_bootindex functions. Which also means we don't need to do
> it on reset (patch #6 can be dropped). And I think we can also drop the
> add_boot_device_path() calls from ${device}_realize then.
>
Bravo! I can't agree more with you. Thanks, Gerd.
Will do it.
Best regards,
-Gonglei
