Re: [Qemu-devel] [PATCH 1/4] qdev: support to get a device firmware path directly

[Gonglei <arei.gonglei@huawei.com>]

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.gonglei@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