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

[Paolo Bonzini <pbonzini@redhat.com>]

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".

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

Paolo

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