Re: [PATCH 2 of 7 v2] blktap3/tapback: Introduces functionality required to access XenStore

[Ian Campbell <Ian.Campbell@citrix.com>]

On Fri, 2013-01-25 at 14:03 +0000, Thanos Makatos wrote:

> > > + */
> > > +static char *
> > > +vmprintf(const char * const fmt, va_list ap)
> > > +{
> > > +    char *s = NULL;
> > 
> > No need to init s.
> 
> Regarding initialising variables, isn't it a good programming practice
> to initialise variables even if they'll be shortly assigned a value?
> Won't the compiler easily optimise such cases?

It should but that's not why we avoid them.

>  Or does it make it too cumbersome to read?

The problem with "unnecessary" initialisations is that the prevent the
compiler's checks for use of uninitialised variables from functioning as
intended and catching bugs.

This is obviously a very simple contrived case but e.g. if you start
with:
	int x = 0, y = 0;

	x = get_a_value();
	y = another_value();

	use_some_values(x, y)
and someone adds a new case:
	int x = 0, y = 0;

	if (some_condition)
		x = get_a_value();
	else
		y = get_an_alternative_value();
	y = another_value();

	use_some_values(x, y);

But they've accidentally used the wrong variable in the new assignment!
So x is actually uninitialised, although it has been set arbitrarily to
0. This is the sort of thing which is easily missed (especially in more
complex example with multiple code paths etc)

If you just had "int x, y;" this would have resulted in "Variable x used
without being initialised" from the compiler.

This doesn't apply in all cases, and obviously it something of a matter
of taste/judgement, but there we go ;-)

> > However xenstore values generally can contain nulls and are not
> > necessarily NULL terminated (see docs/misc/xenstore.txt), but perhaps
> > the block protocol tightens this restriction such that you can rely on
> > NULL termination (at least in practice)?
> 
> Hm.. I saw this in tools/xenstore/xenstore.h and I though xs_read does end the string with a NULL:
> 
> /* Get the value of a single file, nul terminated.
>  * Returns a malloced value: call free() on it after use.
>  * len indicates length in bytes, not including terminator.
>  */
> void *xs_read(struct xs_handle *h, xs_transaction_t t,
> 	      const char *path, unsigned int *len);
> 
> Am I missing something?

I'm not sure, this isn't consistent with the wire protocol doc (at least
AIUI), but perhaps the library is just hiding some complexity for you.
This is one for Ian J I think.

Ian.