On Fri, Jan 30, 2015 at 09:44:56AM -0500, Boris Ostrovsky wrote: > On 01/29/2015 08:14 PM, Luis R. Rodriguez wrote: >> From: "Luis R. Rodriguez" <mcg...@suse.com> >> +/* >> + * Do not pause already paused domains, and allow us to >> + * unpause only quiesced domains. >> + */ >> +static int quiesce_all_domains(xc_interface *xch, >> + struct xc_quiesce_request *quiesce_request) >> +{ >> + xc_domaininfo_t info[1024]; >> + int i, nb_domain; >> + >> + nb_domain = xc_domain_getinfolist(xch, 0, 1024, info); >> + if ( nb_domain < 0 ) >> + { >> + return -1; >> + } >> + >> + for ( i = 0; i < nb_domain; i++ ) >> + { >> + if ( info[i].domain == 0 ) >> + continue; >> + if ( info[i].flags & XEN_DOMINF_paused ) >> + continue; >> + >> + xc_domain_pause(xch, info[i].domain); > > > You are not handling errors returned by xc_domain_pause().
Thanks for the review, shall we just bail if that cannot happen? > So then you will > try to unpause a domain that may not have been paused and (I think more > importantly) may proceed with microcode update while not all domains are > paused. Yeah this would be bad. Perhaps bail and tell the user the domain that we could not pause / quiesce (depending on what we decide to do). >> + >> + quiesce_request->domids[quiesce_request->num_quiesced] = info[i].domain; >> + quiesce_request->num_quiesced++; >> + } >> + >> + return 0; >> +} >> + >> +static void unquiesce_all_domains(xc_interface *xch, >> + struct xc_quiesce_request >> *quiesce_request) >> +{ >> + int i; >> + >> + for ( i = 0; i < quiesce_request->num_quiesced; i++ ) >> + { >> + xc_domain_unpause(xch, quiesce_request->domids[i]); > > > Same here --- you may fail unpausing a domain and noone will know about it. True, that seems like a rather informational thing we can spit out, do we want to return an error for that though too? >> + } >> +} >> + >> +int xc_microcode_update(xc_interface *xch, uint8_t *fbuf, size_t len) >> +{ >> + int ret = 0; >> + DECLARE_HYPERCALL_BUFFER(struct xenpf_microcode_update, uc); >> + DECLARE_HYPERCALL; >> + struct xen_platform_op op_s; >> + struct xen_platform_op *op = &op_s; >> + DECLARE_HYPERCALL_BOUNCE(op, sizeof(*op), >> XC_HYPERCALL_BUFFER_BOUNCE_BOTH); >> + struct xc_quiesce_request quiesce_request; >> + >> + memset(&quiesce_request, 0, sizeof(struct xc_quiesce_request)); >> + >> + if ( !xch ) >> + { >> + return -1; >> + } > > > Not sure what tools coding convention is but you may not need {} here (and > a few more places) I was not sure so went with that. I'm happy to remove that stuff from one liners. >> + /* microcode file as present on /lib/firmware/ */ > > On Linux but not necessarily on other OSs. You code doesn't require it to > be there so you probably want to avoid referring to this path in comments > and commit subject/body. Amended. >> + printf("%s: %ld\n", filename, buf.st_size); > > > Do you need this? (probably leftover from debugging?) Killed. Luis _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel