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