Re: [PATCH v3 12/16] slirp: unregister the win32 SOCKET
Hi
On Thu, Mar 2, 2023 at 10:45 PM Stefan Berger wrote:
>
>
> On 2/21/23 07:47, marcandre.lur...@redhat.com wrote:
> > From: Marc-André Lureau
> >
> > Presumably, this is what should happen when the SOCKET is to be removed.
> > (it probably worked until now because closesocket() does it implicitly,
> > but we never now how the slirp library could use the SOCKET later)
> >
> > Signed-off-by: Marc-André Lureau
> > ---
> > net/slirp.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/slirp.c b/net/slirp.c
> > index 0730a935ba..a7c35778a6 100644
> > --- a/net/slirp.c
> > +++ b/net/slirp.c
> > @@ -259,7 +259,9 @@ static void net_slirp_register_poll_fd(int fd, void
> *opaque)
> >
> > static void net_slirp_unregister_poll_fd(int fd, void *opaque)
> > {
> > -/* no qemu_fd_unregister */
> > +#ifdef WIN32
> The majority of code seems to use _WIN32. Not sure what is 'right'.
>
>
Both should be correct. I think I like the "WIN32" version better though
(see also
https://stackoverflow.com/questions/662084/whats-the-difference-between-the-win32-and-win32-defines-in-c
)
> Reviewed-by: Stefan Berger
>
>
thanks
> > +qemu_socket_unselect(fd, NULL);
> > +#endif
> > }
> >
> > static void net_slirp_notify(void *opaque)
>
>
Re: [PATCH v3 14/16] win32: avoid mixing SOCKET and file descriptor space
Hi
On Fri, Mar 3, 2023 at 12:54 AM Stefan Berger wrote:
>
>
>
> On 2/21/23 07:47, marcandre.lur...@redhat.com wrote:
> > From: Marc-André Lureau
> >
> > Until now, a win32 SOCKET handle is often cast to an int file
> > descriptor, as this is what other OS use for sockets. When necessary,
> > QEMU eventually queries whether it's a socket with the help of
> > fd_is_socket(). However, there is no guarantee of conflict between the
> > fd and SOCKET space. Such conflict would have surprising consequences,
> > we shouldn't mix them.
> >
> > Also, it is often forgotten that SOCKET must be closed with
> > closesocket(), and not close().
> >
> > Instead, let's make the win32 socket wrapper functions return and take a
> > file descriptor, and let util/ wrappers do the fd/SOCKET conversion as
> > necessary. A bit of adaptation is necessary in io/ as well.
> >
> > Unfortunately, we can't drop closesocket() usage, despite
> > _open_osfhandle() documentation claiming transfer of ownership, testing
> > shows bad behaviour if you forget to call closesocket().
> >
> > Signed-off-by: Marc-André Lureau
> > ---
> > include/sysemu/os-win32.h | 4 +-
> > io/channel-watch.c| 6 +-
> > util/aio-win32.c | 9 +-
> > util/oslib-win32.c| 219 --
> > 4 files changed, 197 insertions(+), 41 deletions(-)
>
> > #undef connect
> > @@ -308,7 +315,13 @@ int qemu_connect_wrap(int sockfd, const struct
> > sockaddr *addr,
> > socklen_t addrlen)
> > {
> > int ret;
> > -ret = connect(sockfd, addr, addrlen);
> > +SOCKET s = _get_osfhandle(sockfd);
> > +
> > +if (s == INVALID_SOCKET) {
> > +return -1;
> > +}
> > +
> > +ret = connect(s, addr, addrlen);
>
>
> Previously you passed int sockfd and now you convert this sockfd to a SOCKET
> s and you can pass this as an alternative? Or was sockfd before this patch a
> SOCKET??
yes, sockfd was in fact a SOCKET.
Previous to this patch, a SOCKET is cast to int, as a fake fd, and
back to SOCKET as necessary. The whole point of this patch is to avoid
mixing SOCKET & fd space, instead a SOCKET is associated with a real
CRT fd.
thanks
--
Marc-André Lureau
Re: [PATCH] scsi: megasas: Internal cdbs have 16-byte length
Am 03.03.23 um 16:10 schrieb Guenter Roeck:
> On 3/3/23 01:02, Fiona Ebner wrote:
>> Am 28.02.23 um 18:11 schrieb Guenter Roeck:
>>> Host drivers do not necessarily set cdb_len in megasas io commands.
>>> With commits 6d1511cea0 ("scsi: Reject commands if the CDB length
>>> exceeds buf_len") and fe9d8927e2 ("scsi: Add buf_len parameter to
>>> scsi_req_new()"), this results in failures to boot Linux from affected
>>> SCSI drives because cdb_len is set to 0 by the host driver.
>>> Set the cdb length to its actual size to solve the problem.
>>>
>>
>> Tested-by: Fiona Ebner
>>
>> But I do have a question:
>>
>>> Signed-off-by: Guenter Roeck
>>> ---
>>> hw/scsi/megasas.c | 14 ++
>>> 1 file changed, 2 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
>>> index 9cbbb16121..d624866bb6 100644
>>> --- a/hw/scsi/megasas.c
>>> +++ b/hw/scsi/megasas.c
>>> @@ -1780,7 +1780,7 @@ static int megasas_handle_io(MegasasState *s,
>>> MegasasCmd *cmd, int frame_cmd)
>>> uint8_t cdb[16];
>>> int len;
>>> struct SCSIDevice *sdev = NULL;
>>> - int target_id, lun_id, cdb_len;
>>> + int target_id, lun_id;
>>> lba_count = le32_to_cpu(cmd->frame->io.header.data_len);
>>> lba_start_lo = le32_to_cpu(cmd->frame->io.lba_lo);
>>> @@ -1789,7 +1789,6 @@ static int megasas_handle_io(MegasasState *s,
>>> MegasasCmd *cmd, int frame_cmd)
>>> target_id = cmd->frame->header.target_id;
>>> lun_id = cmd->frame->header.lun_id;
>>> - cdb_len = cmd->frame->header.cdb_len;
>>> if (target_id < MFI_MAX_LD && lun_id == 0) {
>>> sdev = scsi_device_find(>bus, 0, target_id, lun_id);
>>> @@ -1804,15 +1803,6 @@ static int megasas_handle_io(MegasasState *s,
>>> MegasasCmd *cmd, int frame_cmd)
>>> return MFI_STAT_DEVICE_NOT_FOUND;
>>> }
>>> - if (cdb_len > 16) {
>>> - trace_megasas_scsi_invalid_cdb_len(
>>> - mfi_frame_desc(frame_cmd), 1, target_id, lun_id, cdb_len);
>>> - megasas_write_sense(cmd, SENSE_CODE(INVALID_OPCODE));
>>> - cmd->frame->header.scsi_status = CHECK_CONDITION;
>>> - s->event_count++;
>>> - return MFI_STAT_SCSI_DONE_WITH_ERROR;
>>> - }
>>
>> Shouldn't we still fail when cmd->frame->header.cdb_len > 16? Or is the
>> consequence of
>>
>>> Host drivers do not necessarily set cdb_len in megasas io commands.
>>
>> that this can be uninitialized memory and we need to assume it was not
>> explicitly set?
>>
>
> I doubt that real hardware uses or checks the field for the affected
> commands
> because that would be pointless, but it is really up to you to decide how
> you want to handle it.
>
> Guenter
Okay, thank you for the explanation!
>
>> Best Regards,
>> Fiona
>>
>>> -
>>> cmd->iov_size = lba_count * sdev->blocksize;
>>> if (megasas_map_sgl(s, cmd, >frame->io.sgl)) {
>>> megasas_write_sense(cmd, SENSE_CODE(TARGET_FAILURE));
>>> @@ -1823,7 +1813,7 @@ static int megasas_handle_io(MegasasState *s,
>>> MegasasCmd *cmd, int frame_cmd)
>>> megasas_encode_lba(cdb, lba_start, lba_count, is_write);
>>> cmd->req = scsi_req_new(sdev, cmd->index,
>>> - lun_id, cdb, cdb_len, cmd);
>>> + lun_id, cdb, sizeof(cdb), cmd);
>>> if (!cmd->req) {
>>> trace_megasas_scsi_req_alloc_failed(
>>> mfi_frame_desc(frame_cmd), target_id, lun_id);
>>
>
>
>
Re: [PATCH v3 09/14] hw/net/tulip: Finish QOM conversion
On Tue, Feb 28, 2023 at 9:39 PM Philippe Mathieu-Daudé
wrote:
>
> Hi Jason, do you Ack this patch?
Yes.
Acked-by: Jason Wang
Thanks
>
> On 13/2/23 19:43, Philippe Mathieu-Daudé wrote:
> > Use the TULIP() and DEVICE() QOM type-checking macros.
> > Remove uses of DO_UPCAST().
> >
> > Signed-off-by: Philippe Mathieu-Daudé
> > ---
> > hw/net/tulip.c | 20 +++-
> > 1 file changed, 11 insertions(+), 9 deletions(-)
> >
> > diff --git a/hw/net/tulip.c b/hw/net/tulip.c
> > index 915e5fb595..990507859d 100644
> > --- a/hw/net/tulip.c
> > +++ b/hw/net/tulip.c
> > @@ -19,7 +19,10 @@
> > #include "net/eth.h"
> >
> > struct TULIPState {
> > +/*< private >*/
> > PCIDevice dev;
> > +/*< public >*/
> > +
> > MemoryRegion io;
> > MemoryRegion memory;
> > NICConf c;
> > @@ -959,7 +962,7 @@ static void tulip_fill_eeprom(TULIPState *s)
> >
> > static void pci_tulip_realize(PCIDevice *pci_dev, Error **errp)
> > {
> > -TULIPState *s = DO_UPCAST(TULIPState, dev, pci_dev);
> > +TULIPState *s = TULIP(pci_dev);
> > uint8_t *pci_conf;
> >
> > pci_conf = s->dev.config;
> > @@ -967,7 +970,7 @@ static void pci_tulip_realize(PCIDevice *pci_dev, Error
> > **errp)
> >
> > qemu_macaddr_default_if_unset(>c.macaddr);
> >
> > -s->eeprom = eeprom93xx_new(_dev->qdev, 64);
> > +s->eeprom = eeprom93xx_new(DEVICE(pci_dev), 64);
> > tulip_fill_eeprom(s);
> >
> > memory_region_init_io(>io, OBJECT(>dev), _ops, s,
> > @@ -983,27 +986,26 @@ static void pci_tulip_realize(PCIDevice *pci_dev,
> > Error **errp)
> >
> > s->nic = qemu_new_nic(_tulip_info, >c,
> > object_get_typename(OBJECT(pci_dev)),
> > - pci_dev->qdev.id, s);
> > + DEVICE(pci_dev)->id, s);
> > qemu_format_nic_info_str(qemu_get_queue(s->nic), s->c.macaddr.a);
> > }
> >
> > static void pci_tulip_exit(PCIDevice *pci_dev)
> > {
> > -TULIPState *s = DO_UPCAST(TULIPState, dev, pci_dev);
> > +TULIPState *s = TULIP(pci_dev);
> >
> > qemu_del_nic(s->nic);
> > qemu_free_irq(s->irq);
> > -eeprom93xx_free(_dev->qdev, s->eeprom);
> > +eeprom93xx_free(DEVICE(s), s->eeprom);
> > }
> >
> > static void tulip_instance_init(Object *obj)
> > {
> > -PCIDevice *pci_dev = PCI_DEVICE(obj);
> > -TULIPState *d = DO_UPCAST(TULIPState, dev, pci_dev);
> > +TULIPState *s = TULIP(obj);
> >
> > -device_add_bootindex_property(obj, >c.bootindex,
> > +device_add_bootindex_property(obj, >c.bootindex,
> > "bootindex", "/ethernet-phy@0",
> > - _dev->qdev);
> > + DEVICE(obj));
> > }
> >
> > static Property tulip_properties[] = {
>
Re: [Libguestfs] [PATCH] docs: Prefer 'cookie' over 'handle'
On Sat, Mar 04, 2023 at 10:03:46PM +0200, Nir Soffer wrote:
> On Sat, Mar 4, 2023 at 12:15 AM Eric Blake wrote:
> > Makes no difference to implementations (other than older code
> > still using 'handle' may be slightly harder to tie back to the spec).
>
> To avoid confusion with older code that carefully used "handle" to match
> the spec, maybe add a note that "cookie" was named "handle" before?
Yes, this. I'm happy with renaming it cookie (it makes sense), but there
is a *lot* of stuff out there that calls it "handle" (including a
wireshark plugin) and it would be confusing if that link isn't available
anywhere.
--
w@uter.{be,co.za}
wouter@{grep.be,fosdem.org,debian.org}
I will have a Tin-Actinium-Potassium mixture, thanks.
Re: [PATCH v2 5/6] spec: Introduce NBD_FLAG_BLOCK_STATUS_PAYLOAD
On Fri, Mar 03, 2023 at 04:40:38PM -0600, Eric Blake wrote:
> On Wed, Feb 22, 2023 at 12:05:44PM +0200, Wouter Verhelst wrote:
> > On Mon, Nov 14, 2022 at 04:46:54PM -0600, Eric Blake wrote:
> > > Simple reply message
> > >
> > > @@ -1232,6 +1235,19 @@ The field has the following format:
> > >will be faster than a regular write). Clients MUST NOT set the
> > >`NBD_CMD_FLAG_FAST_ZERO` request flag unless this transmission flag
> > >is set.
> > > +- bit 12, `NBD_FLAG_BLOCK_STATUS_PAYLOAD`: Indicates that the server
> > > + understands the use of the `NBD_CMD_FLAG_PAYLOAD_LEN` flag to
> > > + `NBD_CMD_BLOCK_STATUS` to allow the client to request that the
> > > + server filters its response to a specific subset of negotiated
> > > + metacontext ids passed in via a client payload, rather than the
> > > + default of replying to all metacontext ids. Servers MUST NOT
> > > + advertise this bit unless the client successfully negotiates
> > > + extended headers via `NBD_OPT_EXTENDED_HEADERS`, and SHOULD NOT
> > > + advertise this bit in response to `NBD_OPT_EXPORT_NAME` or
> > > + `NBD_OPT_GO` if the client does not negotiate metacontexts with
> > > + `NBD_OPT_SET_META_CONTEXT`; clients SHOULD NOT set the
> > > + `NBD_CMD_FLAG_PAYLOAD_LEN` flag for `NBD_CMD_BLOCK_STATUS` unless
> > > + this transmission flag is set.
> >
> > Given that we are introducing this at the same time as the extended
> > headers (and given that we're running out of available flags in this
> > 16-bit field), isn't it better to make the ability to understand
> > PAYLOAD_LEN be implied by extended headers? Or is there a use case for
> > implementing extended headers but not a payload length on
> > CMD_BLOCK_STATUS that I'm missing?
>
> In my proof of implementation, this was a distinct feature addition on
> top of 64-bit headers.
>
> It is easy to write a server that does not want to deal with a client
> payload on a block status request (for example, a server that only
> knows about one metacontext, and therefore never needs to deal with a
> client wanting a subset of negotiated metacontexts). Forcing a server
> to have to support a client-side filtering request on block status
> commands, merely because the server now supports 64-bit lengths,
> seemed like a stretch too far, which is why I decided to use a feature
> bit for this one.
Okay, yes, that makes sense. Thanks.
--
w@uter.{be,co.za}
wouter@{grep.be,fosdem.org,debian.org}
I will have a Tin-Actinium-Potassium mixture, thanks.
Re: [PATCH v2 3/6] spec: Add NBD_OPT_EXTENDED_HEADERS
On Fri, Mar 03, 2023 at 04:36:41PM -0600, Eric Blake wrote:
> On Wed, Feb 22, 2023 at 11:49:18AM +0200, Wouter Verhelst wrote:
> > On Mon, Nov 14, 2022 at 04:46:52PM -0600, Eric Blake wrote:
[...]
> > > + Note that even when extended headers are in use, the client MUST be
> > > + prepared for the server to use either the compact or extended chunk
> > > + type, regardless of whether the client's hinted effect length was
> > > + more or less than 32 bits; but the server MUST use exactly one of
> > > + the two chunk types per negotiated metacontext ID.
> >
> > Is this last paragraph really a good idea? I would think it makes more
> > sense to require the new format if we're already required to support it
> > on both sides anyway.
>
> My proof of implementation was easier to code when I didn't have to
> resize the block status reply sizing in the same patch as adding the
> 64-bit headers. But if you think requiring the 64-bit reply type
> always (and forbidding the 32-bit reply) when extended headers are in
> force, that's also possible.
Intuitively, this sounds off. It would seem to me that it's easier to do
if you don't have to have a conditional on each received data packet.
But maybe I'm missing something -- I haven't done an implementation yet,
anyway.
--
w@uter.{be,co.za}
wouter@{grep.be,fosdem.org,debian.org}
I will have a Tin-Actinium-Potassium mixture, thanks.
Re: [PATCH v2 2/6] spec: Tweak description of maximum block size
On Fri, Mar 03, 2023 at 04:26:53PM -0600, Eric Blake wrote: > On Tue, Feb 21, 2023 at 05:21:37PM +0200, Wouter Verhelst wrote: > > Hi Eric, > > > > Busy days, busy times. Sorry about the insane delays here. > > No problem; I've been tackling other things in the meantime too, so > this extension has taken far longer than I planned for more reasons > than just slow review time. > > > > > On Mon, Nov 14, 2022 at 04:46:51PM -0600, Eric Blake wrote: > > > Commit 9f30fedb improved the spec to allow non-payload requests that > > > exceed any advertised maximum block size. Take this one step further > > > by permitting the server to use NBD_EOVERFLOW as a hint to the client > > > when a request is oversize (while permitting NBD_EINVAL for > > > back-compat), and by rewording the text to explicitly call out that > > > what is being advertised is the maximum payload length, not maximum > > > block size. This becomes more important when we add 64-bit > > > extensions, where it becomes possible to extend `NBD_CMD_BLOCK_STATUS` > > > to have both an effect length (how much of the image does the client > > > want status on - may be larger than 32 bits) and an optional payload > > > length (a way to filter the response to a subset of negotiated > > > metadata contexts). In the shorter term, it means that a server may > > > (but not must) accept a read request larger than the maximum block > > > size if it can use structured replies to keep each chunk of the > > > response under the maximum payload limits. > > > --- > > > doc/proto.md | 127 +-- > > > 1 file changed, 73 insertions(+), 54 deletions(-) > > > > > > diff --git a/doc/proto.md b/doc/proto.md > > > index 8f08583..53c334a 100644 > > > --- a/doc/proto.md > > > +++ b/doc/proto.md > > > @@ -745,8 +745,8 @@ text unless the client insists on TLS. > > > > > > During transmission phase, several operations are constrained by the > > > export size sent by the final `NBD_OPT_EXPORT_NAME` or `NBD_OPT_GO`, > > > -as well as by three block size constraints defined here (minimum, > > > -preferred, and maximum). > > > +as well as by three block size constraints defined here (minimum > > > +block, preferred block, and maximum payload). > > > > > > If a client can honour server block size constraints (as set out below > > > and under `NBD_INFO_BLOCK_SIZE`), it SHOULD announce this during the > > > @@ -772,15 +772,15 @@ learn the server's constraints without committing > > > to them. > > > > > > If block size constraints have not been advertised or agreed on > > > externally, then a server SHOULD support a default minimum block size > > > -of 1, a preferred block size of 2^12 (4,096), and a maximum block size > > > -that is effectively unlimited (0x, or the export size if that > > > -is smaller), while a client desiring maximum interoperability SHOULD > > > -constrain its requests to a minimum block size of 2^9 (512), and limit > > > -`NBD_CMD_READ` and `NBD_CMD_WRITE` commands to a maximum block size of > > > -2^25 (33,554,432). A server that wants to enforce block sizes other > > > -than the defaults specified here MAY refuse to go into transmission > > > -phase with a client that uses `NBD_OPT_EXPORT_NAME` (via a hard > > > -disconnect) or which uses `NBD_OPT_GO` without requesting > > > +of 1, a preferred block size of 2^12 (4,096), and a maximum block > > > +payload size that is at least 2^25 (33,554,432) (even if the export > > > +size is smaller); while a client desiring maximum interoperability > > > +SHOULD constrain its requests to a minimum block size of 2^9 (512), > > > +and limit `NBD_CMD_READ` and `NBD_CMD_WRITE` commands to a maximum > > > +block size of 2^25 (33,554,432). A server that wants to enforce block > > > +sizes other than the defaults specified here MAY refuse to go into > > > +transmission phase with a client that uses `NBD_OPT_EXPORT_NAME` (via > > > +a hard disconnect) or which uses `NBD_OPT_GO` without requesting > > > > This does more than what the commit message says: it also limits payload > > size from 0x to 2^25. We already have a "A server that desires > > maximum interoperability" clause that mentions the 2^25, so I'm not > > entirely sure why we need to restrict that for the default cause. > > > > Remember, apart from specifying how something should be implemented, the > > spec also documents current and historic behavior. I am probably > > convinced that it makes more sense to steer people towards limiting to > > 2^25, but it should be done in such a way that servers which accept a > > 0x block size are not suddenly noncompliant. I don't think this > > does that. > > I'll have to play more with the wording here. A client shouldn't send > larger write requests to a server without knowing the server will > accept it (because traditional servers disconnect instead - the > alternative is to read the client's entire payload, and the larger the >
Re: [PATCH v2 1/6] spec: Recommend cap on NBD_REPLY_TYPE_BLOCK_STATUS length
On Fri, Mar 03, 2023 at 04:17:40PM -0600, Eric Blake wrote:
> On Fri, Dec 16, 2022 at 10:32:01PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > s-o-b line missed.
>
> I'm not sure if the NBD project has a strict policy on including one,
> but I don't mind adding it.
I've never required it, mostly because it's something that I myself
always forget, too, so, *shrug*.
(if there were a way in git to make it add that automatically, that
would help; I've looked but haven't found it)
--
w@uter.{be,co.za}
wouter@{grep.be,fosdem.org,debian.org}
I will have a Tin-Actinium-Potassium mixture, thanks.
