Re: [PULL 13/27] hw/xen: Add xenstore operations to allow redirection to internal emulation
On Tue, 2023-04-04 at 18:45 +0100, Peter Maydell wrote: > On Tue, 4 Apr 2023 at 18:45, David Woodhouse > wrote: > > > > On Tue, 2023-04-04 at 18:35 +0100, Peter Maydell wrote: > > > On Tue, 7 Mar 2023 at 18:27, David Woodhouse > > > > > > wrote: > > > > > > > > From: Paul Durrant > > > > > > > > Signed-off-by: Paul Durrant > > > > Signed-off-by: David Woodhouse > > > > Reviewed-by: Paul Durrant > > > > --- > > > > > > Hi; Coverity points out a memory leak in this code (CID 1508098): > > > > > > > +static struct qemu_xs_handle *libxenstore_open(void) > > > > +{ > > > > + struct xs_handle *xsh = xs_open(0); > > > > + struct qemu_xs_handle *h = g_new0(struct qemu_xs_handle, > > > > 1); > > > > > > Here we allocate memory... > > > > > > > + > > > > + if (!xsh) { > > > > + return NULL; > > > > > > ...but here we can return without freeing it... > > > > > > > + } > > > > + > > > > + h = g_new0(struct qemu_xs_handle, 1); > > > > > > ...and here we allocate a second time and overwrite the > > > pointer to the first allocation. > > > > > > Deleting the first call to g_new0() would fix both of these. > > > > Indeed, thanks. Do you want a > > > > Reviewed-by: David Woodhouse > > > > or would you prefer me to submit the actual patch as described? > > If you could submit the patch that would be easiest -- you're in > a better position to test it. I've been getting Paul to test the parts on actual Xen, so I'll send it and let him test. smime.p7s Description: S/MIME cryptographic signature
Re: [PULL 13/27] hw/xen: Add xenstore operations to allow redirection to internal emulation
On Tue, 4 Apr 2023 at 18:45, David Woodhouse wrote: > > On Tue, 2023-04-04 at 18:35 +0100, Peter Maydell wrote: > > On Tue, 7 Mar 2023 at 18:27, David Woodhouse > > wrote: > > > > > > From: Paul Durrant > > > > > > Signed-off-by: Paul Durrant > > > Signed-off-by: David Woodhouse > > > Reviewed-by: Paul Durrant > > > --- > > > > Hi; Coverity points out a memory leak in this code (CID 1508098): > > > > > +static struct qemu_xs_handle *libxenstore_open(void) > > > +{ > > > +struct xs_handle *xsh = xs_open(0); > > > +struct qemu_xs_handle *h = g_new0(struct qemu_xs_handle, 1); > > > > Here we allocate memory... > > > > > + > > > +if (!xsh) { > > > +return NULL; > > > > ...but here we can return without freeing it... > > > > > +} > > > + > > > +h = g_new0(struct qemu_xs_handle, 1); > > > > ...and here we allocate a second time and overwrite the > > pointer to the first allocation. > > > > Deleting the first call to g_new0() would fix both of these. > > Indeed, thanks. Do you want a > > Reviewed-by: David Woodhouse > > or would you prefer me to submit the actual patch as described? If you could submit the patch that would be easiest -- you're in a better position to test it. thanks -- PMM
Re: [PULL 13/27] hw/xen: Add xenstore operations to allow redirection to internal emulation
On Tue, 2023-04-04 at 18:35 +0100, Peter Maydell wrote: > On Tue, 7 Mar 2023 at 18:27, David Woodhouse > wrote: > > > > From: Paul Durrant > > > > Signed-off-by: Paul Durrant > > Signed-off-by: David Woodhouse > > Reviewed-by: Paul Durrant > > --- > > Hi; Coverity points out a memory leak in this code (CID 1508098): > > > +static struct qemu_xs_handle *libxenstore_open(void) > > +{ > > + struct xs_handle *xsh = xs_open(0); > > + struct qemu_xs_handle *h = g_new0(struct qemu_xs_handle, 1); > > Here we allocate memory... > > > + > > + if (!xsh) { > > + return NULL; > > ...but here we can return without freeing it... > > > + } > > + > > + h = g_new0(struct qemu_xs_handle, 1); > > ...and here we allocate a second time and overwrite the > pointer to the first allocation. > > Deleting the first call to g_new0() would fix both of these. Indeed, thanks. Do you want a Reviewed-by: David Woodhouse or would you prefer me to submit the actual patch as described? smime.p7s Description: S/MIME cryptographic signature
Re: [PULL 13/27] hw/xen: Add xenstore operations to allow redirection to internal emulation
On Tue, 7 Mar 2023 at 18:27, David Woodhouse wrote: > > From: Paul Durrant > > Signed-off-by: Paul Durrant > Signed-off-by: David Woodhouse > Reviewed-by: Paul Durrant > --- Hi; Coverity points out a memory leak in this code (CID 1508098): > +static struct qemu_xs_handle *libxenstore_open(void) > +{ > +struct xs_handle *xsh = xs_open(0); > +struct qemu_xs_handle *h = g_new0(struct qemu_xs_handle, 1); Here we allocate memory... > + > +if (!xsh) { > +return NULL; ...but here we can return without freeing it... > +} > + > +h = g_new0(struct qemu_xs_handle, 1); ...and here we allocate a second time and overwrite the pointer to the first allocation. Deleting the first call to g_new0() would fix both of these. > +h->xsh = xsh; > + > +notifier_list_init(>notifiers); > +qemu_set_fd_handler(xs_fileno(h->xsh), watch_event, NULL, h); > + > +return h; > +} thanks -- PMM
Re: [PULL 13/27] hw/xen: Add xenstore operations to allow redirection to internal emulation
On Mon, 2023-03-13 at 19:17 -0400, Jason Andryuk wrote: > This looks good, better than what I posted, and seems to work for both > dm_restrict set and unset. Thanks. > For dm_restricted, xs_write() does fail. I verified that with a print > statement. I think "shouldn't even try" makes sense. I'm thinking > that xen_domid_restricted shouldn't even add the callback. Something > like: > > --- a/accel/xen/xen-all.c > +++ b/accel/xen/xen-all.c > @@ -39,8 +39,7 @@ static void xenstore_record_dm_state(const char *state) > * This call may fail when running restricted so don't make it fatal in > * that case. Toolstacks should instead use QMP to listen for state > changes. > */ > - if (!qemu_xen_xs_write(xenstore, XBT_NULL, path, state, strlen(state)) && > - !xen_domid_restrict) { > + if (!qemu_xen_xs_write(xenstore, XBT_NULL, path, state, strlen(state))) { > error_report("error recording dm state"); > exit(1); > } > @@ -101,7 +100,10 @@ static int xen_init(MachineState *ms) > xc_interface_close(xen_xc); > return -1; > } > - qemu_add_vm_change_state_handler(xen_change_state_handler, NULL); > + > + if(!xen_domid_restrict) > + qemu_add_vm_change_state_handler(xen_change_state_handler, NULL); > + > /* > * opt out of system RAM being allocated by generic code > */ > > That works for both dm_restrict 0 & 1. > > I think you should submit your change and I can follow up with the > above if it seems desirable. Let's just do it in one. I'll move that comment about 'call may fail' down to where you've made the qemu_add_vm_change_state_handler() conditional. And QEMU style requires braces even for a one-line if(). I'll send it out and let you add your Signed-off-by: and Tested-by: to it. smime.p7s Description: S/MIME cryptographic signature
Re: [PULL 13/27] hw/xen: Add xenstore operations to allow redirection to internal emulation
Hi, David, On Mon, Mar 13, 2023 at 4:45 AM David Woodhouse wrote: > > On Sun, 2023-03-12 at 15:19 -0400, Jason Andryuk wrote: > > > > This breaks dm_restrict=1 since the xs_open is not allowed by the > > time > > this is called. There are other evtchn errors before this as well: > > # cat /var/log/xen/qemu-dm-debian.log > > char device redirected to /dev/pts/8 (label serial0) > > xen be core: can't open evtchn device > > xen be core: can't open evtchn device > > xen be core: can't open evtchn device > > xen be core: can't open evtchn device > > xen be core: can't open evtchn device > > xen be core: can't open evtchn device > > xen be core: can't open evtchn device > > xen be core: can't open evtchn device > > Could not contact XenStore > > > > Ok, those "xen be core: can't open evtchn device" were there before > > the recent changes and seem to be non-fatal. > > Hm, I *think* we can just revert that part and use the global > 'xenstore' like we did before, except via the new ops. > > --- a/accel/xen/xen-all.c > +++ b/accel/xen/xen-all.c > @@ -32,28 +32,18 @@ xendevicemodel_handle *xen_dmod; > > static void xenstore_record_dm_state(const char *state) > { > -struct xs_handle *xs; > char path[50]; > > -/* We now have everything we need to set the xenstore entry. */ > -xs = xs_open(0); > -if (xs == NULL) { > -fprintf(stderr, "Could not contact XenStore\n"); > -exit(1); > -} > - > snprintf(path, sizeof (path), "device-model/%u/state", xen_domid); > /* > * This call may fail when running restricted so don't make it fatal in > * that case. Toolstacks should instead use QMP to listen for state > changes. > */ > -if (!xs_write(xs, XBT_NULL, path, state, strlen(state)) && > +if (!qemu_xen_xs_write(xenstore, XBT_NULL, path, state, strlen(state)) && > !xen_domid_restrict) { > error_report("error recording dm state"); > exit(1); > } > - > -xs_close(xs); > } This looks good, better than what I posted, and seems to work for both dm_restrict set and unset. > > Alternatively, that xs_write is destined to fail anyway in the > xen_domid_restrict case, isn't it? So the xs_open() should be allowed > to fail similarly. Or perhaps we shouldn't even *try*? For dm_restricted, xs_write() does fail. I verified that with a print statement. I think "shouldn't even try" makes sense. I'm thinking that xen_domid_restricted shouldn't even add the callback. Something like: --- a/accel/xen/xen-all.c +++ b/accel/xen/xen-all.c @@ -39,8 +39,7 @@ static void xenstore_record_dm_state(const char *state) * This call may fail when running restricted so don't make it fatal in * that case. Toolstacks should instead use QMP to listen for state changes. */ -if (!qemu_xen_xs_write(xenstore, XBT_NULL, path, state, strlen(state)) && -!xen_domid_restrict) { +if (!qemu_xen_xs_write(xenstore, XBT_NULL, path, state, strlen(state))) { error_report("error recording dm state"); exit(1); } @@ -101,7 +100,10 @@ static int xen_init(MachineState *ms) xc_interface_close(xen_xc); return -1; } -qemu_add_vm_change_state_handler(xen_change_state_handler, NULL); + +if(!xen_domid_restrict) +qemu_add_vm_change_state_handler(xen_change_state_handler, NULL); + /* * opt out of system RAM being allocated by generic code */ That works for both dm_restrict 0 & 1. I think you should submit your change and I can follow up with the above if it seems desirable. Thanks, Jason
Re: [PULL 13/27] hw/xen: Add xenstore operations to allow redirection to internal emulation
On Sun, 2023-03-12 at 15:19 -0400, Jason Andryuk wrote: > > This breaks dm_restrict=1 since the xs_open is not allowed by the > time > this is called. There are other evtchn errors before this as well: > # cat /var/log/xen/qemu-dm-debian.log > char device redirected to /dev/pts/8 (label serial0) > xen be core: can't open evtchn device > xen be core: can't open evtchn device > xen be core: can't open evtchn device > xen be core: can't open evtchn device > xen be core: can't open evtchn device > xen be core: can't open evtchn device > xen be core: can't open evtchn device > xen be core: can't open evtchn device > Could not contact XenStore > > Ok, those "xen be core: can't open evtchn device" were there before > the recent changes and seem to be non-fatal. Hm, I *think* we can just revert that part and use the global 'xenstore' like we did before, except via the new ops. --- a/accel/xen/xen-all.c +++ b/accel/xen/xen-all.c @@ -32,28 +32,18 @@ xendevicemodel_handle *xen_dmod; static void xenstore_record_dm_state(const char *state) { -struct xs_handle *xs; char path[50]; -/* We now have everything we need to set the xenstore entry. */ -xs = xs_open(0); -if (xs == NULL) { -fprintf(stderr, "Could not contact XenStore\n"); -exit(1); -} - snprintf(path, sizeof (path), "device-model/%u/state", xen_domid); /* * This call may fail when running restricted so don't make it fatal in * that case. Toolstacks should instead use QMP to listen for state changes. */ -if (!xs_write(xs, XBT_NULL, path, state, strlen(state)) && +if (!qemu_xen_xs_write(xenstore, XBT_NULL, path, state, strlen(state)) && !xen_domid_restrict) { error_report("error recording dm state"); exit(1); } - -xs_close(xs); } Alternatively, that xs_write is destined to fail anyway in the xen_domid_restrict case, isn't it? So the xs_open() should be allowed to fail similarly. Or perhaps we shouldn't even *try*? smime.p7s Description: S/MIME cryptographic signature
Re: [PULL 13/27] hw/xen: Add xenstore operations to allow redirection to internal emulation
On Tue, Mar 7, 2023 at 1:29 PM David Woodhouse wrote: > > From: Paul Durrant > > Signed-off-by: Paul Durrant > Signed-off-by: David Woodhouse > Reviewed-by: Paul Durrant > --- > accel/xen/xen-all.c | 11 +- > hw/char/xen_console.c | 2 +- > hw/i386/kvm/xen_xenstore.c | 3 - > hw/i386/kvm/xenstore_impl.h | 8 +- > hw/xen/xen-bus-helper.c | 62 +++ > hw/xen/xen-bus.c| 261 > hw/xen/xen-legacy-backend.c | 119 +++-- > hw/xen/xen-operations.c | 198 + > hw/xen/xen_devconfig.c | 4 +- > hw/xen/xen_pt_graphics.c| 1 - > hw/xen/xen_pvdev.c | 49 +- > include/hw/xen/xen-bus-helper.h | 26 +-- > include/hw/xen/xen-bus.h| 17 +- > include/hw/xen/xen-legacy-backend.h | 6 +- > include/hw/xen/xen_backend_ops.h| 163 + > include/hw/xen/xen_common.h | 1 - > include/hw/xen/xen_pvdev.h | 2 +- > softmmu/globals.c | 1 + > 18 files changed, 525 insertions(+), 409 deletions(-) > > diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c > index e85e4aeba5..425216230f 100644 > --- a/accel/xen/xen-all.c > +++ b/accel/xen/xen-all.c > @@ -90,12 +90,15 @@ void xenstore_store_pv_console_info(int i, Chardev *chr) > } > > > -static void xenstore_record_dm_state(struct xs_handle *xs, const char *state) > +static void xenstore_record_dm_state(const char *state) > { > +struct xs_handle *xs; > char path[50]; > > +/* We now have everything we need to set the xenstore entry. */ > +xs = xs_open(0); > if (xs == NULL) { > -error_report("xenstore connection not initialized"); > +fprintf(stderr, "Could not contact XenStore\n"); > exit(1); > } This breaks dm_restrict=1 since the xs_open is not allowed by the time this is called. There are other evtchn errors before this as well: # cat /var/log/xen/qemu-dm-debian.log char device redirected to /dev/pts/8 (label serial0) xen be core: can't open evtchn device xen be core: can't open evtchn device xen be core: can't open evtchn device xen be core: can't open evtchn device xen be core: can't open evtchn device xen be core: can't open evtchn device xen be core: can't open evtchn device xen be core: can't open evtchn device Could not contact XenStore Ok, those "xen be core: can't open evtchn device" were there before the recent changes and seem to be non-fatal. Regards, Jason