Re: [Qemu-devel] [PATCH v2 1/7] bootindex: add modify_boot_device_path function

[Gonglei (Arei)]

Hi, Gerd

> > > > > > > I think it is ok to allow only *changing* the bootindex.
> > > > > > >
> > > > > > Yes, that's no problem.
> > > > >
> > > > > But then yoy always  will have a old entry where you can take the 
> > > > > suffix
> > > > > from, and you don't need the suffix as parameter for the monitor
> > > > > command.
> > > > >
> > > > No, optional.
> > >
> > > > Because the bootindex property is not a necessary property for devices.
> > > > If a device, such as IDE cdrom haven't attach the bootindex in qemu
> booting
> > > > command line, the global fw_boot_order will not have its entry.
> > >
> > > I'd suggest to simply not support this and throw an error then.
> > >
> > Ok.
> >
> > > > So, we should
> > > > save the suffix as parameter.
> > >
> > > I think it is a bad idea to have the suffix as monitor command
> > > parameter.  It is a implementation detail which the monitor users should
> > > not have to worry about.
> > >
> > Yes. Actually I also have this misgivings.
> >
> > > Easiest way to do this is to allow *changing* an existing bootindex
> > > entry only, and not support *adding* new boot entries.  The user has to
> > > set a bootindex for any device it might want to boot from in the future
> > > then.  I think this is acceptable.
> > >
> > Hmm..
> >
> > > If you want support adding bootentries at runtime (and it probably makes
> > > sense to support removing entries too in that case) the suffix handling
> > > should be reworked.  The suffix could be stored in DeviceState instead
> > > of being passed to the add_bootentry function, so you can add boot
> > > entries later on without expecting the user to know what the correct
> > > suffix is.
> > >
> > That's a good idea! I think this is a good improvement.
> >
> > Any other comments? Thanks!
> >
> Maybe we should save the suffix as parameter in add_boot_device_path()
> 
> There are two reasons:
> 
> 1. the floppy device may have two bootindex, which configure as below:
>  " -global isa-fdc.driveA=drive-fdc0-0-0, bootindexA=xxx \
>   -global isa-fdc.driveB=drive-fdc0-0-1 bootindexB=xxx \"
>  We can see it in isabus_fdc_realize() [hw/block/fdc.c]
> 
> 2. The option rom don't need pass dev to add_bootentry,
>   which realized in rom_add_file() [hw/core/loader.c]
> 
> in those situation, we have to pass suffix to add_bootentry function.
> 
In addition, because an isa-fdc device can be configured two drive,
we have to pass the suffix for changing floppy's bootindex. Both del_bootentry
and add_bootentry need suffix to confirm the unique device.

Because the suffix of bootindex is invisible for common user. Maybe we 
can introduce a query interface for user. Such as hmp monitor command
"info bootindex", which show the information of bootindex have configured,
includeing suffix. But the suffix is optional, maybe just useful for floppy 
disk,
that will be not a serious or trouble problem IMHO. 

So, I want to do those in the next version:
- save the suffix as optional continue.
- allow changing the existing bootindex entry only, 
 not support adding new boot entries.
- rework modify_boot_device_path(), add checking logic for the suffix
 when remove old bootindex entry from global boot order list. 

I'm looking forward to your reply, thanks!

Best regards,
-Gonglei