On 9/7/07, Daniel P. Berrange <berrange@xxxxxxxxxx> 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
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list