| Message ID | 20231110204207.2927514-5-volodymyr_babchuk@epam.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/7] xen-block: Do not write frontend nodes | expand |
On Fri, 2023-11-10 at 20:42 +0000, Volodymyr Babchuk wrote: > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > Instead of forcing the owner to domid 0, use XS_PRESERVE_OWNER to save > the previous owner of the directory. > You're missing the words "... if it already exists" from that sentence. If the directory *didn't* already exist, it gets created with dom0 as the owner still, right? Assuming XenStore allows QEMU to do that. Strictly, the node gets created (if permitted) and *then* xs_set_permissions() attempts to set dom0 as the owner (if permitted). > Note that for other than Dom0 domain (non toolstack domain) the > "driver_domain" property should be set in domain config file for the > toolstack to create required directories in advance. > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> > --- > hw/xen/xen_pvdev.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/hw/xen/xen_pvdev.c b/hw/xen/xen_pvdev.c > index c5ad71e8dc..42bdd4f6c8 100644 > --- a/hw/xen/xen_pvdev.c > +++ b/hw/xen/xen_pvdev.c > @@ -60,7 +60,8 @@ void xen_config_cleanup(void) > > int xenstore_mkdir(char *path, int p) > { > - if (!qemu_xen_xs_create(xenstore, 0, 0, xen_domid, p, path)) { > + if (!qemu_xen_xs_create(xenstore, 0, XS_PRESERVE_OWNER, > + xen_domid, p, path)) { > xen_pv_printf(NULL, 0, "xs_mkdir %s: failed\n", path); > return -1; > } Why bother with xenstore_mkdir()? AFAICT it's *only* used from the legacy XenLegacyDevice stuff, and can't we just finish the job of moving from that to the XenDevice model? I've done console and net recently; want to keep going? And even then... the xenstore_mkdir() function is called twice from xen_config_dev_dirs() in hw/xen/xen_devconfig.c to create the frontend and backend directories — which is what the rest of your patch series is trying to eliminate because a driver domain doesn't have permissions to do that anyway. It's also called from xen_be_register() in hw/xen/xen_devconfig.c to create device-model/${GUEST_DOMID}/backends/${DEVICE_TYPE} (using a relative path, so in the driver domain's XenStore). That one presumably *won't* exist already, and so XS_PRESERVE_OWNER won't even have any effect? What practical difference does this even make? Am I missing something?
Hi David, David Woodhouse <dwmw2@infradead.org> writes: > [[S/MIME Signed Part:Undecided]] > On Fri, 2023-11-10 at 20:42 +0000, Volodymyr Babchuk wrote: >> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> >> Instead of forcing the owner to domid 0, use XS_PRESERVE_OWNER to save >> the previous owner of the directory. >> > > You're missing the words "... if it already exists" from that sentence. > > If the directory *didn't* already exist, it gets created with dom0 as > the owner still, right? Assuming XenStore allows QEMU to do that. If it didn't already exist, it is created and it inherits access rights from the parent node. > Strictly, the node gets created (if permitted) and *then* > xs_set_permissions() attempts to set dom0 as the owner (if permitted). Yes. I'll rephrase this as "Instead of forcing the owner to domid 0, use XS_PRESERVE_OWNER to save the inherited owner of the directory." will it be okay? > >> Note that for other than Dom0 domain (non toolstack domain) the >> "driver_domain" property should be set in domain config file for the >> toolstack to create required directories in advance. >> >> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> >> --- >> hw/xen/xen_pvdev.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/hw/xen/xen_pvdev.c b/hw/xen/xen_pvdev.c >> index c5ad71e8dc..42bdd4f6c8 100644 >> --- a/hw/xen/xen_pvdev.c >> +++ b/hw/xen/xen_pvdev.c >> @@ -60,7 +60,8 @@ void xen_config_cleanup(void) >> >> int xenstore_mkdir(char *path, int p) >> { >> - if (!qemu_xen_xs_create(xenstore, 0, 0, xen_domid, p, path)) { >> + if (!qemu_xen_xs_create(xenstore, 0, XS_PRESERVE_OWNER, >> + xen_domid, p, path)) { >> xen_pv_printf(NULL, 0, "xs_mkdir %s: failed\n", path); >> return -1; >> } > > Why bother with xenstore_mkdir()? AFAICT it's *only* used from the > legacy XenLegacyDevice stuff, and can't we just finish the job of > moving from that to the XenDevice model? I've done console and net > recently; want to keep going? Well, I am not so familiar with QEMU to accomplish this in a short time. If you really need help, I can take alook at 9p driver, it looks simplest of them all... > > And even then... the xenstore_mkdir() function is called twice from > xen_config_dev_dirs() in hw/xen/xen_devconfig.c to create the frontend > and backend directories — which is what the rest of your patch series > is trying to eliminate because a driver domain doesn't have permissions > to do that anyway. > > It's also called from xen_be_register() in hw/xen/xen_devconfig.c to > create device-model/${GUEST_DOMID}/backends/${DEVICE_TYPE} (using a > relative path, so in the driver domain's XenStore). That one presumably > *won't* exist already, and so XS_PRESERVE_OWNER won't even have any > effect? As I said, it will inherit driver's domain access rights. So yeah, Oleksandr's patch covers this case, mostly. I agree, it would be better to drop xenstore_mkdir() at all, but this is tangent to my task of adding virtio-pci support for ARM guests. Those Oleksandr's patches for drive domain, that you are seeing, come to life only because our system happens to use a separate driver domain.
diff --git a/hw/xen/xen_pvdev.c b/hw/xen/xen_pvdev.c index c5ad71e8dc..42bdd4f6c8 100644 --- a/hw/xen/xen_pvdev.c +++ b/hw/xen/xen_pvdev.c @@ -60,7 +60,8 @@ void xen_config_cleanup(void) int xenstore_mkdir(char *path, int p) { - if (!qemu_xen_xs_create(xenstore, 0, 0, xen_domid, p, path)) { + if (!qemu_xen_xs_create(xenstore, 0, XS_PRESERVE_OWNER, + xen_domid, p, path)) { xen_pv_printf(NULL, 0, "xs_mkdir %s: failed\n", path); return -1; }