[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 3/3] libxc/PV: save/restore data breakpoint extension registers
>>> On 11.04.14 at 18:37, <dunlapg@xxxxxxxxx> wrote: > On Mon, Apr 7, 2014 at 10:39 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote: >> @@ -634,11 +640,42 @@ static int buffer_tail_pv(xc_interface * >> ERROR("Error allocating VCPU ctxt tail buffer"); >> goto free_pfntab; >> } >> + alloc = 1; >> } >> // DPRINTF("Reading VCPUS: %d bytes\n", vcpulen); >> - if ( RDEXACT(fd, buf->vcpubuf, vcpulen) ) { >> - PERROR("Error when reading ctxt"); >> - goto free_vcpus; >> + for ( total = i = 0, ptr.raw = buf->vcpubuf; ext_vcpucontext; ) { > > This isn't actually a for loop. Would it really be so terrible to do > if(ext_vcpucontext) { while(1) {}}? Nothing really terrible, but it would increase indentation by one level for no real reason. >> + if ( RDEXACT(fd, buf->vcpubuf + total, vcpulen) ) { >> + PERROR("Error when reading ctxt"); >> + goto free_vcpus; >> + } >> + total += vcpulen; >> + for ( vcpulen = 0; i < buf->vcpucount; ++i ) { >> + size_t msrlen; >> + >> + if ( (const unsigned char *)(ptr.evc + 1) > buf->vcpubuf + >> total ) >> + break; >> + if ( ptr.evc->size < >> + (offsetof(xen_domctl_ext_vcpucontext_t, msrs) + >> + sizeof(ptr.evc->msrs)) ) >> + ptr.evc->msr_count = 0; >> + msrlen = ptr.evc->msr_count * sizeof(xen_domctl_ext_vcpu_msr_t); >> + vcpulen += msrlen; >> + ptr.raw += 128 + msrlen + vcpuextstate_size; >> + } >> + if ( !vcpulen ) { >> + assert(i == buf->vcpucount); >> + break; >> + } > > If you're going to write such twisted logic, you absolutely must > comment it -- at very least you need to give a high level description > of what it's doing. I've been staring at this for an hour now, and I > only have a vague idea what it's supposed to be doing. Seriously, is > this any better than spaghetti code? Now do you have any better suggestion? I was just trying to extend what is there with as little churn on the code as possible. I'm not even certain the model chose is desirable - I could imagine that introducing another "extended info block", e.g. tagged "msrs", might be preferable. In any event, this isn't code I'm feeling comfortable changing, so of all possible options I'd prefer to have this whole aspect of the intended functionality done by someone else who is. And the patch here then might serve as no more than a baseline reference, to be thrown away entirely if so desired. Perhaps the best thing really is to leave the save/restore aspect out for now, and get it implemented once Andrew's rewrite is in place. >> @@ -1960,16 +1963,42 @@ int xc_domain_save(xc_interface *xch, in >> domctl.domain = dom; >> memset(&domctl.u, 0, sizeof(domctl.u)); >> domctl.u.ext_vcpucontext.vcpu = i; >> - if ( xc_domctl(xch, &domctl) < 0 ) >> + frc = xc_domctl(xch, &domctl); >> + if ( frc < 0 && errno == ENOBUFS && >> + domctl.u.ext_vcpucontext.size >= >> + (offsetof(xen_domctl_ext_vcpucontext_t, msrs) + >> + sizeof(domctl.u.ext_vcpucontext.msrs)) && >> + domctl.u.ext_vcpucontext.msr_count ) >> + { >> + msrs = xc_hypercall_buffer_alloc(xch, msrs, >> + >> domctl.u.ext_vcpucontext.msr_count * >> + sizeof(*msrs)); >> + set_xen_guest_handle(domctl.u.ext_vcpucontext.msrs, msrs); >> + frc = msrs ? xc_domctl(xch, &domctl) : -1; >> + /* Don't save the actual pointer. */ >> + set_xen_guest_handle_raw(domctl.u.ext_vcpucontext.msrs, NULL); >> + } > > I think this could use a comment as well. Not sure what you refer to here - the one non-obvious thing (clearing out the handle) is being commented. Are you trying to tell me that this isn't sufficient of an explanation, or are you referring to other parts of this code block? Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |