Re: [Xen-devel] [PATCH v5 6/8] libxl: introduce libxl__alloc_vdev

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Stefano Stabellini writes ("Re: [PATCH v5 6/8] libxl: introduce 
libxl__alloc_vdev"):
> On Fri, 4 May 2012, Ian Jackson wrote:
> > All you need to do is actually pay attention to its error return.
> 
> OK
> 
> > > +    return NULL;
> > 
> > All the NULL error exits should log an error surely.
> 
> The error log is in the next patch. Should I add a second log here?
> Or maybe I should move the error log from the caller to the callee?

Well, my view is that a function should either always log if it
returns ERROR_FAIL (or NULL if that's its calling pattern), or never
do so.  And if it logs this should be in a doc comment.

I inferred that the function was supposed to log by the fact that the
other error paths logged (that is, I didn't read the doc comment or
the rest of the code).

I don't mind where the logging is done (although normally it's best to
do it closer to the source of the error) but I do want to make sure
that we don't have cases where there is an exit path which simply
fails without explaining why.

> > > +/* same as in Linux */
> > > +static char *encode_disk_name(char *ptr, unsigned int n)
> > > +{
> > > +    if (n >= 26)
> > > +        ptr = encode_disk_name(ptr, n / 26 - 1);
> > > +    *ptr = 'a' + n % 26;
> > > +    return ptr + 1;
> > > +}
> > 
> > This still makes it difficult to see that the buffer cannot be
> > overrun.  I mentioned this in response to a previous posting of this
> > code and IIRC you were going to do something about it, if only a
> > comment explaining what the maximum buffer length is and why it's
> > safe.
> 
> I am keen on keeping the code as is, because it is the same that is in
> Linux (see comment above).

I read the comment as "this implements the same transformation as the
Linux kernel" not "this is the same code as actually used in the Linux
kernel".  If you mean the latter, do say so.

Also, in that case why is the recursive function name, formatting,
etc. not the same as in Linux (as far as they can be) ?  Surely if
Linux changes this we will want to change our code too and that will
be easiest if we can c&p without needing to reformat, rename, etc.

> I'll add a comment.

OK.  The comments, taken together, should constitute proofs that the
code does not overrun any buffers.

Thanks,
Ian.

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