[Libvir] [PATCH] header file changes for Solaris
This patch has the includes need to build on Solaris. I've been using ifdef linux & ifndef linux to distinguish between solaris and linux at this point. I'm not sure what the preference is... MRJ
On Thu, Jun 14, 2007 at 05:26:39PM -0400, Mark Johnson wrote:
This patch has the includes need to build on Solaris. I've been using ifdef linux & ifndef linux to distinguish between solaris and linux at this point.
Looks ok aside from
+#ifndef __linux__ +#define NAME_MAX 14 +#endif
The #ifndef should test against NAME_MAX itself, rather than __linux__ Also, NAME_MAX is intended to be the maximum length of an unqualified filename, so 14 characters is rather too small. 255 is what Linux has it defined as, so I'd go for that unless Solaris has a different named constant for maximum filename length ? I know BSD uses MAXNAMLEN, but NAME_MAX is POSIX
I'm not sure what the preference is...
I don't think it really matters one way or the other when we're only having to worry about a #ifdef choice between 2 platforms. If someone ports libvirt to BSD then we can adapt to whichever results in smaller code as needed. Regards, Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
On 6/14/07, Daniel P. Berrange <berrange@redhat.com> wrote:
On Thu, Jun 14, 2007 at 05:26:39PM -0400, Mark Johnson wrote:
This patch has the includes need to build on Solaris. I've been using ifdef linux & ifndef linux to distinguish between solaris and linux at this point.
Looks ok aside from
+#ifndef __linux__ +#define NAME_MAX 14 +#endif
The #ifndef should test against NAME_MAX itself, rather than __linux__
Also, NAME_MAX is intended to be the maximum length of an unqualified filename, so 14 characters is rather too small. 255 is what Linux has it defined as, so I'd go for that unless Solaris has a different named constant for maximum filename length ? I know BSD uses MAXNAMLEN, but NAME_MAX is POSIX
Yep, I agree with both... Do yo want me to re-submit the patch or do you want to make those changes?
I'm not sure what the preference is...
I don't think it really matters one way or the other when we're only having to worry about a #ifdef choice between 2 platforms. If someone ports libvirt to BSD then we can adapt to whichever results in smaller code as needed.
great. Thanks! Marj
Regards, Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
On Thu, Jun 14, 2007 at 07:05:49PM -0400, Mark Johnson wrote:
On 6/14/07, Daniel P. Berrange <berrange@redhat.com> wrote:
On Thu, Jun 14, 2007 at 05:26:39PM -0400, Mark Johnson wrote:
This patch has the includes need to build on Solaris. I've been using ifdef linux & ifndef linux to distinguish between solaris and linux at this point.
Looks ok aside from
+#ifndef __linux__ +#define NAME_MAX 14 +#endif
The #ifndef should test against NAME_MAX itself, rather than __linux__
Also, NAME_MAX is intended to be the maximum length of an unqualified filename, so 14 characters is rather too small. 255 is what Linux has it defined as, so I'd go for that unless Solaris has a different named constant for maximum filename length ? I know BSD uses MAXNAMLEN, but NAME_MAX is POSIX
Yep, I agree with both... Do yo want me to re-submit the patch or do you want to make those changes?
No need - its quick enough to change this when its applied to CVS. Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
Daniel P. Berrange wrote:
On Thu, Jun 14, 2007 at 05:26:39PM -0400, Mark Johnson wrote:
This patch has the includes need to build on Solaris. I've been using ifdef linux & ifndef linux to distinguish between solaris and linux at this point.
Looks ok aside from [..]
No, I don't agree. We should use configure.in to test for the presence of header files and then do things like: #ifdef HAVE_STRINGS_H #include <strings.h> #endif See the current configure.in, AC_CHECK_HEADERS. In fact we already have the HAVE_STRINGS_H symbol defined. Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
On Fri, Jun 15, 2007 at 09:06:55AM +0100, Richard W.M. Jones wrote:
Daniel P. Berrange wrote:
On Thu, Jun 14, 2007 at 05:26:39PM -0400, Mark Johnson wrote:
This patch has the includes need to build on Solaris. I've been using ifdef linux & ifndef linux to distinguish between solaris and linux at this point.
Looks ok aside from [..]
No, I don't agree. We should use configure.in to test for the presence of header files and then do things like:
#ifdef HAVE_STRINGS_H #include <strings.h> #endif
See the current configure.in, AC_CHECK_HEADERS.
In fact we already have the HAVE_STRINGS_H symbol defined.
Agreed. And in the case of system specific header use the system specific macro to guard e.g.: + +#ifndef __linux__ +#include <iso/limits_iso.h> +#endif should probably be rewritten as #ifdef __sun__ #include <iso/limits_iso.h> #endif or with whatever the macro should be on your platform, so that if someone tries to compile on say AIX it doesn't break on an inexistant header but rather if there is a missing reference in the code itself. thanks, Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On 6/15/07, John Levon <levon@movementarian.org> wrote:
On Fri, Jun 15, 2007 at 05:21:30AM -0400, Daniel Veillard wrote:
+#ifndef __linux__ +#include <iso/limits_iso.h> +#endif
This looks like an odd header to include anyway. Mark why isn't limits.h enough?
I'll check in a little bit.. I'll change this to sun (assuming there's a good reason for the include). As far a strings.h goes, I'm not sure how HAVE_STRINGS_H would help us. Not that I understand configure* very well... In this case, both linux and solaris have string.h and strings.h. There are some things defined in string.h in linux which are in strings.h in solaris. Mark.
On Fri, Jun 15, 2007 at 09:06:55AM +0100, Richard W.M. Jones wrote:
Daniel P. Berrange wrote:
On Thu, Jun 14, 2007 at 05:26:39PM -0400, Mark Johnson wrote:
This patch has the includes need to build on Solaris. I've been using ifdef linux & ifndef linux to distinguish between solaris and linux at this point.
Looks ok aside from [..]
No, I don't agree. We should use configure.in to test for the presence of header files and then do things like:
#ifdef HAVE_STRINGS_H #include <strings.h> #endif
For strings.h I don't see the point in making it conditional really, unless we're going to do the same for every single other header we include. The strings.h header is always present on Linux. In recent times stuff that was previously in strings.h has moved to string.h, but they're still in the original header too. So we should always include both string.h & strings.h for maximum portability. Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
On Fri, Jun 15, 2007 at 01:59:41PM +0100, Daniel P. Berrange wrote:
On Fri, Jun 15, 2007 at 09:06:55AM +0100, Richard W.M. Jones wrote:
Daniel P. Berrange wrote:
On Thu, Jun 14, 2007 at 05:26:39PM -0400, Mark Johnson wrote:
This patch has the includes need to build on Solaris. I've been using ifdef linux & ifndef linux to distinguish between solaris and linux at this point.
Looks ok aside from [..]
No, I don't agree. We should use configure.in to test for the presence of header files and then do things like:
#ifdef HAVE_STRINGS_H #include <strings.h> #endif
For strings.h I don't see the point in making it conditional really, unless we're going to do the same for every single other header we include. The strings.h header is always present on Linux. In recent times stuff that was previously in strings.h has moved to string.h, but they're still in the original header too. So we should always include both string.h & strings.h for maximum portability.
Hum, I don't think they are really the same. In libxml2 I do a configure test for HAVE_STRINGS_H but string.h is included without checks in a lot of places. Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Fri, Jun 15, 2007 at 09:03:42AM -0400, Daniel Veillard wrote:
On Fri, Jun 15, 2007 at 01:59:41PM +0100, Daniel P. Berrange wrote:
On Fri, Jun 15, 2007 at 09:06:55AM +0100, Richard W.M. Jones wrote:
Daniel P. Berrange wrote:
On Thu, Jun 14, 2007 at 05:26:39PM -0400, Mark Johnson wrote:
This patch has the includes need to build on Solaris. I've been using ifdef linux & ifndef linux to distinguish between solaris and linux at this point.
Looks ok aside from [..]
No, I don't agree. We should use configure.in to test for the presence of header files and then do things like:
#ifdef HAVE_STRINGS_H #include <strings.h> #endif
For strings.h I don't see the point in making it conditional really, unless we're going to do the same for every single other header we include. The strings.h header is always present on Linux. In recent times stuff that was previously in strings.h has moved to string.h, but they're still in the original header too. So we should always include both string.h & strings.h for maximum portability.
Hum, I don't think they are really the same. In libxml2 I do a configure test for HAVE_STRINGS_H but string.h is included without checks in a lot of places.
They're not the same - I string.h is a superset of strings.h on Linux, but on other places they're disjoint sets. Comment in strings.h... /* We don't need and should not read this file if <string.h> was already read. The one exception being that if __USE_BSD isn't defined, then these aren't defined in string.h, so we need to define them here. */ Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
Daniel P. Berrange wrote:
On Fri, Jun 15, 2007 at 09:06:55AM +0100, Richard W.M. Jones wrote:
Daniel P. Berrange wrote:
On Thu, Jun 14, 2007 at 05:26:39PM -0400, Mark Johnson wrote:
This patch has the includes need to build on Solaris. I've been using ifdef linux & ifndef linux to distinguish between solaris and linux at this point. Looks ok aside from [..]
No, I don't agree. We should use configure.in to test for the presence of header files and then do things like:
#ifdef HAVE_STRINGS_H #include <strings.h> #endif
For strings.h I don't see the point in making it conditional really, unless we're going to do the same for every single other header we include. The strings.h header is always present on Linux. In recent times stuff that was previously in strings.h has moved to string.h, but they're still in the original header too. So we should always include both string.h & strings.h for maximum portability.
<string.h> is in ISO C, but <strings.h> is not. So as Daniel Veillard said you can include the former without checks, but the latter should be defended with an #ifdef HAVE_STRINGS_H. On BSD, <strings.h> has the legacy functions like bcopy and index. Are we using those? Shouldn't we instead replace any instances with memcpy / memmove / strchr / strrchr? There are apparently some platforms where you can't include both ... urrgh: http://gcc.gnu.org/ml/gcc-patches/1998-08/msg00317.html Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
As johnlev said, I can fix the limits_iso.h with limits.h
<string.h> is in ISO C, but <strings.h> is not. So as Daniel Veillard said you can include the former without checks, but the latter should be defended with an #ifdef HAVE_STRINGS_H.
On BSD, <strings.h> has the legacy functions like bcopy and index. Are we using those? Shouldn't we instead replace any instances with memcpy / memmove / strchr / strrchr?
It's needed because of index().
There are apparently some platforms where you can't include both ... urrgh: http://gcc.gnu.org/ml/gcc-patches/1998-08/msg00317.html
That can break on BSD and solaris since they have both string.h and strings.h which don't overlap.
On 6/15/07, Mark Johnson <johnson.nh@gmail.com> wrote:
As johnlev said, I can fix the limits_iso.h with limits.h
<string.h> is in ISO C, but <strings.h> is not. So as Daniel Veillard said you can include the former without checks, but the latter should be defended with an #ifdef HAVE_STRINGS_H.
On BSD, <strings.h> has the legacy functions like bcopy and index. Are we using those? Shouldn't we instead replace any instances with memcpy / memmove / strchr / strrchr?
It's needed because of index().
I guess not anymore :-) MRJ
There are apparently some platforms where you can't include both ... urrgh: http://gcc.gnu.org/ml/gcc-patches/1998-08/msg00317.html
That can break on BSD and solaris since they have both string.h and strings.h which don't overlap.
On Fri, Jun 15, 2007 at 09:32:13AM -0400, Mark Johnson wrote:
As johnlev said, I can fix the limits_iso.h with limits.h
<string.h> is in ISO C, but <strings.h> is not. So as Daniel Veillard said you can include the former without checks, but the latter should be defended with an #ifdef HAVE_STRINGS_H.
On BSD, <strings.h> has the legacy functions like bcopy and index. Are we using those? Shouldn't we instead replace any instances with memcpy / memmove / strchr / strrchr?
It's needed because of index().
please use strchr() let's get rid of strings.h , that will be way simpler. Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
OK, I'll make another stab at this patch with all the comments and send it out shortly. Thanks, Mark
Mark Johnson wrote:
updated header file patch attached. This one is much nicer. :-)
I think that's fine, except that I will change: +#ifdef __linux__ #include <paths.h> +#endif to: +#ifdef HAVE_PATHS_H #include <paths.h> +#endif and add the appropriate detection to configure.in. If everyone else is happy with that then I can commit it. Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
On Fri, Jun 15, 2007 at 04:00:01PM +0100, Richard W.M. Jones wrote:
Mark Johnson wrote:
updated header file patch attached. This one is much nicer. :-)
I think that's fine, except that I will change:
+#ifdef __linux__ #include <paths.h> +#endif
to:
+#ifdef HAVE_PATHS_H #include <paths.h> +#endif
and add the appropriate detection to configure.in. If everyone else is happy with that then I can commit it.
+1 Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
Mark: Committed. Can you just check that it still fixes Solaris compiles over there? Also, thank you for your contribution to libvirt. Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
participants (5)
-
Daniel P. Berrange -
Daniel Veillard -
John Levon -
Mark Johnson -
Richard W.M. Jones