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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2451 bytes --]

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?

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

> +
> +    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?

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 617 bytes --]