[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4 9/9] scsi/scsi_bus: fix races in REPORT LUNS
From: |
Maxim Levitsky |
Subject: |
Re: [PATCH v4 9/9] scsi/scsi_bus: fix races in REPORT LUNS |
Date: |
Wed, 09 Sep 2020 11:20:24 +0300 |
User-agent: |
Evolution 3.36.3 (3.36.3-1.fc32) |
On Tue, 2020-09-08 at 16:27 +0100, Stefan Hajnoczi wrote:
> On Mon, Aug 31, 2020 at 06:01:24PM +0300, Maxim Levitsky wrote:
> > Currently scsi_target_emulate_report_luns iterates
> > over child devices list twice, and there is guarantee, that
> > it will not be changed meanwhile.
> >
> > This reason for two loops is that it needs to know how much memory
> > to allocate.
> >
> > Avoid this by iterating once, and allocating the memory for the output
> > dynamically with reserving enought memory so that in practice it won't
> > be reallocated often.
> >
> > Bugzilla for reference: https://bugzilla.redhat.com/show_bug.cgi?id=1866707
>
> "Buglink:" is the tag name documented in
> https://wiki.qemu.org/Contribute/SubmitAPatch#Write_a_meaningful_commit_message
Noted
>
> > static bool scsi_target_emulate_report_luns(SCSITargetReq *r)
> > {
> > BusChild *kid;
> > - int i, len, n;
> > int channel, id;
> > - bool found_lun0;
> > + uint8_t tmp[8] = {0};
> > + int len = 0;
> > +
> > + /* reserve space for 63 LUNs*/
> > + GByteArray *buf = g_byte_array_sized_new(512);
> >
> > if (r->req.cmd.xfer < 16) {
> > return false;
>
> buf is leaked.
Oops, will fix
>
> > @@ -460,46 +466,36 @@ static bool
> > scsi_target_emulate_report_luns(SCSITargetReq *r)
> > }
> > channel = r->req.dev->channel;
> > id = r->req.dev->id;
> > - found_lun0 = false;
> > - n = 0;
> >
> > - rcu_read_lock();
> >
> > - QTAILQ_FOREACH_RCU(kid, &r->req.bus->qbus.children, sibling) {
> > - DeviceState *qdev = kid->child;
> > - SCSIDevice *dev = SCSI_DEVICE(qdev);
> > + /* add size (will be updated later to correct value */
> > + g_byte_array_append(buf, tmp, 8);
> > + len += 8;
>
> Can g_byte_array_size() be used instead of keeping a len local variable?
Glib don't seem to have this function, I checked the docs.
Its seems that they want to convert it to GBytes which is basically immutible
verion
of GByteArray and it does have g_bytes_get_size.
I decided that a local variable while ugly is still better that this.
I haven't wrote much code that uses Glib, so I might have missed something
though.
I had read this reference:
https://developer.gnome.org/glib/stable/glib-Byte-Arrays.html
>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Best regards,
Maxim Levitsky