Re: [Qemu-devel] [PATCH v2 1/4] block: make bdrv_find_backing_image compare canonical filenames

[Jeff Cody <jcody@redhat.com>]

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 <jcody@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