[Libvir] [PATCH] Support Sun's Compiler
The following patch allows libvirt to be compiled with Sun's CC compiler. tested on today's CVS bits on a FC7 dom0 (LD_PRELOAD=src/.libs/libvirt.so src/.libs/virsh) Thanks, MRJ
On Fri, Sep 07, 2007 at 11:33:48AM -0400, Mark Johnson wrote:
The following patch allows libvirt to be compiled with Sun's CC compiler.
Small request - for any future patches could you make sure gmail sends them as text/plain rather tha application/octet-stream, or have them inline instead of attachments - makes it easier to reply quoting the patch. So, the bulk of this patch is just getting rid of the anonymous members in the union. Looks huge, but its an obvious safe fix - I explored doing this myself before to make us compile in c89 mode, but decided it wasn't worth it at the time, since we've a tonne of other stuff which breaks in c89 mode already. I'm rather puzzelled about this: - free(names[i]); + free((void *)names[i]); There should never be any need to cast to (void*) as far as I understand things. There's a couple more examples of this. What error does the Sun compiler give without this explicit cast ? For this chunk, I assume the compiler was complaining about unreachable code since all branches in the switch() returned ? default: - return gettext_noop("no state"); /* = dom0 state */ - } - return NULL; + ;/*FALLTHROUGH*/ + } + return gettext_noop("no state"); /* = dom0 state */ Can you explain this chunk: /* As of Hypervisor Call v2, DomCtl v5 we are now 8-byte aligned even on 32-bit archs when dealing with uint64_t */ +#ifdef __linux__ #define ALIGN_64 __attribute__((aligned(8))) +#else +#define ALIGN_64 +#endif The alignment to 8 byte boundaries is neccessary for the Xen Dom0 ABI when running on 32-bit platforms since it has to be 32/64-bit invariant. Is this a mistake, or is the Solaris 32-bit Xen ABI different ? If so can you add a comment about the Solaris ABI difference so we remember the reason for this conditional in the future. Aside from those things I'm fine with the patch. 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 9/7/07, Daniel P. Berrange <berrange@redhat.com> wrote:
On Fri, Sep 07, 2007 at 11:33:48AM -0400, Mark Johnson wrote:
The following patch allows libvirt to be compiled with Sun's CC compiler.
Small request - for any future patches could you make sure gmail sends them as text/plain rather tha application/octet-stream, or have them inline instead of attachments - makes it easier to reply quoting the patch.
Will do..
So, the bulk of this patch is just getting rid of the anonymous members in the union. Looks huge, but its an obvious safe fix - I explored doing this myself before to make us compile in c89 mode, but decided it wasn't worth it at the time, since we've a tonne of other stuff which breaks in c89 mode already.
I'm rather puzzelled about this:
- free(names[i]); + free((void *)names[i]);
There should never be any need to cast to (void*) as far as I understand things. There's a couple more examples of this. What error does the Sun compiler give without this explicit cast ?
Hmm, strange... Yeah a cast is not needed here. Was this something other than a char ** sometime in the past? I will yank those out.
For this chunk, I assume the compiler was complaining about unreachable code since all branches in the switch() returned ?
Yes.
default: - return gettext_noop("no state"); /* = dom0 state */ - } - return NULL; + ;/*FALLTHROUGH*/ + } + return gettext_noop("no state"); /* = dom0 state */
Can you explain this chunk:
/* As of Hypervisor Call v2, DomCtl v5 we are now 8-byte aligned even on 32-bit archs when dealing with uint64_t */ +#ifdef __linux__ #define ALIGN_64 __attribute__((aligned(8))) +#else +#define ALIGN_64 +#endif
The alignment to 8 byte boundaries is neccessary for the Xen Dom0 ABI when running on 32-bit platforms since it has to be 32/64-bit invariant. Is this a mistake, or is the Solaris 32-bit Xen ABI different ? If so can you add a comment about the Solaris ABI difference so we remember the reason for this conditional in the future.
It's a mistake. I'll remove this too... I'll re-submit the patch later with the fixes... Thanks, MRJ
On Fri, Sep 07, 2007 at 05:05:15PM -0400, Mark Johnson wrote:
On 9/7/07, Daniel P. Berrange <berrange@redhat.com> wrote:
So, the bulk of this patch is just getting rid of the anonymous members in the union. Looks huge, but its an obvious safe fix - I explored doing this myself before to make us compile in c89 mode, but decided it wasn't worth it at the time, since we've a tonne of other stuff which breaks in c89 mode already.
I'm rather puzzelled about this:
- free(names[i]); + free((void *)names[i]);
There should never be any need to cast to (void*) as far as I understand things. There's a couple more examples of this. What error does the Sun compiler give without this explicit cast ?
Hmm, strange... Yeah a cast is not needed here. Was this something other than a char ** sometime in the past? I will yank those out.
A while back in the 0.1.x series somewhere these params had the wrong 'const' annotations, so it could be that the compiler was complaining about free'ing a const pointer. So sounds like these are redundant now. Regards, Dam. -- |=- 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 -=|
participants (2)
-
Daniel P. Berrange -
Mark Johnson