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

[Gonglei]

On 2015/1/21 0:10, Paolo Bonzini wrote:

> 
> 
> On 19/01/2015 14:23, address@hidden 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);

> 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:

diff --git a/bootdevice.c b/bootdevice.c
index 5914417..546ca9d 100644
--- a/bootdevice.c
+++ b/bootdevice.c
@@ -225,7 +225,18 @@ char *get_boot_devices_list(size_t *size, bool 
ignore_suffixes)
             snprintf(bootpath, bootpathlen, "%s%s", devpath, i->suffix);
             g_free(devpath);
         } else if (devpath) {
-            bootpath = devpath;
+            char *d = NULL;
+            size_t bootpathlen;
+
+            d = qdev_get_own_fw_dev_path_from_handler(i->dev->parent_bus, 
i->dev);
+            if (d) {
+                bootpathlen = strlen(devpath) + strlen(d) + 1;
+                bootpath = g_malloc(bootpathlen);
+                snprintf(bootpath, bootpathlen, "%s%s", devpath, d);
+                g_free(devpath);
+            } else {
+                bootpath = devpath;
+            }
         } else if (!ignore_suffixes) {
             assert(i->suffix);
             bootpath = g_strdup(i->suffix);

But I feel this more complicated. Isn't ?

Regards,
-Gonglei

> Paolo
> 
>> +        if (d) {
>>              l += snprintf(p + l, size - l, "%s", d);
>>              g_free(d);
>>          } else {