On 2015/1/21 18:52, Paolo Bonzini wrote:
>
>
> On 21/01/2015 03:14, Gonglei wrote:
>> On 2015/1/21 0:10, Paolo Bonzini wrote:
>>
>>>
>>>
>>> On 19/01/2015 14:23, arei.gong...@huawei.com wrote:
>>>> @@ -780,6 +788,12 @@ static int qdev_get_fw_dev_path_helper(DeviceState
>>>> *dev, char *p, int size)
>>>> d = bus_get_fw_dev_path(dev->parent_bus, dev);
>>>> }
>>>> if (d) {
>>>> + l += snprintf(p + l, size - l, "%s/", d);
>>>> + g_free(d);
>>>> + }
>>>> +
>>>> + d = qdev_get_own_fw_dev_path_from_handler(dev->parent_bus, dev);
>>>
>>> This changes preexisting behavior. If d was true, you wouldn't go down
>>> the following else. Now it does.
>>>
>>
>> On the face of things, it is. But actually they are equal. Please notice I
>> added a "/" at the
>> end p, and then the function can return if d was NULL.
>> l += snprintf(p + l, size - l, "%s/", d);
>
> But in this case I think the "return l" should become unconditional.
> It should move out of the "else".
>
After calling qdev_get_own_fw_dev_path_from_handler(), if d was true, there is
also
l += snprintf(p + l, size - l, "%s", d);
we still have to add a '/' too. These two situation are different.
>>> I was thinking it should be handled though the "suffix" argument to
>>> add_boot_device_path, but that's harder now that the suffix has to be
>>> passed to device_add_bootindex_property.
>>>
>>
>> Yes.
>>
>>> Perhaps you could call qdev_get_own_fw_dev_path_from_handler in
>>> get_boot_devices_list, and convert non-NULL suffixes to implementations
>>> of FWPathProvider?
>>
>> Maybe your meaning is "convert NULL suffixes to implementations
>> of FWPathProvider"? Something like below:
>
> No, I meant non-NULL.
>
> In the beginning it can be something like this:
>
> diff --git a/bootdevice.c b/bootdevice.c
> index 5914417..916bfb7 100644
> --- a/bootdevice.c
> +++ b/bootdevice.c
> @@ -210,7 +210,8 @@ char *get_boot_devices_list(size_t *size, bool
> ignore_suffixes)
> char *list = NULL;
>
> QTAILQ_FOREACH(i, &fw_boot_order, link) {
> - char *devpath = NULL, *bootpath;
> + char *devpath = NULL, *suffix = NULL;
> + char *bootpath;
> size_t len;
>
> if (i->dev) {
> @@ -218,21 +219,22 @@ char *get_boot_devices_list(size_t *size, bool
> ignore_suffixes)
> assert(devpath);
> }
>
> - if (i->suffix && !ignore_suffixes && devpath) {
> - size_t bootpathlen = strlen(devpath) + strlen(i->suffix) + 1;
> -
> - bootpath = g_malloc(bootpathlen);
> - snprintf(bootpath, bootpathlen, "%s%s", devpath, i->suffix);
> - g_free(devpath);
> - } else if (devpath) {
> - bootpath = devpath;
> - } else if (!ignore_suffixes) {
> - assert(i->suffix);
> - bootpath = g_strdup(i->suffix);
> - } else {
> - bootpath = g_strdup("");
> + if (!ignore_suffixes) {
> + d = qdev_get_own_fw_dev_path_from_handler(i->dev->parent_bus,
> i->dev);
> + if (d) {
> + assert(!i->suffix);
> + suffix = d;
> + } else {
> + suffix = g_strdup(i->suffix);
> + }
> }
>
> + bootpath = g_strdup_printf("%s%s",
> + devpath ? devpath : "",
> + suffix ? suffix : "");
> + g_free(devpath);
> + g_free(suffix);
> +
> if (total) {
> list[total-1] = '\n';
> }
>
>
> Then as time permits the suffix can be phased out and replaced by
> FWPathProvider on the device.
>
Anyway, I like your approach, it's so clear. :)
Regards,
-Gonglei
