On 10/16/2012 11:12 AM, Eric Blake wrote:
> On 10/16/2012 08:44 AM, Jeff Cody wrote:
>> Currently, bdrv_find_backing_image compares bs->backing_file with
>> what is passed in as a backing_file name. Mismatches may occur,
>> however, when bs->backing_file and backing_file are both not
>
> Reads better as s/both not/not both/.
>
>> absolute or relative.
>>
>> Use path_combine() to make sure any relative backing filenames are
>> relative to the current image filename being searched, and then use
>> realpath() to make all comparisons based on absolute filenames.
>>
>> This also changes bdrv_find_backing_image to no longer be recursive,
>> but iterative.
>>
>> Signed-off-by: Jeff Cody <jc...@redhat.com>
>> ---
>> block.c | 64
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
>> 1 file changed, 58 insertions(+), 6 deletions(-)
>
>> - if (!bs->drv) {
>> + char *filename_full = NULL;
>> + char *backing_file_full = NULL;
>> + char *filename_tmp = NULL;
>> + int is_protocol = 0;
>
> Any reason you didn't use bool here?
>
path_has_protocol() returns an int, so I preferred to just match that.
>> + BlockDriverState *curr_bs = NULL;
>> + BlockDriverState *retval = NULL;
>> +
>> + if (!bs || !bs->drv || !backing_file) {
>> return NULL;
>> }
>>
>> - if (bs->backing_hd) {
>> - if (strcmp(bs->backing_file, backing_file) == 0) {
>> - return bs->backing_hd;
>> + filename_full = g_malloc(sizeof(char) * PATH_MAX);
>
> sizeof(char) is guaranteed to be 1; this can be simplified to
> g_malloc(PATH_MAX).
>
>> + backing_file_full = g_malloc(sizeof(char) * PATH_MAX);
>> + filename_tmp = g_malloc(sizeof(char) * PATH_MAX);
>> + if (!filename_full || !backing_file_full || !filename_tmp) {
>> + goto error;
>> + }
>
> Dead 'if', since g_malloc() is guaranteed to succeed.
>
I'll go ahead and simplify both of the above with the next spin.
>> +
>> + is_protocol = path_has_protocol(backing_file);
>> +
>> + for (curr_bs = bs; curr_bs->backing_hd; curr_bs = curr_bs->backing_hd) {
>> +
>> + /* If either of the filename paths is actually a protocol, then
>> + * compare unmodified paths; otherwise make paths relative */
>> + if (is_protocol || path_has_protocol(curr_bs->backing_file)) {
>> + if (strcmp(backing_file, curr_bs->backing_file) == 0) {
>
> I guess we are guaranteed that if is_protocol and path_has_protocol()
> give different answers, then the strcmp() will fail?
>
Yes, since is_protocol is determined via path_has_protocol() - if
they are different, then by definition strcmp() should fail.
Jeff
