On Wed, Jun 25, 2008 at 05:17:56PM -0700, Ryan Scott wrote:
>
> Hi,
>
> Most of the changes in the attached patch are trivial #ifdef
> corrections for Solaris compilation.
>
> The biggest part of the change gets 'virsh capabilities' running
> correctly under Xen on Solaris.
>
> Signed-off-by: Mark Johnson <Mark.Johnson@xxxxxxx>
>
>
> --- a/src/xs_internal.c
> +++ b/src/xs_internal.c
> @@ -35,7 +35,7 @@
>
> #ifdef __linux__
> #define XEN_HYPERVISOR_SOCKET "/proc/xen/privcmd"
> -#elif define(__sun__)
> +#elif defined(__sun)
> #define XEN_HYPERVISOR_SOCKET "/dev/xen/privcmd"
> #else
> #error "unsupported platform"
> --- a/src/xen_internal.c
> +++ b/src/xen_internal.c
> @@ -75,7 +75,7 @@ typedef struct v1_hypercall_struct
> #define XEN_V1_IOCTL_HYPERCALL_CMD \
> _IOC(_IOC_NONE, 'P', 0, sizeof(v1_hypercall_t))
> typedef v1_hypercall_t hypercall_t;
> -#elif define(__sun__)
> +#elif defined(__sun)
> typedef privcmd_hypercall_t hypercall_t;
> #else
> #error "unsupported platform"
> @@ -340,8 +340,10 @@ lock_pages(void *addr, size_t len)
> {
> #ifdef __linux__
> return (mlock(addr, len));
> -#elif define(__sun)
> +#elif defined(__sun)
> return (0);
> +#else
> +#error "unsupported platform"
> #endif
> }
>
> @@ -350,8 +352,10 @@ unlock_pages(void *addr, size_t len)
> {
> #ifdef __linux__
> return (munlock(addr, len));
> -#elif define(__sun)
> +#elif defined(__sun)
> return (0);
> +#else
> +#error "unsupported platform"
> #endif
> }
>
> @@ -666,7 +670,7 @@ typedef struct xen_op_v2_dom xen_op_v2_d
> #define XEN_HYPERVISOR_SOCKET "/proc/xen/privcmd"
> #define HYPERVISOR_CAPABILITIES "/sys/hypervisor/properties/capabilities"
> #define CPUINFO "/proc/cpuinfo"
> -#elif define(__sun__)
> +#elif defined(__sun)
> #define XEN_HYPERVISOR_SOCKET "/dev/xen/privcmd"
> #define HYPERVISOR_CAPABILITIES ""
> #define CPUINFO "/dev/cpu/self/cpuid"
> @@ -1948,7 +1952,7 @@ xenHypervisorInit(void)
> goto detect_v2;
> }
>
> -#ifndef __sun__
> +#ifndef __sun
> /*
> * check if the old hypercall are actually working
> */
> @@ -2265,6 +2269,92 @@ xenHypervisorBuildCapabilities(virConnec
> return NULL;
> }
>
> +#ifdef __linux__
> +void
> +loadCapabilities(FILE *cpuinfo, FILE *capabilities, char *hvm_type,
> + int *host_pae, char *line, int LINE_SIZE)
> +{
> + regmatch_t subs[4];
> +
> + /* /proc/cpuinfo: flags: Intel calls HVM "vmx", AMD calls it "svm".
> + * It's not clear if this will work on IA64, let alone other
> + * architectures and non-Linux. (XXX)
> + */
> + if (cpuinfo) {
> + while (fgets (line, LINE_SIZE, cpuinfo)) {
> + if (regexec (&flags_hvm_rec, line,
> + sizeof(subs)/sizeof(regmatch_t), subs, 0) == 0
> + && subs[0].rm_so != -1) {
> + strncpy (hvm_type,
> + &line[subs[1].rm_so], subs[1].rm_eo-subs[1].rm_so+1);
> + hvm_type[subs[1].rm_eo-subs[1].rm_so] = '\0';
> + } else if (regexec (&flags_pae_rec, line, 0, NULL, 0) == 0)
> + *host_pae = 1;
> + }
> + }
> +
> + /* Expecting one line in this file - ignore any more. */
> + (void) fgets(line, LINE_SIZE, capabilities);
> +}
> +
> +#else
i think this should be
#elif defined(__sun)
or something similar i see things like &rp->r_ebx; I don't think it's fully
platform agnostic, right ?
> +void
> +loadCapabilities(FILE *cpuinfo, FILE *capabilities, char *hvm_type,
> + int *host_pae, char *line, int LINE_SIZE)
> +{
> + struct {
> + uint32_t r_eax, r_ebx, r_ecx, r_edx;
> + } _r, *rp = &_r;
> + int d, cmd;
> + char *s;
> + hypercall_t hc;
> +
> +
> + if ((d = open(CPUINFO, O_RDONLY)) == -1) {
> + goto cpuinfo_open_fail;
> + }
> +
> + if (pread(d, rp, sizeof (*rp), 0) != sizeof (*rp)) {
> + goto cpuinfo_pread_fail;
> + }
> +
> + s = (char *)&rp->r_ebx;
> + if (strncmp(s, "Auth" "cAMD" "enti", 12) == 0) {
> + if (pread(d, rp, sizeof (*rp), 0x80000001) == sizeof (*rp)) {
> + /* Read secure virtual machine bit (bit 2 of ECX feature ID) */
> + if ((rp->r_ecx >> 2) & 1) {
> + strcpy(hvm_type, "svm");
> + }
> + if ((rp->r_edx >> 6) & 1) {
> + *host_pae = 1;
> + }
> + }
> + } else if (strncmp(s, "Genu" "ntel" "ineI", 12) == 0) {
> + if (pread(d, rp, sizeof (*rp), 0x00000001) == sizeof (*rp)) {
> + /* Read VMXE feature bit (bit 5 of ECX feature ID) */
> + if ((rp->r_ecx >> 5) & 1) {
> + strcpy(hvm_type, "vmx");
> + }
> + if ((rp->r_edx >> 6) & 1) {
> + *host_pae = 1;
> + }
> + }
> + }
> +cpuinfo_pread_fail:
> + (void) close(d);
> +cpuinfo_open_fail:
> +
> + d = open(XEN_HYPERVISOR_SOCKET, O_RDWR);
> + hc.op = __HYPERVISOR_xen_version;
> + hc.arg[0] = (unsigned long) XENVER_capabilities;
> + hc.arg[1] = (unsigned long) line;
> + cmd = IOCTL_PRIVCMD_HYPERCALL;
> + (void) ioctl(d, cmd, (unsigned long) &hc);
> + close(d);
I'm surprized to see no error handling there. There is a lot of reasons
why that code could fail I think.
> +}
> +
> +#endif
> +
> /**
> * xenHypervisorGetCapabilities:
> * @conn: pointer to the connection block
> @@ -2278,7 +2368,8 @@ xenHypervisorMakeCapabilitiesXML(virConn
> const char *hostmachine,
> FILE *cpuinfo, FILE *capabilities)
> {
> - char line[1024], *str, *token;
> + const int LINE_SIZE = 1024;
> + char line[LINE_SIZE], *str, *token;
> regmatch_t subs[4];
> char *saveptr = NULL;
> int i;
> @@ -2295,24 +2386,12 @@ xenHypervisorMakeCapabilitiesXML(virConn
>
> memset(guest_archs, 0, sizeof(guest_archs));
>
> - /* /proc/cpuinfo: flags: Intel calls HVM "vmx", AMD calls it "svm".
> - * It's not clear if this will work on IA64, let alone other
> - * architectures and non-Linux. (XXX)
> - */
> - if (cpuinfo) {
> - while (fgets (line, sizeof line, cpuinfo)) {
> - if (regexec (&flags_hvm_rec, line, sizeof(subs)/sizeof(regmatch_t), subs, 0) == 0
> - && subs[0].rm_so != -1) {
> - strncpy (hvm_type,
> - &line[subs[1].rm_so], subs[1].rm_eo-subs[1].rm_so+1);
> - hvm_type[subs[1].rm_eo-subs[1].rm_so] = '\0';
> - } else if (regexec (&flags_pae_rec, line, 0, NULL, 0) == 0)
> - host_pae = 1;
> - }
> - }
> + memset(line, 0, sizeof(line));
> + loadCapabilities(cpuinfo, capabilities, hvm_type, &host_pae, line,
> + LINE_SIZE);
>
> - /* Most of the useful info is in /sys/hypervisor/properties/capabilities
> - * which is documented in the code in xen-unstable.hg/xen/arch/.../setup.c.
> + /* The capabilities line is documented in the code in
> + * xen-unstable.hg/xen/arch/.../setup.c.
> *
> * It is a space-separated list of supported guest architectures.
> *
> @@ -2335,80 +2414,77 @@ xenHypervisorMakeCapabilitiesXML(virConn
> * +--------------- "xen" or "hvm" for para or full virt respectively
> */
>
> - /* Expecting one line in this file - ignore any more. */
> - if ((capabilities) && (fgets (line, sizeof line, capabilities))) {
> - /* Split the line into tokens. strtok_r is OK here because we "own"
> - * this buffer. Parse out the features from each token.
> - */
> - for (str = line, nr_guest_archs = 0;
> - nr_guest_archs < sizeof guest_archs / sizeof guest_archs[0]
> - && (token = strtok_r (str, " ", &saveptr)) != NULL;
> - str = NULL) {
> -
> - if (regexec (&xen_cap_rec, token, sizeof subs / sizeof subs[0],
> - subs, 0) == 0) {
> - int hvm = STRPREFIX(&token[subs[1].rm_so], "hvm");
> - const char *model;
> - int bits, pae = 0, nonpae = 0, ia64_be = 0;
> -
> - if (STRPREFIX(&token[subs[2].rm_so], "x86_32")) {
> - model = "i686";
> - bits = 32;
> - if (subs[3].rm_so != -1 &&
> - STRPREFIX(&token[subs[3].rm_so], "p"))
> - pae = 1;
> - else
> - nonpae = 1;
> - }
> - else if (STRPREFIX(&token[subs[2].rm_so], "x86_64")) {
> - model = "x86_64";
> - bits = 64;
> - }
> - else if (STRPREFIX(&token[subs[2].rm_so], "ia64")) {
> - model = "ia64";
> - bits = 64;
> - if (subs[3].rm_so != -1 &&
> - STRPREFIX(&token[subs[3].rm_so], "be"))
> - ia64_be = 1;
> - }
> - else if (STRPREFIX(&token[subs[2].rm_so], "powerpc64")) {
> - model = "ppc64";
> - bits = 64;
> - } else {
> - /* XXX surely no other Xen archs exist */
> - continue;
> - }
> + /* Split the line into tokens. strtok_r is OK here because we "own"
> + * this buffer. Parse out the features from each token.
> + */
> + for (str = line, nr_guest_archs = 0;
> + nr_guest_archs < sizeof guest_archs / sizeof guest_archs[0]
> + && (token = strtok_r (str, " ", &saveptr)) != NULL;
> + str = NULL) {
> +
> + if (regexec (&xen_cap_rec, token, sizeof subs / sizeof subs[0],
> + subs, 0) == 0) {
> + int hvm = STRPREFIX(&token[subs[1].rm_so], "hvm");
> + const char *model;
> + int bits, pae = 0, nonpae = 0, ia64_be = 0;
> +
> + if (STRPREFIX(&token[subs[2].rm_so], "x86_32")) {
> + model = "i686";
> + bits = 32;
> + if (subs[3].rm_so != -1 &&
> + STRPREFIX(&token[subs[3].rm_so], "p"))
> + pae = 1;
> + else
> + nonpae = 1;
> + }
> + else if (STRPREFIX(&token[subs[2].rm_so], "x86_64")) {
> + model = "x86_64";
> + bits = 64;
> + }
> + else if (STRPREFIX(&token[subs[2].rm_so], "ia64")) {
> + model = "ia64";
> + bits = 64;
> + if (subs[3].rm_so != -1 &&
> + STRPREFIX(&token[subs[3].rm_so], "be"))
> + ia64_be = 1;
> + }
> + else if (STRPREFIX(&token[subs[2].rm_so], "powerpc64")) {
> + model = "ppc64";
> + bits = 64;
> + } else {
> + /* XXX surely no other Xen archs exist */
> + continue;
> + }
>
> - /* Search for existing matching (model,hvm) tuple */
> - for (i = 0 ; i < nr_guest_archs ; i++) {
> - if (STREQ(guest_archs[i].model, model) &&
> - guest_archs[i].hvm == hvm) {
> - break;
> - }
> + /* Search for existing matching (model,hvm) tuple */
> + for (i = 0 ; i < nr_guest_archs ; i++) {
> + if (STREQ(guest_archs[i].model, model) &&
> + guest_archs[i].hvm == hvm) {
> + break;
> }
> -
> - /* Too many arch flavours - highly unlikely ! */
> - if (i >= sizeof(guest_archs)/sizeof(guest_archs[0]))
> - continue;
> - /* Didn't find a match, so create a new one */
> - if (i == nr_guest_archs)
> - nr_guest_archs++;
> -
> - guest_archs[i].model = model;
> - guest_archs[i].bits = bits;
> - guest_archs[i].hvm = hvm;
> -
> - /* Careful not to overwrite a previous positive
> - setting with a negative one here - some archs
> - can do both pae & non-pae, but Xen reports
> - separately capabilities so we're merging archs */
> - if (pae)
> - guest_archs[i].pae = pae;
> - if (nonpae)
> - guest_archs[i].nonpae = nonpae;
> - if (ia64_be)
> - guest_archs[i].ia64_be = ia64_be;
> }
> +
> + /* Too many arch flavours - highly unlikely ! */
> + if (i >= sizeof(guest_archs)/sizeof(guest_archs[0]))
> + continue;
> + /* Didn't find a match, so create a new one */
> + if (i == nr_guest_archs)
> + nr_guest_archs++;
> +
> + guest_archs[i].model = model;
> + guest_archs[i].bits = bits;
> + guest_archs[i].hvm = hvm;
> +
> + /* Careful not to overwrite a previous positive
> + setting with a negative one here - some archs
> + can do both pae & non-pae, but Xen reports
> + separately capabilities so we're merging archs */
> + if (pae)
> + guest_archs[i].pae = pae;
> + if (nonpae)
> + guest_archs[i].nonpae = nonpae;
> + if (ia64_be)
> + guest_archs[i].ia64_be = ia64_be;
> }
> }
>
> @@ -2447,29 +2523,33 @@ xenHypervisorGetCapabilities (virConnect
> /* Really, this never fails - look at the man-page. */
> uname (&utsname);
>
> - cpuinfo = fopen ("/proc/cpuinfo", "r");
> +#ifdef __linux__
> + cpuinfo = fopen (CPUINFO, "r");
> if (cpuinfo == NULL) {
> if (errno != ENOENT) {
> - virXenPerror (conn, "/proc/cpuinfo");
> + virXenPerror (conn, CPUINFO);
> return NULL;
> }
> }
>
> - capabilities = fopen ("/sys/hypervisor/properties/capabilities", "r");
> + capabilities = fopen (HYPERVISOR_CAPABILITIES, "r");
> if (capabilities == NULL) {
> if (errno != ENOENT) {
> fclose(cpuinfo);
> - virXenPerror (conn, "/sys/hypervisor/properties/capabilities");
> + virXenPerror (conn, HYPERVISOR_CAPABILITIES);
> return NULL;
> }
> }
> +#endif
okay reusing the same function but passing NULL args if not linux is
fine, but if we keep the same signature, why not merge the content to have
only one entry point:
void
loadCapabilities(FILE *cpuinfo, FILE *capabilities, char *hvm_type,
int *host_pae, char *line, int LINE_SIZE) {
#ifdef __linux__
...
#elif __sun
...
#else
...
#endif
}
then you have only one referenced entry point easier when digging code
for example.
> xml = xenHypervisorMakeCapabilitiesXML(conn, utsname.machine, cpuinfo, capabilities);
>
> +#ifdef __linux__
> if (cpuinfo)
> fclose(cpuinfo);
> if (capabilities)
> fclose(capabilities);
> +#endif
>
> return xml;
> }
A few things to check first before commiting this IMHO,
thanks,
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard@xxxxxxxxxx | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list