Xen devel - Sep 2010 - [PATCH 2/5] libxl: add xlu_cfg_get_type function

As cpuid= can be used with two syntaxes (one as a list, the other as a
string), we need to distinguish them without an error message. Introduce 
a helper function to detect the type of entry used before issuing a warning.

Signed-off-by: Andre Przywara

-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 448-3567-12


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

As cpuid= can be used with two syntaxes (one as a list, the other as a
string), we need to distinguish them without an error message. Introduce
a helper function to detect the type of entry used before issuing a warning.

Signed-off-by: Andre Przywara

-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 448-3567-12


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Andre Przywara writes ("[Xen-devel] [PATCH 2/5] libxl: add xlu_cfg_get_type
function"):
> As cpuid= can be used with two syntaxes (one as a list, the other as a
> string), we need to distinguish them without an error message. Introduce
> a helper function to detect the type of entry used before issuing a
warning.
Thanks.  This is a generally good idea although I''m not quite
convinced by this:

 +    errno = 0;
 +    l = strtol(set->values[0], &endptr, 0);
 +    if (errno == EINVAL || endptr == set->values[0])
 +        return XLU_CFG_STRING;
 +    return XLU_CFG_LONG;

Firstly, it will fail for unsigned longs bigger than LONG_MAX, and we
would normally think about unsigned longs.  Secondly, if callers say
things like
  if (type == XLU_CFG_STRING) ....
they''ll have a bug.

I would suggest XLU_CFG_ATOM.  Callers can use strto[u]l (or whatever)
themselves if they need to distinguish numbers from strings.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Ian Jackson wrote:
> Andre Przywara writes ("[Xen-devel] [PATCH 2/5] libxl: add
xlu_cfg_get_type function"):
>> As cpuid= can be used with two syntaxes (one as a list, the other as a
>> string), we need to distinguish them without an error message.
Introduce
>> a helper function to detect the type of entry used before issuing a
warning.
> 
> Thanks.  This is a generally good idea although I''m not quite
> convinced by this:
> 
>  +    errno = 0;
>  +    l = strtol(set->values[0], &endptr, 0);
>  +    if (errno == EINVAL || endptr == set->values[0])
>  +        return XLU_CFG_STRING;
>  +    return XLU_CFG_LONG;
> 
> Firstly, it will fail for unsigned longs bigger than LONG_MAX, and we
> would normally think about unsigned longs.
But there is only xlu_cfg_get_long, which returns signed values (used 28 
times in xl_cmdimpl.c). I don''t see any usage of strtoul in
xl_cmdimpl.c
which is preceded by xlu_cfg_get_string().
>  Secondly, if callers say things like
>   if (type == XLU_CFG_STRING) ....
> they''ll have a bug.
> I would suggest XLU_CFG_ATOM.  Callers can use strto[u]l (or whatever)
> themselves if they need to distinguish numbers from strings.
Makes sense. Do you mean like the attached delta patch?

I could also live with making the reporting of the error in 
libxl_cfg_get_list() optional, so that users aren''t bothered with a 
confusing error output everytime. That would make the whole function 
obsolete.

Tell me what you like more.


Regards,
Andre.


-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 448-3567-12


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Andre Przywara writes ("Re: [Xen-devel] [PATCH 2/5] libxl: add
xlu_cfg_get_type function"):
> But there is only xlu_cfg_get_long, which returns signed values (used 28 
> times in xl_cmdimpl.c). I don''t see any usage of strtoul in
xl_cmdimpl.c
> which is preceded by xlu_cfg_get_string().
That''s true, although it may not remain so forever.  But my other
arguments stand I think.
> >  Secondly, if callers say things like
> >   if (type == XLU_CFG_STRING) ....
> > they''ll have a bug.
> > I would suggest XLU_CFG_ATOM.  Callers can use strto[u]l (or whatever)
> > themselves if they need to distinguish numbers from strings.
>
> Makes sense. Do you mean like the attached delta patch?
Right, yes, that seems sensible.
> I could also live with making the reporting of the error in 
> libxl_cfg_get_list() optional, so that users aren''t bothered with
a
> confusing error output everytime. That would make the whole function 
> obsolete.
That would be fine too.
> Tell me what you like more.
My usual rule is "do whatever makes the code smaller".  I guess in
this case that probably means have the error reporting flag on
libxl_cfg_get_list.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel