|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v3 17/25] tools/xenstore: rework struct xs_tdb_record_hdr
On 28.07.23 16:08, Julien Grall wrote: Hi Juergen, On 28/07/2023 14:24, Juergen Gross wrote:On 28.07.23 14:48, Julien Grall wrote:Hi, On 28/07/2023 13:06, Juergen Gross wrote:On 28.07.23 13:19, Julien Grall wrote:In case of a runtime check I agree that a more central place would be preferred. In the end I don't mind that much, but BUILD_BUG_ON(XENSTORE_PAYLOAD_MAX >= (typeof((struct node_hdr *)NULL->datalen))(-1)); is a little bit clumsy IMHO.Agree. We could introduce FIELD_SIZEOF() (as Linux did) to hide the complexity. The code would then look like:>= (8 * FIELD_SIZEOF(struct node_hdr, datalen))Oh, I guess you mean sizeof_field(). And even with that it would look quite clumsy: BUILD_BUG_ON(XENSTORE_PAYLOAD_MAX >= (1UL << (8 * sizeof_field(struct node_hdr, datalen))));How about keeping the BUILD_BUG_ON() in write_node_raw() and add the following comment on top of handle_input():Some fields in Xenstored are sized based on the max payload (see various BUILD_BUG_ON()). This would need extra runtime check if we ever decide to have a dynamic payload size.I _could_ do that, but where to stop adding such comments?When someone other than the author is able to understand the code without too much effort. More comments never hurts, less will in the longer run (see below).I agree with that statement in general, but requesting a comment to aid a future potential change violating the Xenstore wire protocol is a little bit weird.Well... This is violating the existing protocol, but it is not set in stone and I think this is acceptable to update it when there is no change for the VMs and for new features (e.g. Live-Update/Live-Migration). No, I don't think so. Think of Xenstore living in a stubdom. This means that even dom0 requests have to go via a ring page, and you are just assuming that the kernel side driver forwarding the requests from user mode is fine with XENSTORE_PAYLOAD_MAX _not_ being the absolute maximum of data transferred via a single command. I could perfectly understand that the kernel might have a XENSTORE_PAYLOAD_MAX sized buffer for the payload, which would be not large enough for your use case. So violating XENSTORE_PAYLOAD_MAX might be a bad idea with some implementations, at least in theory. This means, BTW, that changing XENSTORE_PAYLOAD_MAX is a bad idea, too, as this would require to sync between all components knowing its value. That's assuming you will still be around when this happens :). I am not wishing anything bad but the code will likely outlast any of us.TBH, I really don't see the point doing that. In case a patch came up upstream trying to violate XENSTORE_PAYLOAD_MAX I would surely NACK it.Maybe. But would you really Ack patches adding comments like that in other areas?Potentially yes. We had a similar discussion on Arm when allowing paddr_t to be 32-bit.[...]In case we need payloads larger than XENSTORE_PAYLOAD_MAX we should split the related operation in multiple parts (see e.g. XS_DIRECTORY_PART or XS_CONTROL for uploading a new kernel to Xenstore-stubdom for live update). Which is, BTW, the way AWS should have handled the migration problem (transactions come to my mind in this context).I wasn't part of the original design, but I can see why it was done like that.I can see why it was done that way, but this doesn't mean I can understand why such a design should be supported by adding comments helping to repeat such a bad decision.Using multiple commands has also its downside. The first that comes to my mind if that you need to keep around the data. But, with your proposal, you we wouldn't be able to store it in the database (like for transaction update) as datalen can only be 65KB.I wasn't aware that a complete transaction needs to be kept in a single data base record. :-)IIUC, you are thinking that the client will restore all the state bits by bits. But if you look at the design in docs/designs/xenstore-migration.md, this is a blob. Of course it is. I was never assuming that the state would be applied piecemeal, this has to happen atomically. It would work perfectly fine to allocate the needed memory via talloc() and to reference it from a special node being part of the transaction, or to not use a node at all (see again the XS_CONTROL example).I am not convinced the complexity is worth it here. To be honest, I think the payload limit should have been relaxed for Live-Update as well as you don't gain much to split. That said, this is less a concern because you are not time constrained.[...]But maybe that comment was based on wrong assumptions, like the mentioned change not violating the protocol. >I am happy to rewrite the comment so it doesn't lead to think that you (as the maintainer) are open to have a more relax length check.Yes, please make a suggestion for a proper comment not suggesting we are fine to violate the wire protocol.Here we go:"The payload size is not only currently restricted by the protocol but also the internal implementation (see various BUILD_BUG_ON())." Hmm, I'm still feeling uneasy to imply that the payload size might be changed. See above reasoning. The only way I could imagine this being possible would be a per-ring-page attribute with both sides agreeing to the max allowed size (the minimum being today's value). With that in mind I can hesitantly add the comment, maybe with the addition: "Any potential change of the maximum payload size needs to be negotiated between the involved parties." Juergen Attachment:
OpenPGP_0xB0DE9DD628BF132F.asc Attachment:
OpenPGP_signature
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |