Re: [Xen-devel] [blktap2] fix two 'maybe uninitialized' variables

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

[Adding Thanos (I hope I used the right email address) and Dave (or
should I add someone else, from the XS team perhaps?)

On Thu, Jun 12, 2014 at 1:40 PM, Dario Faggioli <raistlin.df@xxxxxxxxx> wrote:
> So, I did try, and could came up with the patch below.
>
> The thing I'm unsure about is whether I should `return 0' in both the
> cases (I'm quite sure about the first, less about the second). I think
> that is ok, and that's by looking at the code of the function itself, at
> it's doc comment ("return 0 if not allocated"), and at the various call
> sites.
>
> Fact is the function does not look that consistent to me, as far as
> error (if they're error!) reporting is concerned. In fact, the return
> type is uint64_t, and there are places where it returns 0, places where
> it returns -1 and, finally, one place when it returns cluster_offset (at
> the end).
>
> As said above, looking at the doc comment and at the various callers, 0
> seems to be the answer, for instance because all the callers only check
> whether the function returns 0 or !0, without any further investigation
> of the !0 case... However, if the returning of -1 from an uint64_t
> function is done on purpose --e.g., to let the caller know that the
> allocation did happen, but using (uint64_y)-1 as a signal of "but, hey,
> something went wrong!"-- I really can't tell. It really does not look
> like so (and that's why I'm going for 'return 0' in both cases), but I
> know too few about blocktap, qcow2, etc, to be positive about it. :-(
>
> So, I'd have to ask to someone who's more into that code to advise, I'm
> afraid. :-/
>
Any ideas? It's all but urgent, I guess, but there looks to be people
starting to hit this (yeah, well, only 1 guy for now, but still... :-)
)

Dario

> ---
> diff --git a/tools/blktap2/drivers/block-qcow.c 
> b/tools/blktap2/drivers/block-qcow.c
> index d5053d4..b45bcaa 100644
> --- a/tools/blktap2/drivers/block-qcow.c
> +++ b/tools/blktap2/drivers/block-qcow.c
> @@ -427,6 +427,7 @@ static uint64_t get_cluster_offset(struct tdqcow_state *s,
>
>                 if (posix_memalign((void **)&tmp_ptr, 4096, 4096) != 0) {
>                         DPRINTF("ERROR allocating memory for L1 table\n");
> +                        return 0;
>                 }
>                 memcpy(tmp_ptr, l1_ptr, 4096);
>
> @@ -600,6 +601,7 @@ found:
>
>                 if (posix_memalign((void **)&tmp_ptr2, 4096, 4096) != 0) {
>                         DPRINTF("ERROR allocating memory for L1 table\n");
> +                        return 0;
>                 }
>                 memcpy(tmp_ptr2, l2_ptr, 4096);
>                 lseek(s->fd, l2_offset + (l2_sector << 12), SEEK_SET);
>

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
---------------------------------------------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel