| Message ID | 1406271172-1192-2-git-send-email-arei.gonglei@huawei.com |
|---|---|
| State | New |
| Headers | show |
Hi, > +void modify_boot_device_path(int32_t bootindex, DeviceState *dev, > + const char *suffix) > +{ > + FWBootEntry *node, *i; > + > + assert(dev != NULL || suffix != NULL); > + > + 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); > + return; > + } > + /* delete the same original dev */ > + if (i->dev->id && !strcmp(i->dev->id, dev->id)) { > + QTAILQ_REMOVE(&fw_boot_order, i, link); Ok ... > + g_free(i->suffix); > + g_free(i); ... but you should free the old entry later ... > + > + break; > + } > + } > + > + if (bootindex >= 0) { > + node = g_malloc0(sizeof(FWBootEntry)); > + node->bootindex = bootindex; > + node->suffix = g_strdup(suffix); ... because you can just copy the suffix from the old entry here, instead of expecting the caller pass it in. cheers, Gerd
Hi, Gerd > -----Original Message----- > From: Gerd Hoffmann [mailto:kraxel@redhat.com] > Sent: Friday, July 25, 2014 5:46 PM > > Hi, > > > +void modify_boot_device_path(int32_t bootindex, DeviceState *dev, > > + const char *suffix) > > +{ > > + FWBootEntry *node, *i; > > + > > + assert(dev != NULL || suffix != NULL); > > + > > + 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); > > + return; > > + } > > + /* delete the same original dev */ > > + if (i->dev->id && !strcmp(i->dev->id, dev->id)) { > > + QTAILQ_REMOVE(&fw_boot_order, i, link); > > Ok ... > > > + g_free(i->suffix); > > + g_free(i); > > ... but you should free the old entry later ... > > > + > > + break; > > + } > > + } > > + > > + if (bootindex >= 0) { > > + node = g_malloc0(sizeof(FWBootEntry)); > > + node->bootindex = bootindex; > > + node->suffix = g_strdup(suffix); > > ... because you can just copy the suffix from the old entry here, > instead of expecting the caller pass it in. > Okay, agreed. But we should also think about the situation which a device don't have old entry in global fw_boot_order list. So we can do like this: if (suffix) { node->suffix = g_strdup(suffix); } else if (has_old_entry) { node->suffix = g_strdup(old_entry->suffix); } else { node->suffix = NULL; } Best regards, -Gonglei
Hi, > > ... because you can just copy the suffix from the old entry here, > > instead of expecting the caller pass it in. > > > Okay, agreed. > > But we should also think about the situation which a device don't have > old entry in global fw_boot_order list. Throw an error? I think it is ok to allow only *changing* the bootindex. cheers, Gerd
> -----Original Message----- > From: Gerd Hoffmann [mailto:kraxel@redhat.com] > Sent: Monday, July 28, 2014 4:31 PM > Subject: Re: [PATCH v2 1/7] bootindex: add modify_boot_device_path function > > Hi, > > > > ... because you can just copy the suffix from the old entry here, > > > instead of expecting the caller pass it in. > > > > > Okay, agreed. > > > > But we should also think about the situation which a device don't have > > old entry in global fw_boot_order list. > > Throw an error? > No. I should firstly use the suffix which the caller pass in IMHO, just like V3 I have posted, Thanks. > I think it is ok to allow only *changing* the bootindex. > Yes, that's no problem. Best regards, -Gonglei
> > 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. cheers, Gerd
Hi, > -----Original Message----- > From: Gerd Hoffmann [mailto:kraxel@redhat.com] > Sent: Monday, July 28, 2014 5:28 PM > Subject: Re: [PATCH v2 1/7] bootindex: add modify_boot_device_path function > > > > 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. So, we should save the suffix as parameter. Best regards, -Gonglei
Hi, > > > > 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. > 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. 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. 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. cheers, Gerd
> -----Original Message----- > From: Gerd Hoffmann [mailto:kraxel@redhat.com] > Sent: Monday, July 28, 2014 6:02 PM > Subject: Re: [PATCH v2 1/7] bootindex: add modify_boot_device_path function > > Hi, > > > > > > 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! Best regards, -Gonglei
Hi, > -----Original Message----- > From: Gonglei (Arei) > Sent: Monday, July 28, 2014 6:16 PM > Subject: RE: [PATCH v2 1/7] bootindex: add modify_boot_device_path function > > > -----Original Message----- > > From: Gerd Hoffmann [mailto:kraxel@redhat.com] > > Sent: Monday, July 28, 2014 6:02 PM > > Subject: Re: [PATCH v2 1/7] bootindex: add modify_boot_device_path function > > > > Hi, > > > > > > > > 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. Best regards, -Gonglei
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
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h index d8539fd..e1b0659 100644 --- a/include/sysemu/sysemu.h +++ b/include/sysemu/sysemu.h @@ -209,6 +209,8 @@ void usb_info(Monitor *mon, const QDict *qdict); void add_boot_device_path(int32_t bootindex, DeviceState *dev, const char *suffix); +void modify_boot_device_path(int32_t bootindex, DeviceState *dev, + const char *suffix); char *get_boot_devices_list(size_t *size, bool ignore_suffixes); DeviceState *get_boot_device(uint32_t position); diff --git a/vl.c b/vl.c index fe451aa..83d7dc4 100644 --- a/vl.c +++ b/vl.c @@ -1248,6 +1248,48 @@ void add_boot_device_path(int32_t bootindex, DeviceState *dev, QTAILQ_INSERT_TAIL(&fw_boot_order, node, link); } +void modify_boot_device_path(int32_t bootindex, DeviceState *dev, + const char *suffix) +{ + FWBootEntry *node, *i; + + assert(dev != NULL || suffix != NULL); + + 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); + return; + } + /* delete the same original dev */ + if (i->dev->id && !strcmp(i->dev->id, dev->id)) { + QTAILQ_REMOVE(&fw_boot_order, i, link); + g_free(i->suffix); + g_free(i); + + break; + } + } + + if (bootindex >= 0) { + node = g_malloc0(sizeof(FWBootEntry)); + node->bootindex = bootindex; + node->suffix = g_strdup(suffix); + node->dev = dev; + + /* add to the global boot list */ + QTAILQ_FOREACH(i, &fw_boot_order, link) { + if (i->bootindex < bootindex) { + continue; + } + QTAILQ_INSERT_BEFORE(i, node, link); + return; + } + + QTAILQ_INSERT_TAIL(&fw_boot_order, node, link); + } +} + DeviceState *get_boot_device(uint32_t position) { uint32_t counter = 0;