libxl: build fix

libxl: build fix

[Christoph Egger]

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


Hi!

Attached patch makes libxl build again on NetBSD.

The build error is:
warning: array subscript has type 'char'
Note, that we compile libxl with -Werror

Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

[-- Attachment #2: xen_libxl.diff --]
[-- Type: text/x-diff, Size: 413 bytes --]

diff -r 897d8b86deae tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Tue May 25 10:01:18 2010 +0200
+++ b/tools/libxl/xl_cmdimpl.c	Tue May 25 11:32:49 2010 +0200
@@ -1226,7 +1226,7 @@ static int64_t parse_mem_size_kb(char *m
     if (strlen(endptr) > 1)
         return -1;
 
-    switch (tolower(*endptr)) {
+    switch (tolower((uint8_t)*endptr)) {
     case 't':
         kbytes <<= 10;
     case 'g':

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: libxl: build fix

[Ian Jackson]

Christoph Egger writes ("[Xen-devel] libxl: build fix"):
> Attached patch makes libxl build again on NetBSD.
> 
> The build error is:
> warning: array subscript has type 'char'
> Note, that we compile libxl with -Werror

This patch is an improvement.  But the correct approach is to cast to
unsigned char, not uint8_t.  (In the context of Xen this doesn't
matter since our unsigned chars are all uint8_t anyway.)

Ian.

libxl: build fix

[Christoph Egger]

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


Hi!

Attached patch fixes libxl build.
libgen.h is needed for basename().

Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>

-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

[-- Attachment #2: xen_buildxl.diff --]
[-- Type: text/x-diff, Size: 322 bytes --]

diff -r be2550c2aa2a tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Fri Nov 05 15:47:17 2010 +0100
+++ b/tools/libxl/xl_cmdimpl.c	Fri Nov 05 16:07:17 2010 +0100
@@ -35,6 +35,7 @@
 #include <xenctrl.h>
 #include <ctype.h>
 #include <inttypes.h>
+#include <libgen.h>
 
 #include "libxl.h"
 #include "libxl_utils.h"

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: libxl: build fix

[Ian Jackson]

Christoph Egger writes ("[Xen-devel] libxl: build fix"):
> Attached patch fixes libxl build.
> libgen.h is needed for basename().

Nacked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Did you test this change ?

Looking at the manpage I have here, and the xl code, the version of
basename() expected by the libxl cpuid code is the GNU one, not the
POSIX one, and they have different semantics.  I think we will have to
open code an implementation of basename.

I see that there is a lot of const-correctness misssing; if filename
had been declared const char* as it should your compiler would have
spotted the problem.

Ian.

Re: libxl: build fix

[Christoph Egger]

On Monday 08 November 2010 17:38:18 Ian Jackson wrote:
> Christoph Egger writes ("[Xen-devel] libxl: build fix"):
> > Attached patch fixes libxl build.
> > libgen.h is needed for basename().
>
> Nacked-by: Ian Jackson <ian.jackson@eu.citrix.com>
>
> Did you test this change ?

Sure. W/o that patch, compiling fails with
"implicit declaration" on NetBSD.

> Looking at the manpage I have here,

How does it differ to mine [1] ?

> and the xl code, the version of 
> basename() expected by the libxl cpuid code is the GNU one,
> not the POSIX one, and they have different semantics.  I think we will have
> to open code an implementation of basename.

Can you imagine to make Xen tools prefer POSIX over GNU in general, please?

> I see that there is a lot of const-correctness misssing; if filename
> had been declared const char* as it should your compiler would have
> spotted the problem.

The compiler is pretty quiet w/o  -Wconst-char -Wwrite-strings
in respect to const-correctness.

[1] http://netbsd.gw.com/cgi-bin/man-cgi?basename+3+NetBSD-current

Christoph


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Re: libxl: build fix

[Ian Jackson]

Christoph Egger writes ("Re: [Xen-devel] libxl: build fix"):
> On Monday 08 November 2010 17:38:18 Ian Jackson wrote:
> > Did you test this change ?
> 
> Sure. W/o that patch, compiling fails with
> "implicit declaration" on NetBSD.

No, I mean: did you execute the new code ?  Never mind.

> > Looking at the manpage I have here,
> 
> How does it differ to mine [1] ?
> [1] http://netbsd.gw.com/cgi-bin/man-cgi?basename+3+NetBSD-current

Look at the code and you will see that it assumes that basename does
not modify its argument.  I'm about to push a const-correctness fix
which will make this more obvious and cause your broken attempt at a
fix not to compile :-).

> > and the xl code, the version of 
> > basename() expected by the libxl cpuid code is the GNU one,
> > not the POSIX one, and they have different semantics.  I think we will have
> > to open code an implementation of basename.
> 
> Can you imagine to make Xen tools prefer POSIX over GNU in general, please?

I agree that the current code is a mistake.  The POSIX basename is
irritating and we shouldn't use it.  Simply open coding a search for a
'/' will be fine.

Ian.

Re: libxl: build fix

[Christoph Egger]

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

On Monday 08 November 2010 18:01:54 Ian Jackson wrote:
> Look at the code and you will see that it assumes that basename does
> not modify its argument.  I'm about to push a const-correctness fix
> which will make this more obvious and cause your broken attempt at a
> fix not to compile :-).

I rebased my source tree and yes, it does not compile.

> > > and the xl code, the version of
> > > basename() expected by the libxl cpuid code is the GNU one,
> > > not the POSIX one, and they have different semantics.  I think we will
> > > have to open code an implementation of basename.
> >
> > Can you imagine to make Xen tools prefer POSIX over GNU in general,
> > please?
>
> I agree that the current code is a mistake.  The POSIX basename is
> irritating and we shouldn't use it.  Simply open coding a search for a
> '/' will be fine.

Attached patch replaces basename with strrchr(3).

Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

[-- Attachment #2: xen_buildxl.diff --]
[-- Type: text/x-diff, Size: 771 bytes --]

diff -r fcdab7bf732b -r 35070850561e tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -5323,7 +5323,7 @@ int main_tmem_freeable(int argc, char **
 
 int main_cpupoolcreate(int argc, char **argv)
 {
-    const const char *filename = NULL;
+    const char *filename = NULL;
     const char *p;
     char extra_config[1024];
     int dryrun = 0;
@@ -5430,7 +5430,7 @@ int main_cpupoolcreate(int argc, char **
     if (!xlu_cfg_get_string (config, "name", &buf))
         name = strdup(buf);
     else
-        name = basename(filename);
+        name = strrchr(filename, '/');
     if (!libxl_name_to_cpupoolid(&ctx, name, &poolid)) {
         fprintf(stderr, "Pool name \"%s\" already exists\n", name);
         return -ERROR_FAIL;

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: libxl: build fix

[Ian Jackson]

Christoph Egger writes ("Re: [Xen-devel] libxl: build fix"):
> Attached patch replaces basename with strrchr(3).
...
> -        name = basename(filename);
> +        name = strrchr(filename, '/');

*ahem*

Ian.

Re: libxl: build fix

[Christoph Egger]

On Tuesday 09 November 2010 12:54:45 Ian Jackson wrote:
> Christoph Egger writes ("Re: [Xen-devel] libxl: build fix"):
> > Attached patch replaces basename with strrchr(3).
>
> ...
>
> > -        name = basename(filename);
> > +        name = strrchr(filename, '/');

Sorry, should be

 name = strrchr(filename, '/') + 1;

Christoph


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Re: libxl: build fix

[John Haxby]

On 09/11/10 12:58, Christoph Egger wrote:
> On Tuesday 09 November 2010 12:54:45 Ian Jackson wrote:
>> Christoph Egger writes ("Re: [Xen-devel] libxl: build fix"):
>>> Attached patch replaces basename with strrchr(3).
>> ...
>>
>>> -        name = basename(filename);
>>> +        name = strrchr(filename, '/');
> Sorry, should be
>
>   name = strrchr(filename, '/') + 1;
>

No, it should be

     name = strrchr(filename, '/');
     if (name)
         name++;
     else
         name = filename;

In fact even that isn't right because basename(3) discards (or at least 
ignores) trailing '/' characters.

jch

Re: libxl: build fix

[Ian Jackson]

John Haxby writes ("Re: [Xen-devel] libxl: build fix"):
> No, it should be
> 
>      name = strrchr(filename, '/');
>      if (name)
>          name++;
>      else
>          name = filename;

As you say.

> In fact even that isn't right because basename(3) discards (or at least 
> ignores) trailing '/' characters.

In this particular case, there won't be any, because if there were
then opening the cpupool config file would fail.

Ian.