Do we know the rationale behind setting disk->physpath to "" in the
below code in libxl? Hopefully there is more to it than to avoid a
potential null pointer crash at a later point :)
int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid,
libxl_device_disk *disk) {
int num, i;
uint32_t stubdomid;
libxl_device_disk *disks;
int ret = ERROR_FAIL;
if (!disk->physpath) {
disk->physpath = "";
disk->phystype = PHYSTYPE_PHY;
}
Here is why I ask - I moved the disk file path validation code to
libxl and was looking at the code in that area and stumbled on this.
With the above logic the validate code I wrote will fail for cdrom
insert as I am validating the block device path also. So, if I am
going to special case this I would like to first understand why I
would want to do that.
Kamala
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
On Fri, 2011-01-14 at 22:37 +0000, Kamala Narasimhan wrote:> Do we know the rationale behind setting disk->physpath to "" in the > below code in libxl? Hopefully there is more to it than to avoid a > potential null pointer crash at a later point :)I think it might indicate an empty CDROM drive in the HVM case. Arguably a PHYSTYPE_EMPTY/NONE or some such concept might have been clearer. "hg annotate" should lead you to the original commit. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On Sat, Jan 15, 2011 at 10:03 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:> On Fri, 2011-01-14 at 22:37 +0000, Kamala Narasimhan wrote: >> Do we know the rationale behind setting disk->physpath to "" in the >> below code in libxl? Hopefully there is more to it than to avoid a >> potential null pointer crash at a later point :) > > I think it might indicate an empty CDROM drive in the HVM case. Arguably > a PHYSTYPE_EMPTY/NONE or some such concept might have been clearer. "hg > annotate" should lead you to the original commit. >In that case, if we want to keep "", per Gianni Tedesco we would need the below fix as the string would get freed later - diff -r ce208811f540 tools/libxl/libxl.c --- a/tools/libxl/libxl.c Thu Jan 13 01:26:44 2011 +0000 +++ b/tools/libxl/libxl.c Fri Jan 14 17:39:16 2011 -0500 @@ -1676,7 +1676,7 @@ int libxl_cdrom_insert(libxl_ctx *ctx, u int ret = ERROR_FAIL; if (!disk->physpath) { - disk->physpath = ""; + disk->physpath = strdup(""); disk->phystype = PHYSTYPE_PHY; } disks = libxl_device_disk_list(ctx, domid, &num); Gianni might have to sign off on it or at least ack it but here is mine if that is needed too - Signed-off-by: Kamala Narasimhan <kamala.narasimhan@citrix.com> Kamala _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On Sat, 15 Jan 2011, Ian Campbell wrote:> On Fri, 2011-01-14 at 22:37 +0000, Kamala Narasimhan wrote: > > Do we know the rationale behind setting disk->physpath to "" in the > > below code in libxl? Hopefully there is more to it than to avoid a > > potential null pointer crash at a later point :) > > I think it might indicate an empty CDROM drive in the HVM case. Arguably > a PHYSTYPE_EMPTY/NONE or some such concept might have been clearer. "hg > annotate" should lead you to the original commit. >Yes, that is what it means: libxl callers set physpath to NULL to eject a cdrom, however libxl_device_disk_add cannot handle NULL physpath''s. I think we should introduce PHYSTYPE_EMPTY and start using it in libxl.c:libxl_event_get_disk_eject_info and xl_cmdimpl.c:cd_insert. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Just realized that we missed this one. IanJ mentioned that he wouldn''t want any interface changes for 4.1 in which case the below change is necessary to fix a bug. Kamala On Sat, Jan 15, 2011 at 1:57 PM, Kamala Narasimhan <kamala.narasimhan@gmail.com> wrote:> On Sat, Jan 15, 2011 at 10:03 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote: >> On Fri, 2011-01-14 at 22:37 +0000, Kamala Narasimhan wrote: >>> Do we know the rationale behind setting disk->physpath to "" in the >>> below code in libxl? Hopefully there is more to it than to avoid a >>> potential null pointer crash at a later point :) >> >> I think it might indicate an empty CDROM drive in the HVM case. Arguably >> a PHYSTYPE_EMPTY/NONE or some such concept might have been clearer. "hg >> annotate" should lead you to the original commit. >> > > In that case, if we want to keep "", per Gianni Tedesco we would need > the below fix as the string would get freed later - > > diff -r ce208811f540 tools/libxl/libxl.c > --- a/tools/libxl/libxl.c Thu Jan 13 01:26:44 2011 +0000 > +++ b/tools/libxl/libxl.c Fri Jan 14 17:39:16 2011 -0500 > @@ -1676,7 +1676,7 @@ int libxl_cdrom_insert(libxl_ctx *ctx, u > int ret = ERROR_FAIL; > > if (!disk->physpath) { > - disk->physpath = ""; > + disk->physpath = strdup(""); > disk->phystype = PHYSTYPE_PHY; > } > disks = libxl_device_disk_list(ctx, domid, &num); > > Gianni might have to sign off on it or at least ack it but here is > mine if that is needed too - > > Signed-off-by: Kamala Narasimhan <kamala.narasimhan@citrix.com> > > Kamala >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Kamala Narasimhan writes ("Re: [Xen-devel]
libxl_cdrom_insert"):> Just realized that we missed this one. IanJ mentioned that he
> wouldn''t want any interface changes for 4.1 in which case the
below
> change is necessary to fix a bug.
Oh, yes, sorry. I have applied it, thanks.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel