Re: [PATCH v4 9/9] scsi/scsi_bus: fix races in REPORT LUNS

[Maxim Levitsky <mlevitsk@redhat.com>]

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